feat(utxo): prioritize electrum connections

[rozhkovdmitrii]

This PR refactors the electrum part of the RPC clients. And also introduces a new feature related to connection management decisions.

Within the electrum enable request, one could now specify two parameters (min_connected & max_connected).

• min_connected: KDF needs to keep at least min_connected working active (maintained) connections at all times.
• max_connected: KDF needs to keep at most max_connected working active (maintained) connections at any time.
• To achieve the selective/single-server effect set these parameters to (1, 1)
• To achieve the multiple/all-servers effect, set these parameters to (1, len(servers)) (or don't set at all, as this is used as the default)
  • The connection manager will initially try to connect to a maximum of len(servers) connections and keep them maintained. As some connections get lost for whatever reason, they will not be retried right away. Retries only take place:
    • every 10 minutes (should we decrease?)
    • if we fall below min_connected, which is 1 in this case.
  • So one could use (10, 68) for instance to make sure we are connected to at least 10 servers at all times, and if we ever fall below 10 we fire another round of reconnections. After this round we are guaranteed to have <= 68 connections.
    • e.g. we have 10 connections now, we lose 1 and have 9 now. A round of reconnections start and we end up with some number of active connections between 9 (assuming only our 9 connected servers were the non-faulty ones) and 68. If after the reconnections round we still fall below 10=min_connected connections, we keep retrying.
• These parameters must satisfy: 0 < min_connected <= max_connected <= len(servers)

The connection manager maintains these maintained (working active) connections and these are the ones used for any electrum request (they are all used concurrently and whichever returns the success result first is considered the response for this request).

For requests directed for specific servers (like server_version, get_block_count_from, etc...), the connections to these servers are established first if they are not in the maintained servers set and then the request is performed. So even if the server isn't maintained/active, it could still be queried.

The servers order in the electrum enable request encode an implicit priority (previously in this PR, was encoded as primary & secondary). Servers that appear first in the list are assumed to be more robust and thus have higher priority compared to servers after.

Since we do an altruistic round of retries every 10 minutes, if we ever had the case were a high priority server was down and thus wasn't maintained by us and now is back online, we will prefer maintaining it over another already-maintained low priority server (this is assuming the maintained servers set is full, i.e. len(maintained server) == min_connected). Simply put, high priority servers will kick out low priority ones when they come back online if we have no room to fit both.

Addresses #791

[onur-ozkan]

Thank you for your effort!

The PR seems to be in its early stages right now. This is not an actual review, I just did a quick review to point out some areas that should not be forgotten.

[shamardy]

Since this is still in progress, I left some comments that can help you improve the code.
I will do a more thorough review when this is ready for review!

Please also merge with latest dev and start adding doc comments.

[shamardy]

This is a very bad design choice, the struct should never reference it self to not create any unstable behavior.

[rozhkovdmitrii]

I'm sorry, what unstable behavior can there be if we use Weak reference? The only purpose of this weak_self is to be spawned as an argument of self hosted futures, e.g. server_ping

In the previous implementation, the corresponding code was where it shouldn't be [1, 2]

[borngraced]

why not just introduce a new struct ? e.g

struct ElectrumClientImplWeak(Mutex<Option<Weak<ElectrumClientImpl>>>)

[shamardy]

Since this is a new implementation, please start using specific error types and MmError

[rozhkovdmitrii]

partially done

[shamardy]

Can you please elaborate on this more? ConnectingStateCtx will be considered dropped before connecting is set to false if the lock was held by other operation for sometime.

[rozhkovdmitrii]

Certainly should have been more detailed! Thanks for pointing this out! This place is the heart of selective connectivity.

done

[shamardy]

Suggested change

	let address: String = {
	if address.is_some() {
	address.map(|internal| internal.to_string())
	} else {
	guard.active.as_ref().cloned()
	}
	}?;
	let address = address.map_or_else(|| guard.active.as_ref().cloned(), |internal| Some(internal.to_string()))?;

[shamardy]

Next iteration! I wanted to finish the review completely this time, but decided to give it another last look after the below comments are addressed!

Please fix failing CI steps. One thing to add to the tracking issue checklist (ref. #1966 (comment)) is an RPC to get the list of currently connected / used electrums, so we remember to add this in the future.

[shamardy]

Thanks for the fixes, reviewed the latest changes only, still one more review to go!

This connection manager can become a great crate in the future to be used by bitcoin wallet devs or any other UTXO wallet dev, I don't think they will find a better electrum client than this. But in time they will just use KDF straight away :)

A few notes for you @mariocynicys / me to remember:

• I guess we need to merge this chore(tests): don't use .wait() and use block_on instead #2220 first.
• There is a failing test in CI that you should check mm2_tests::mm2_tests_inner::test_cancel_all_orders