p2p: Fill reconciliation sets (Erlay) attempt 2 #30116
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30116. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
I've talked to @naumenkogs about picking this up and he was happy about it. I'm happy to close this if he changes his mind. |
047f61c to
bc8e7d7
Compare
d4b9fc6 to
935fc82
Compare
|
I've slightly extended the approach adding 3 commits to deal with short id collisions, which were not taken into account. Some of this may be squashable, I've added them separately for now so they are easy to diff/review. This would be missing an additional commit/amend to deal with #28765 (comment), which I overlooked when addressing the outstanding comments |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
e3b5988 to
7c94dae
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
1a58a43 to
e7d426a
Compare
|
I've rebased to account for the recent I've also decided to clean the design while I was at it, to make things easier to follow and test. As part of this, the file structure has been changed from two files ( The code is split as follows:
|
7e4c66f to
ac0a2a0
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Splits the txreconciliation logic in three files instead of two, allowing the TxreconciliationState to be properly tested, instead of being internal to txreconciliation.cpp. Also defines includes everything in the node namespace, instead of being part of an anonymous one.
These comments became irrelevant in one of the previous code changes. They simply don't make sense anymore.
They will be used later on. Co-authored-by: Gleb Naumenko <naumenko.gs@gmail.com>
It helps to avoid recomputing every time we consider a transaction for fanout/reconciliation. Co-authored-by: Gleb Naumenko <naumenko.gs@gmail.com>
Fanout or reconciliation is decided on a transaction basis, based on the following criteria: If the peer is inbound, we fanout to a pre-defined subset of peers (which is rotated periodically). If the peer is outbound, we will reconcile the transaction if we received it via reconciliation, or defer the decision to relay time otherwise. At relay time, we will fanout to outbounds until a threshold is met (selecting peers in the order their timers go off) and reconcile with the rest. With this approach we try to fanout when we estimate to be early in the propagation of the transaction, and reconcile otherwise. Notice these heuristics don't apply to inbound peers, since they would be easily exploitable. For inbounds we just aim for a target subset picked at random.
Transactions eligible for reconciliation are added to the reconciliation sets. For the remaining txs, low-fanout is used. Co-authored-by: Gleb Naumenko <naumenko.gs@gmail.com>
… short ids to wtxids
If a transaction to be added to a peer's recon set has a shot id collisions (a previously added wtxid maps to the same short id), both transaction should be fanout, given our peer may have added the opposite transaction to our recon set, and these two transaction won't be reconciled.
ac0a2a0 to
e5f1244
Compare
This is a re-attempt of #28765
The new approach differs from the previous in how peers are selected for fanout:
For Erlay-enabled peers, fanout targets are chosen for each transaction individually. Once the fanout target is reached, the transaction is reconciled with the rest of our peers. Whether a peer is selected for fanout depends on factors like peer type (inbound or outbound), how the transaction was received (via fanout or reconciliation), and how many of our peers are already aware of it.
For outbound peers:
If the transaction was received through fanout (or we originated it), we set a fanout rate threshold
OUTBOUND_FANOUT_THRESHOLD{4}. As long as the transaction has been announced to (or received from) fewer than this number of peers, we continue faning out based on the order in which peers' Poisson timers go off.If the transaction was received via reconciliation, we simply reconcile it with the rest of our peers.
For inbound peers:
We select a subset of inbound peers as fanout targets and send all transactions to them within a set time interval. Once the interval ends, the selection is randomly rotated.
This approach helps scale the transaction fanout rate for outbound peers by approximating how widely a transaction has already been propagated. For inbound peers, we use a random selection strategy to prevent them from easily exploiting our heuristic.
You may notice 398078a is not currently being used. It is likely to be dropped and brought back in a follow-up. Currently, we are picking Erlay fanout targets independently of whether we already have other non-erlay peers (which are always fanout). We may want to scale the selection based on non-Erlay connections, but I think that can be addressed on it's own PR.
Erlay Project Tracking: #30249