Connect additional peers on failed request #1
Conversation
There was a problem hiding this comment.
Good idea in general. However, I tried to model the electrum-client as closely as possible to the Nimiq client, which does not have this behavior of connecting to other peers when a request fails. Then again, the Nimiq client has more than one connection from the start.
The electrum-client does not yet connect to multiple agents, because I did not have time and expertise on how to create a (pico) consensus between the agents, which would be required. The client would need to connect to 3 agents and find a common head block in 2-of-3 of them, etc.
So IMO this solution presented here is a good workaround, but the real solution would be to connect to more peers at the start and establish an actual consensus.
Does the issue with a missing transaction come up often for Ledger? Is asking a centralized API of a block explorer maybe a more reliable option?
| private async callAgents<R>(call: AgentCall<R>, onAgentError?: AgentErrorCallback, stopAfterFirstResult?: true): Promise<R>; | ||
| private async callAgents<R>(call: AgentCall<R>, onAgentError: AgentErrorCallback | undefined, stopAfterFirstResult: false): Promise<R[]>; | ||
| private async callAgents<R>(call: AgentCall<R>, onAgentError?: AgentErrorCallback, stopAfterFirstResult: boolean = true): Promise<R | R[]> { |
There was a problem hiding this comment.
I don't like that there is an optional third parameter that can be overlooked. I would prefer two separate functions, one for each use case: callFirstAgent and callAllAgents. That way the behavior is clear from the naming.
| if (stopAfterFirstResult) break; | ||
| } catch (error) { | ||
| if (onAgentError) { | ||
| onAgentError(agent, error); |
There was a problem hiding this comment.
I think the convention is that the error is the first parameter to an error callback. agent should be the second, in case the error handler does not use it.
|
|
||
| if (!newAgent) break; | ||
|
|
||
| await this.waitForConsensusEstablished(); |
There was a problem hiding this comment.
This has no effect on the newly connected agent as long as the first agent is still in synced status.
This also plays into the whole consensus-between-multiple-agents topic that I described in the main review comment.
Connect to additional peers if our currently connected peers can't answer a request.
Initially I implemented a method to manually connect to additional peers for use in the
SignBtcTransactionLedgerflow, if the currently connected peer(s) doesn't know a transaction that I need.This is necessary for pending transactions with low transaction fee which do not seem to be propagated to all peers.
However, I think it is generally a good handling of failed requests, so I implemented it for all requests.
This now also means that the client might be connected to multiple peers which hasn't been the case so far.
But that shouldn't be a problem I think? I was actually a bit surprised, that the client only connected to one peer.