Page MenuHomePhabricator

[avalanche] Annouce our proofs to our peers
AbandonedPublic

Authored by Fabien on May 9 2021, 16:56.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Summary

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.

Test Plan
./test/functional/test_runner.py abc_p2p_proof_inventory

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_send_inv_on_connect
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 15668
Build 31240: Build Diffbuild-without-wallet · build-diff · build-debug · lint-circular-dependencies · build-clang-tidy · build-clang
Build 31239: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.May 9 2021, 16:56

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
Fabien planned changes to this revision.May 9 2021, 16:59

Drop the conditions for creating the relay structure (not needed at this point), fix the circular dependencies.

deadalnix requested changes to this revision.May 9 2021, 21:15
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
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?

This revision now requires changes to proceed.May 9 2021, 21:15

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.
The test doesn't really reflect that yet because we need more code before we can get more proofs to share.

deadalnix requested changes to this revision.May 10 2021, 15:33

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

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

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

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

In term of priority, shouldn't this go in between blocks and transactions?

This revision now requires changes to proceed.May 10 2021, 15:33
src/net.h
1175

1/ This isn't thread safe
2/ The fact this skips inventories when the struct is null is a feature, not a bug. This is the whole point of that design.

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 .

Mengerian added a task: Restricted Maniphest Task.May 21 2021, 15:52
Fabien planned changes to this revision.Jun 4 2021, 19:59