We previously deleted the call to `unregister()` after the refinery is
successful on a16dd562d9.
It was considered redundant since the `unregister()` call happens in
`subscribe_on_stop()`, which for refine session is called directly after
the refinery process finishes (after `channel.stop()`).
However, in a highly async environment the `unregister()` call in
`subscribe_on_stop()` can be called before the call to `move_host()`, meaning
that the host would then be stuck in the `Moving` state.
We have fixed this by specifying that `unregister` should only be called
in `subscribe_on_stop()` to peers that are not part of a refine session.
We seperately call `unregister` after `move_host` in the refinery.
This commit also fixes some documentation.
Note: the call to `unregister()` is highly fragile and can lead to race
conditions. We are working to replace this with something more robust
(like `tombstone()`).
- All rpc use same fn to perform requests towards darkfid\n- Moved all rpc related Drk fns to rpc.rs\n- Fixed subscribe where if darkfid went off, drk subscription errored and drk hanged
unregister() will get called when the refine session channel disconnects
in session::remove_sub_on_stop(). Calling it here is actually dangerous
and creates rare race conditions.
We organize this functionality into distinct methods which get called
higher up, for example rather than manually resizing inside of store(),
we call resize() after we call store().
This is about reducing the "critical section" where locks are held and
using function scopes to ensure locks are released as quickly as possible.
Rationale: using a sync Mutex wherever possible is the recommended
method.
Additionally, using a sync Mutex here fixes some really weird fairness
behaviors we observed in the smol::lock::RwLock where writers in the
priority queue were occassionally ignored.
We don't need to use an AsyncMutex here since we're not holding across .await points or for long periods of time.
Using a sync Mutex here also fixes some really weird fairness behaviors we observed in the smol::lock::Mutex where writers in the priority queue were occasionally getting ignored. This was apparently not a deadlock since subsequent and prior readers and writers were able to access the data with no problems.
We don't need to use an AsyncMutex here since we're not holding across .await points or for long periods of time.
Using a sync Mutex here also fixes some really weird fairness behaviors we observed in the smol::lock::Mutex where writers in the priority queue were occasionally getting ignored. This was apparently not a deadlock since subsequent and prior readers and writers were able to access the data with no problems.
We check whether there are any remaining channels when we remove a
channel in remove_sub_on_stop(). If the channel list is empty,
we call notify() on disconnect_pubisher and set its inner value to true.
Note that this only signals when we do not have any connections, and
does not update to false when new connections are formed.
Previously we were locking the greylist to prevent the index from changing
while performing a handshake. This isn't necessary. It is acceptable
for the greylist index to change while the handshake is ongoing.
The thing we don't want to happen is that the addr we are operating on
is somehow removed from the list- this should never happen since it has
been marked as "Refine" in the HostRegistry. Therefore, calling unwrap()
on get_index_at_addr() should pass 100% of the time (and if it doesn't,
that indicates a logic failure elsewhere in the code).
RwLock is overkill in this case since there is only ever one reader and
one writer.
For more info why a sync Mutex is appropiate in this case, see commit 65a8e9a44fa3c835158550e7eb5b5e1946e3028f
Locks should be sync for simple data operations and async if:
1. the lock must be held across an .await point
2. the new data being stored in the lock is calculated from data already inside the lock
Since ipv6_available is a fast and simple data operation it is more appropiate to use
a sync Mutex here.
unwrap() can panic in the rare case that a node disconnects while we are
in the middle of accepting a connection from it.
In the case that this happens we should instead just exit with an error.
Selecting a random port allows us to run the test on the same machine
concurrently without conflicts.
Note that selecting a port for the seed node and seed sync session is
fairly safe, since we immediately start the node after the port is
selected, meaning most of the time the same port will not come up as
available if another test is being run concurrently.
However there is slightly less certainty with the manual session, since
we first generate a list of available ports, and then start them in a
loop, creating a slight delay and increasing the probability that
another test, run concurrently, could select one of those ports as
available.
Gas-based limit implementation to retrieve unproposed transactions. The limit is maintained by the constant 'GAS_LIMIT_UNPROPOSED_TXS', currently calculated as the product of the average total gas used and a gas limit multiplier equivalent to the existing TX_CAP value (i.e, 50).
The average total gas was obtained through analysis of empirical transaction test data, using a gas analysis tool.
The aim of this approach is to allow a gradual and controlled transition towards an optimal gas-based system. This minimizes potential adverse effects brought by changes to TX_CAP's implementation and provides the benefits of using gas to limit the number of txs received. This implementation will be fine-tuned until the discovery of the most efficient formula for determining the unproposed transactions' gas limit.
Tests have been added to verify the implementation's correctness by running transactions against it. To run the tests, run the following command from the bin/darkfid directory:
cargo test --release --bin darkfid tests::unproposed_txs
- Fix issue where `half` was calculated incorrectly for odd values of
`value`
- Fix issue where sending a `value` of 1 and `half_split` of true would
result in two empty value transfers
Note that this is only a client issue. The wasm back-end would cause the
invalid transfer to fail as the tx outputs from the client would not
match the inputs
clippy linting at the workspace level for all crates
Configure all workspaces to use lints from the top-level Cargo.toml
file
Add example lints that the project could configure to improve security
and reliability.
Configure lints to warn level. Using deny level makes other binaries
fail to compile if even one of them has a failure.
No lints are added in this commit. Future changes can enable and fix
lints
1. Shutting down all channels in p2p.stop() is redundant since they will
be cleaned up by the session shutdown.
2. subscribe_stop().expect(..) could panic in the case that the node we
are connecting to disconnects at this exact moment (triggering a
`ChannelStopped` error).
