Page MenuHomePhabricator

[net processing] Annotate m_recently_announced_{invs|proofs} as guarded by g_msgproc_mutex and move to Peer
ClosedPublic

Authored by PiRK on Jan 26 2024, 10:22.

Diff Detail

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

Event Timeline

PiRK requested review of this revision.Jan 26 2024, 10:22
Fabien requested changes to this revision.Jan 26 2024, 15:32
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/net_processing.cpp
514 ↗(On Diff #44613)

We could probably use the m_recently_announced_invs for proofs rather than duplicating

3235 ↗(On Diff #44613)

Do you still need cs_main ?

4662 ↗(On Diff #44613)

Do you still need cs_main ?

7871 ↗(On Diff #44613)

Are you holding NetEventsInterface::g_msgproc_mutex here ?

7980 ↗(On Diff #44613)

Same question

This revision now requires changes to proceed.Jan 26 2024, 15:32

remove unneeded cs_main lock

src/net_processing.cpp
514 ↗(On Diff #44613)

There are some differences, I think the TxRelay is not necessarily initialized if a node does not relay transactions whereas the ProofRelay is not necessarily initialized if avalanche is not enabled.
Those are just memory optimizations, maybe we could just always initialize those structures are little cost. I will try to guesstimate the footprint.

7871 ↗(On Diff #44613)

Yes, see line 7510.

bool PeerManagerImpl::SendMessages(const Config &config, CNode *pto) {
    AssertLockHeld(g_msgproc_mutex);
   ...
7980 ↗(On Diff #44613)

yes, we are still in PeerManagerImpl::SendMessages

This revision is now accepted and ready to land.Jan 26 2024, 17:16