Page MenuHomePhabricator

[avalanche] Request compact proofs from our avalanche outbound peers

Authored by Fabien on Jun 1 2022, 09:14.



This sends a getavaproofs message upon connection of an avalanche outbound peer or manually added peer with avalanche enabled.

Depends on D11533.

Test Plan
ninja all check-all

Diff Detail

rABC Bitcoin ABC
Lint Passed
No Test Coverage
Build Status
Buildable 19214
Build 38176: Build Difflint-circular-dependencies · build-without-wallet · build-debug · build-diff · build-clang · build-clang-tidy
Build 38175: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Jun 1 2022, 09:14
deadalnix requested changes to this revision.Jun 1 2022, 13:15
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
1676 ↗(On Diff #33762)

I don't see anything in the condition that would lead me to think this peer is trusted.

This revision now requires changes to proceed.Jun 1 2022, 13:15

Rename the isAvalancheTrustedPeer() to isAvalancheOutboundOrManual()


I don't understand how this is thread safe.

deadalnix requested changes to this revision.Jun 1 2022, 20:51

I'm not app at all with ho this is going.

I told you what the question was. This is half the work. Either this was thread safe and making things atomic is useless, or it wasn't and then, what about the other fields hat aren't guarded b a mutex? Why didn't thread sanitizer detect it?

Do we really need to wait month or untill something break before making sure that the design make sense?

This revision now requires changes to proceed.Jun 1 2022, 20:51
Fabien requested review of this revision.Jun 2 2022, 09:23

I answered the question in D11545 but updated this diff, this is confusing.

There is no race on this because it's always accessed from the same thread. Core tends to put lock requirements on these variables even when not required btw, because they are likely to be used from other threads at some point (see relevant discussion).
For consistency I did the same here, but used an atomic rather than a lock which is enough for the purpose of this variable. The surrounding variable is thead safe but it's not obvious, so I clarified in D11556.

This revision is now accepted and ready to land.Jun 2 2022, 11:57