Page MenuHomePhabricator

[avalanche] Request compact proofs from our avalanche outbound peers
ClosedPublic

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

Details

Summary

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

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.
src/net_processing.cpp
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()

src/net_processing.cpp
3699 ↗(On Diff #33765)

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