p2p: ProcessHeadersMessage(): fix received_new_header #26172
Conversation
Follow-up to bitcoin#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.
|
ACK bdcafb9 Great catch @LarryRuane ! |
There was a problem hiding this comment.
ACK bdcafb9
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 bdcafb9 .
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?
|
ACK bdcafb9, I believe this is correct and don't see anything to suggest the switch was intentional. |
…eader bdcafb9 p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane) Pull request description: Follow-up to bitcoin#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 bitcoin#25717: ``` bool received_new_header{WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash()) == nullptr)}; ``` After bitcoin#25717 (simplified): ``` { LOCK(cs_main); last_received_header = m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash()); } bool received_new_header{last_received_header != nullptr}; ``` ACKs for top commit: dergoegge: ACK bdcafb9 glozow: ACK bdcafb9, I believe this is correct and don't see anything to suggest the switch was intentional. stickies-v: ACK bdcafb9 Tree-SHA512: 35c12762f1429585a0b1c15053e310e83efb28c3d8cbf4092fad9fe81c893f6d766df1f2b20624882acb9654d0539a0c871f587d7090dc2a198115adf59db3ec
|
utACK, thanks for catching this.
Yes I think this is a good idea! |
There was a problem hiding this comment.
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.
Follow-up to bitcoin#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. GitHub-Pull: bitcoin#26172 Rebased-From: bdcafb9
|
Added to #26133 for backporting for now. |
Follow-up to bitcoin#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. GitHub-Pull: bitcoin#26172 Rebased-From: bdcafb9
e2e4c29 tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow) 4d42c3a psbt: Only include m_tap_tree if it has scripts (Andrew Chow) d810fde psbt: Change m_tap_tree to store just the tuples (Andrew Chow) a9419ef tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow) 4abd2ab psbt: Fix merging of m_tap_tree (Andrew Chow) 1390c96 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin) 9b438f0 refactor: revert m_next_resend to not be std::atomic (stickies-v) 43ced0b wallet: only update m_next_resend when actually resending (stickies-v) fc8f2bf refactor: carve out tx resend timer logic into ShouldResend (stickies-v) a6fb674 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v) 5ad82a0 index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky) 997faf6 contrib: Fix capture_output in getcoins.py (willcl-ark) 7e0bcfb p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane) c97d924 Correct sanity-checking script_size calculation (Pieter Wuille) da6fba6 docs: Add 371 to bips.md (Andrew Chow) Pull request description: Will collect backports for rc2 as they become available. Currently: * #25858 * #26124 * #26149 * #26172 * #26205 * #26212 * #26215 ACKs for top commit: dergoegge: ACK e2e4c29 achow101: ACK e2e4c29 instagibbs: ACK e2e4c29 Tree-SHA512: b6374fe202561057dbe1430d4c40f06f721eb568f91e7275ae1ee7747edf780ce64620382d13ecc4b9571d931dc25d226af8284987cf35ff6a6182c5f64eb10c
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:
After #25717 (simplified):