This is the first step toward proof relaying. When we meet an avalanche
enabled peer, announce our proofs with an inv message. This is heavily
based on the current implementation for transactions.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- Restricted Maniphest Task
./test/functional/test_runner.py abc_p2p_proof_inventory
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- avalanche_send_inv_on_connect
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 15665 Build 31234: Build Diff build-debug · build-diff · lint-circular-dependencies · build-without-wallet · build-clang · build-clang-tidy Build 31233: arc lint + arc unit
Event Timeline
Comment Actions
Tail of the build log:
/work /work/abc-ci-builds/lint-circular-dependencies A new circular dependency in the form of "avalanche/node -> net -> avalanche/processor -> avalanche/node" appears to have been introduced. A new circular dependency in the form of "avalanche/peermanager -> net -> avalanche/processor -> avalanche/peermanager" appears to have been introduced. A new circular dependency in the form of "avalanche/processor -> net_processing -> net -> avalanche/processor" appears to have been introduced. /work/abc-ci-builds/lint-circular-dependencies Build lint-circular-dependencies failed with exit code 1
Comment Actions
Drop the conditions for creating the relay structure (not needed at this point), fix the circular dependencies.
| src/net.cpp | ||
|---|---|---|
| 3010 ↗ | (On Diff #28397) | That doesn't look like the right place to do this. |
| src/net.h | ||
| 1176 ↗ | (On Diff #28397) | Why does this one doesn't need a mutex while the others do? |
| src/net_processing.cpp | ||
| 2984 ↗ | (On Diff #28397) | That does not make sense. If they don't have it, they have the id necessary to request it. No need to aggressively push for it. |
| 5386 ↗ | (On Diff #28397) | This pattern is duplicated, doesn't belong in a lambda? |
Comment Actions
Move the relay struct initialization to it's first use, guard the send buffer with mutex, rebase on top of D9490.
| src/net.h | ||
|---|---|---|
| 1176 ↗ | (On Diff #28397) | Good point. I removed it when splitting out the filter because it didn't have the guard annotation, but this is actually a bug in the existing codebase. I'll bring it back with proper annotation and fix the tx set missing guard. |
| src/net_processing.cpp | ||
| 2984 ↗ | (On Diff #28397) | Your peers will know your local proof id that you sent through the delegation, the proofs you will relay when you get some new, but you need a way to share your initial proof pool state. |
Comment Actions
You'll note that there is a set of message to exchange mempool content, there is no reason to not do the same for proofs, because this is introducing quadratic behavior.
| src/net_processing.cpp | ||
|---|---|---|
| 2983 ↗ | (On Diff #28400) | It's literally impossible that this is thread safe. Also, as per your previous comment, this doesn't seem like a reasonable thing to do anyways. This is creating a quadratic behavior in the number of proof (more proof, mean more node supporting proofs for which we need to do an ops that is linear in the number of proofs). This is simply not a good idea. |
| 2983 ↗ | (On Diff #28400) | Upon further digging, this is thread safe because it creates a full copy of the whole peer list, as per D9290 . I'm not sure if this better or worse. This clearly shows the need for an inversion of control. Ditch getPeers, and add a way to iterrate over all peers using a lambda, in CConMan::ForEachNode . |
| 5201 ↗ | (On Diff #28400) | It looks like you should move the vector here instead of copying just to clear it after. (I commented on the other diff about this). |
| 5368 ↗ | (On Diff #28400) | In term of priority, shouldn't this go in between blocks and transactions? |
| src/net.h | ||
|---|---|---|
| 1175 ↗ | (On Diff #28400) | 1/ This isn't thread safe |
Comment Actions
OK I missed that the broadcast set was actually fixing a quadratic issue. So here is the plan:
- Add a set of unbroadcasted proofs to the peer manager
- Make the peer manager remove the proofs from the set as they become invalid
- Add binding at the processor level to add and remove an unbroadcasted proof
- Add manually submitted proofs to the unbroadcasted set
- Remove the proof from the unbroadcasted set on first peer that requested it
- Extend the PeerManager::ReattemptInitialBroadcast() to include the unbroadcasted proofs
Our local proof id will not be added to this unbroadcasted set and only pushed via the hello message.
The problem now is that I need a way to test that behavior, so I need to complete answering the proof invs with getdata requests first .