p2p: ProcessHeadersMessage(): fix received_new_header #26172

pull LarryRuane wants to merge 1 commits into bitcoin:master from LarryRuane:2022-09-fix-received_new_header changing 1 files +1 −1

LarryRuane commented at 6:26 AM on September 24, 2022: contributor

Follow-up to #25717. The commit "Utilize anti-DoS headers download strategy" changed how this bool variable is computed, so that its value is now the opposite of what it should be.

Prior to #25717:

bool received_new_header{WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash()) == nullptr)};

After #25717 (simplified):

{
    LOCK(cs_main);
    last_received_header = m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash());
}
bool received_new_header{last_received_header != nullptr};

p2p: ProcessHeadersMessage(): fix received_new_header

Follow-up to #25717. The commit "Utilize anti-DoS headers download
strategy" changed how this bool variable is computed, so that its value
is now the opposite of what it should be.

bdcafb9133

fanquake added the label P2P on Sep 24, 2022
fanquake requested review from sdaftuar on Sep 24, 2022
maflcko added this to the milestone 24.0 on Sep 24, 2022
dergoegge commented at 10:33 AM on September 26, 2022: member

ACK bdcafb913398f0cdaff9c880618f9ebfc85c7693

Great catch @LarryRuane !
stickies-v approved
stickies-v commented at 5:53 PM on September 26, 2022: contributor

ACK bdcafb913398f0cdaff9c880618f9ebfc85c7693

This changes behaviour back to how it was previously and how it is described in its docstring, which appears accurate to me.

Without this fix, the expected update behaviour of CNodeState::m_last_block_announcement is broken. It should only get updated to the current time for the one peer that provided the latest header, but currently this is mostly (there are additional conditions) flipped around where instead all the other peers are updated to the current time. I observed this empirically on signet by adding additional log statements when m_last_block_announcement was (falsely) updated, as well as when it wasn't but should have because of the flipped received_new_header value. I also observed empirically this behaviour is gone after bdcafb913398f0cdaff9c880618f9ebfc85c7693 .

The current behaviour is problematic because wrong timestamps in CNodeState::m_last_block_announcement can lead to the wrong peer being selected for eviction.

Should we add a relevant test case in p2p_eviction.py (or elsewhere), perhaps?
glozow commented at 10:01 AM on September 27, 2022: member

ACK bdcafb913398f0cdaff9c880618f9ebfc85c7693, I believe this is correct and don't see anything to suggest the switch was intentional.
glozow merged this on Sep 27, 2022
glozow closed this on Sep 27, 2022
glozow added the label Needs backport (24.x) on Sep 27, 2022
sidhujag referenced this in commit 46070827fc on Sep 27, 2022
sdaftuar commented at 3:30 PM on September 27, 2022: member

utACK, thanks for catching this.

Should we add a relevant test case in p2p_eviction.py (or elsewhere), perhaps?

Yes I think this is a good idea!
mzumsande commented at 4:06 PM on September 27, 2022: contributor

post-merge ACK, good catch!

Should we add a relevant test case in p2p_eviction.py (or elsewhere), perhaps?

That would be helpful - maybe split into p2p_inbound_eviction.py and p2p_inbound_eviction.py.

The outbound eviction logic is currently tested in denialofservice_tests unit tests, which mocks the change to m_last_block_announcement by calling the test-only UpdateLastBlockAnnounceTime, so it couldn't catch this. But I think a functional test might be the better approach here anyway.
fanquake referenced this in commit e4f7fd3461 on Sep 29, 2022
fanquake removed the label Needs backport (24.x) on Sep 29, 2022
fanquake commented at 9:31 AM on September 29, 2022: member

Added to #26133 for backporting for now.
LarryRuane deleted the branch on Sep 30, 2022
fanquake referenced this in commit 7e0bcfbfef on Oct 11, 2022
achow101 referenced this in commit 885366c67a on Oct 13, 2022
bitcoin locked this on Oct 13, 2023