feat(utxo): prioritize electrum connections #1966
Commits on Aug 26, 2023
Commits on Sep 1, 2023
Commits on Sep 4, 2023
Commits on Sep 7, 2023
Commits on Sep 8, 2023
Commits on Sep 10, 2023
Commits on Sep 11, 2023
Commits on Sep 21, 2023
Commits on Sep 22, 2023
Commits on Sep 25, 2023
Commits on Sep 27, 2023
Commits on Sep 28, 2023
Commits on Sep 29, 2023
Commits on Oct 1, 2023
Commits on Oct 3, 2023
Commits on Jan 30, 2024
-
Merge branch 'dev' into 1520-prioritize-nodes
# Conflicts: # mm2src/adex_cli/Cargo.lock # mm2src/coins/utxo.rs # mm2src/coins/utxo/rpc_clients.rs # mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs # mm2src/coins/utxo/utxo_tests.rs # mm2src/coins/z_coin.rs
-
-
-
Commits on Jan 31, 2024
-
merge ConnectionManagerSelectiveQueue with ConnectionManagerSelective…
…State and other minor changes
Commits on Feb 6, 2024
-
manage sending multiple sub requests for same scripthash and other fi…
…xes/improvements
Commits on Feb 12, 2024
-
Merge remote-tracking branch 'origin/dev' into 1520-prioritize-nodes
# Conflicts: # mm2src/coins/utxo/rpc_clients.rs # mm2src/coins/utxo/utxo_balance_events.rs
-
Commits on Feb 15, 2024
-
-
rename guarded to state_guard and move connect impl to impl trait for…
… ConnectionManagerMultiple
-
move get_connection_ctx_mut and get_connection_ctx to ConnectionManag…
…erMultipleState impl
Commits on Feb 16, 2024
Commits on Feb 17, 2024
-
Refactor ConnectionManagerMultiple & ConnectionManagerSelective
This commit refactors `multiple` and `selective` connection managers. Now the inner impl managers' `Arc` implements `ConnectionManagerTrait` while the outer wrapper only implements `Deref` to return the `Arc`. Also some methods that only used the `state: MutexGuard` are now moved to `ConnectionManager[Multiple|Selective]State`.
Commits on Mar 22, 2024
Commits on Apr 18, 2024
Commits on Apr 19, 2024
Commits on Apr 21, 2024
Commits on Apr 22, 2024
Commits on Apr 30, 2024
Commits on May 13, 2024
-
to avoid 2 connections starting in close proximity and cancelling each other (rare race condition) this essentially merges the `is_connected` & setting the tx end of the conneciton in one guard
-
-
-
add back scripthash notification sender since it's used to send Subsc…
…ribeToAddresses notifications unfortunately :(
Commits on May 20, 2024
Commits on May 22, 2024
Commits on May 23, 2024
-
bound establish_connecion_loop method with a timeout
the total time this method might take is at most the timeout decided in the connection settings, which is the time needed to establish the connection and query for the version
-
-
Commits on May 29, 2024
Commits on May 30, 2024
-
-
temp fix: leak the abortable system to keep its children systems alive
the abortable system that created the children abortable systems for the connections were dropped and that resulted in a non functioning connection abortable systems
-
-
Commits on May 31, 2024
-
don't query for the version again
electrum servers don't like it when the same connection query for the vesion again, they expect us to store the version and never ask about it again
-
don't create an extra child subsystem, use the parent directly
the middle subsystem wasn't used anywhere, thus was dropped and caused all the connection subsystems to get aborted.
-
wait till one of the electrum server connects, applied while initiali…
…zation and not in tests
-
-
Commits on Jun 2, 2024
Commits on Jun 4, 2024
-
-
adapt test_unavailable_electrum_proto_version to the new design
the new design doesn't break the coin initilization if the electrum servers fail for whatever reason. Create the coin and make sure that each server fails version check when queried
-
Commits on Jun 23, 2024
Commits on Jun 28, 2024
-
-
use hashset instead of vec for the subs list
execution-wise this should make no difference (we won't ever get the address twice and event if we did it should be a benign side-effect), but it makes the code logically easier to understand
-
package
disconnected_atandnext_suspend_timein one mutexThese two pieces of info are always used & updated together, put them into a single mutex.
-
-
-
we are trying to be easy on the servers anyways, so avoid unrecoverable labelling when possible
Commits on Jul 1, 2024
-
-
adex cli: use closer versions in cargo toml
to avoid duplications as much as possible
-
-
-
Commits on Jul 29, 2024
Commits on Aug 13, 2024
Commits on Aug 14, 2024
Commits on Aug 15, 2024
-
endlessly try
electrum_request_multialso parallelize it so the overall timeout is reducued (ELECTRUM_REQUEST_TIMEOUT instead of N * ELECTRUM_REQUEST_TIMEOUT, assuming at least one server is actually working)
-
-
Commits on Aug 29, 2024
-
trigger disconnecting a connection if it errored
this helps refreshing and trying new connections in selective mode
Commits on Sep 2, 2024
-
-
some macros to simplify if-let & let-else patterns
let-else isn't formatabble by cargo fmt and the `Some()` around the variable kind of reduces the importance of it. we also execute some important code inside the if-let block but it doesn't look that important since it is inside an if and we continue normally otherwise. The point is, most of these options should never be None (e.g. `connections().get(address)`), we can either panic there or just ignore the error. Ignoring it is safer but fills the code with a lot of if-let let-else patterns. The added macros makes the refactor somewhat similar to `?` in `ret_option()?`
-
only aquire write lock when needed
over the maintained connections set. One could find the change of quesstion would benifit us with a fewer write lock acquisitions when we have a low max_connected value with a lot of very robust servers. The previous code would have acquired the lock periodically for each checked server
-
try connections sequentially on the background task
this takes more time but respects the number of connections open at a single time by mm2. trying all the connections in parallel can cause a lot of open file descriptors on mobile devices
-
I used trigger disconnect because I thought we don't have access to the event handlers to notify them, so the simplist way then was to just let the connection disconnect on its own when the sender is taken out (in trigger_disconnect). but since we have the event_handlers already in electurm client, we can just disconnect right away and call them
Commits on Sep 3, 2024
-
resolve fixme: version negotiation logic is correct
looks like after the refactor i miss understood what `negotiate_version` flag is for. it's for running a version negotiation actually, i.e. not fail if the versions doesn't match (we will accept the server even though it has a bad version).
-
-
-
revert to trying connections in parallel in background task (to some …
…extent) so looks like trying connections sequntially introduced a long wait time till we got a valid working connection. this happens when the list of electrums are bunch of faulty ones and the last of them is working fine. this didn't hurt the correctness in anyway, but made it kind of slow to bootstrap (finding the first working connection), and since tests doesn't try requests forever (forever-waiting until the request is successful is only used on cfg(not(test)) environments), this meant a lot of tests will fail to find a working connection soon enough and will panic on the first request. we might want to find a strong fix for this. In this commit, however, the fix to mitigate this issue is to re-intoduce concurrency to the point that the number of open connections at any time is less than or equal `max_connected` specified in the enable request. For multiple policy (min=1, max=len(servers)), this is just trying all the servers concurrently so that's fast, the multiple policy is the one used by default and in tests
-
-
-
So the issue was, we didn't have the = when comparing the number of maintained connections with min_connected. But since we never maintain more then min_connected, this means the check without the = will always be false thus skiping the sleep code in the background task. The background task was basically looping non-stop and degrading the performance of all other async tasks running on the runtime. Very slow tests led to slow connections and timeouting everytime.
Commits on Sep 4, 2024
-
This refactors expirable map to make it so that deleting expired entries is cheap (not looping over the whole map, but looping only over the known expired entries). This increases the memory requirements though because the key of type `K` is now cloned an extra time to be stored in the inverse map. A solution for the potentially expensive cloning is to wrap the key in an Arc, but, looking at the usage of expirable map, it looks like most (actually all) of the times, the keys are integer IDs, which are cheaply cloned/copied. So leaving this as a direct clone for now until the need arises.
-
let the responses map in eletrum connections be expriable
otherwise responses which timeout are never deleted :O Also we now send electrum_request_multi in parallel, the first response is considered and the others are discarded. This can make up end in a case where one connection had the response notifier registered in the responses map but the request was never sent, thus a response will never be received, thus this entry will never be removed.
-
make wait_till_connected test-only
not needed in non-test environments since these wait forever till the request is resolved
-
split the connection manager into multiple impl blocks
for organization
-
let connections map inside the manager be dynaimc
so we can add and remove connections in runtime
-
-
nothing was changed. seems like the header loop is ran forever but only the notifier is short lived
Commits on Sep 5, 2024
-
simplfying establish_connection_loop method as much as possible
splitting the big function into smaller ones and sharing the sharable parts between native and wasm
Commits on Sep 12, 2024
-
don't retry forever, and also try all connections when the active one…
…s fail this is kind of doing the connection managers work as a last resort fail safe mechanisim when electrum request fails, that is, we establish all connections sequentially and try them. things we need to consider: - we prolly need to introduce wait_till_connected back (now that we don't wait forever) - we don't disconnect any of the established connections, they will disconnect automatically when not used for a long time, but for this long time, we might break the `max_connected` rule.
-
properly respect the max_connected threshold
let the background thread of the connection manager respect the `max_connected` threshold (not entirely really, we can reach up to `max_connected + 1`). This is done by refactoring the connection establishment to not establish all connections but only connections that we know we will maintain for sure. This also is lighter overall than the old approach. but it's also slower since connections are tried sequentially, we don't need the speed here anyways since if an electrum request fails we fail back to try all connections which is a logic that isn't controlled by the connection manager in any way.
-
-
suggestion from omar - 2 - (un)maintain methods
removed write_maintained_connections and added maintain & unmaintain methods.
-
testing if this would fix tests
disconnecting like that broke a lot of requests, we probably shouldn't disconnect from some thread since we don't know if another thread is holding the connection to use it
Commits on Sep 24, 2024
-
Revert "testing if this would fix tests"
This reverts commit d98fd21. This should make a lot of tests fail again, proving we shouldn't randomly just disconnect a connection from different threads.
-
bug: don't disconnect the connection we want to connect while version…
… checking when doing version checking, the connection isn't marked as maintained yet, this means flagging the not needed will disconnect it immediately. a fix for this is to keep a state of `Connecting` for example to avoid disconnecting a connection which is in progress, eaiser solution is to not ask for disconnection for connections that we didn't force connect. so only if we caused the connection to connect will we call for it to be not needed anymore
-
for the fall back case. But delegate whether to disconnect or not to the connection manager. this helps with concurrent requests disconnecting for no reason (so no error) and breaking other requests
-
abandon the wait_till_connected test-only method
this was used to make sure the background task have found a working connection by the time we start querying electrum requests. we don't need this now though. because if there are no working/maintained connections we fall back to using any/all other connections (and we establish the connection inline first)
-
fix test_electrum_rpc_client_error
the test was failing because the expected request id was 0 and not 1. this is becasue the server version request was constructed AFTER the get verbose transaction request. this might not be always the case, so the background thread might in some (rare) instances be fast enough and establish the connection and query for version before get verbose transaction is called, so the id 0 is reserved then for the server version query. lets just remove the id part in the expected string as we don't really care about it
-
-
yield in both test and non-test environment
to avoid a potential starvation due to a potential busy waiting loop
-
-
-
add a todo regarding abortablequeue abort vs finish
This currently isn't really problamatic, but if re-used later could have a hard to debug nightmares the solution here would be to totally order both operations favoring abortion over finishing. In `AbortableQueue` we would need to realize this by checking the future ID in question and not call `on_future_finished` if it's already aborted
Commits on Sep 25, 2024
-
patch on_future_finished to not mark id as finished if it was aborted
this was the side effect we had (described in the previous commit) in abortable queue. on_future_finished was called AFTER the abortable queue has been fully cleared and reset. We need to make sure first that an abort handle the future actually exists in the abort handlers vector and that the future is aborted (i.e. the same future ID wasn't used to run a new future that is still running).
Commits on Sep 30, 2024
Commits on Oct 3, 2024
-
fix test_one_unavailable_electrum_proto_version
version mismatch was labeled as temporary error by mistake. note that when electrum replys successfully for the version query this means there is no version mismatch and we don't need to check for that
Commits on Oct 4, 2024
-
-
simplyfy timeouting - handle timeout in establish_connection_loop only
don't let child helper funcs know about the timeout
-
simplify erroring: handle disconnection in establish_connection_loop …
…only let the other helper methods just return the error but not disconnect the connection
-
actually validate min & max connected
instead of just constructing a string and dumping it into the sea :)
-
-
reduce the btree key of json bach request to its hash
which makes it susbtible to hash conflicts so this might not be a good idea
-
use a deterministically-selected rpc_id from the batch
and use it as a batch_id
Commits on Oct 7, 2024
-
-
-
label version mismatch only when the electrum server replys
to us with a json rpc error. we could have encountered other errors like timeouts and transport errors in the version queries, such errors shouldn't be version mismatch as we never knew the server's reply
-
-
disable the coin if the balance query fails
otherwise the GUI will get an error and think the coin wasn't enabled successfuly but it was. so disabling the coin to keep consistent with the GUIs expectionations. one solution would be to omit the balance field if balance querying fialed, but we would need the GUI to account for that then
Commits on Oct 9, 2024
-
run rpc futures in a tokio thread till completion
when ran directly inside hyper's service function, it might get aborted mid-way when a client disconnects, leaving the future in complete. this is an issue as we might have some code that need to be executed atomically inside any part of the code base that could be triggered by an RPC request. if the client disconnects, such a function will abort on the next await call, leaving us with non-atomic state (e.g. partial update for a map and its inverse).
-
cap the electrum request to 5s
the balance query by default could take up to 40s: 20s to establish a connnection with an electrum server & 20s to perform the electrum request. the old electrum rpc would timeout in 5s if the electrum servers aren't responsive, and it seems like the desktop GUI accounted only for such timeout. let's use 5s timeout internally for the electrum request to make sure we are compatible with GUIs. note that this only caps the timeout for the enable request to 5s, but in normal operations the other configured timeouts are used
Commits on Oct 10, 2024
-
mitigate the noisy logs from the endless retries
don't log (trivial) errors inside establish connection loop, so no connection establishment error no version errors no timeout errors. log these errors instead from the manager, but if the manager is in panic mode we don't want to log no further. also don't pre-log the 'connected securely/not-securely' message, only print it when the connection is final and ready to be used.
-
Revert "to revert: try to deser the electrum response as json and log…
… it" This reverts commit decacb8.
Commits on Oct 16, 2024
-
convert async mutexs to sync ones
this carried over from the usage of these fields in past impl. there is no reason to have these mutexes async. sync mutexs are better for performance. one mutex is left async though (establishing_connection) to not cpu-block other threads when they are waiting on the same connection to be established.
Commits on Oct 18, 2024
-
review(sami): let max & min connected be usize
and avoid lots of conversions