p2p: Headers-sync followups #25960

sdaftuar commented at 4:48 PM on August 30, 2022: member

Remove BCLog::HEADERSSYNC and move all headerssync logging to BCLog::NET.

Bypass headers anti-DoS checks for NoBan peers

Also fix a typo that was introduced in PR25717.

Move headerssync logging to BCLog::NET 132ed7eaaa

sdaftuar commented at 4:49 PM on August 30, 2022: member

@ajtowns @instagibbs These should address the remaining review items that came up in #25717 (comment)

fanquake added the label P2P on Aug 30, 2022

in test/functional/p2p_headers_sync_with_minchainwork.py:58 in 1206e2deb2 outdated

  53 |              self.generate(self.nodes[0], NODE1_BLOCKS_REQUIRED-1, sync_fun=self.no_op)
  54 |  
  55 | -        for node in self.nodes[1:]:
  56 | +        # Node3 should always allow headers due to noban permissions
  57 | +        self.log.info("Check that node3 will sync headers (due to noban permissions)")
  58 | +        def check_node3_chaintips(num_tips, tip_hash, height):

fanquake commented at 5:05 PM on August 30, 2022:

/tmp/cirrus-ci-build/test/functional/p2p_headers_sync_with_minchainwork.py:57: unused variable 'num_tips' (100% confidence)
test/functional/p2p_headers_sync_with_minchainwork.py:57:9: E306 expected 1 blank line before a nested definition, found 0

sdaftuar commented at 6:30 PM on August 30, 2022:

Thanks, fixed.

jonatack commented at 5:15 PM on August 30, 2022: contributor

<details><summary>Some initial WIP follow-up suggestions on first look-over of [#25717](/bitcoin-bitcoin/25717/).</summary><p>

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c4a1b5fa15..1158b6c06a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -634,23 +634,23 @@ private:
      *              acceptance by the caller).
      */
     bool IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfrom,
-            std::vector<CBlockHeader>& headers)
+                                            std::vector<CBlockHeader>& headers)
         EXCLUSIVE_LOCKS_REQUIRED(peer.m_headers_sync_mutex, !m_headers_presync_mutex);
     /** Check work on a headers chain to be processed, and if insufficient,
      * initiate our anti-DoS headers sync mechanism.
      *
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   peer                The peer whose headers we're processing.
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   pfrom               CNode of the peer
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   chain_start_header  Where these headers connect in our index.
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in,out]   headers             The headers to be processed.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in]     peer                The peer whose headers we're processing.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in]     pfrom               CNode of the peer
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in]     chain_start_header  Where these headers connect in our index.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in,out] headers             The headers to be processed.
      *
      * [@return](/bitcoin-bitcoin/contributor/return/)      True if chain was low work and a headers sync was
      *              initiated (and headers will be empty after calling); false
      *              otherwise.
      */
     bool TryLowWorkHeadersSync(Peer& peer, CNode& pfrom,
-                                  const CBlockIndex* chain_start_header,
-                                  std::vector<CBlockHeader>& headers)
+                               const CBlockIndex* chain_start_header,
+                               std::vector<CBlockHeader>& headers)
         EXCLUSIVE_LOCKS_REQUIRED(!peer.m_headers_sync_mutex, !m_peer_mutex, !m_headers_presync_mutex);
 
     /** Return true if the given header is an ancestor of
@@ -1482,7 +1482,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
 
 void PeerManagerImpl::FinalizeNode(const CNode& node)
 {
+    AssertLockNotHeld(m_headers_presync_mutex);
     NodeId nodeid = node.GetId();
     int misbehavior{0};
     {
@@ -2481,6 +2481,9 @@ bool PeerManagerImpl::CheckHeadersAreContinuous(const std::vector<CBlockHeader>&
 
 bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfrom, std::vector<CBlockHeader>& headers)
 {
+    AssertLockHeld(peer.m_headers_sync_mutex);
+    AssertLockNotHeld(m_headers_presync_mutex);
     if (peer.m_headers_sync) {
         auto result = peer.m_headers_sync->ProcessNextHeaders(headers, headers.size() == MAX_HEADERS_RESULTS);
         if (result.request_more) {
@@ -2494,13 +2497,12 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro
                 // it may be possible to bypass this via compactblock
                 // processing, so check the result before logging just to be
                 // safe.
-                bool sent_getheaders = MaybeSendGetHeaders(pfrom, locator, peer);
-                if (sent_getheaders) {
+                if (MaybeSendGetHeaders(pfrom, locator, peer)) {
                     LogPrint(BCLog::NET, "more getheaders (from %s) to peer=%d\n",
-                            locator.vHave.front().ToString(), pfrom.GetId());
+                             locator.vHave.front().ToString(), pfrom.GetId());
                 } else {
                     LogPrint(BCLog::NET, "error sending next getheaders (from %s) to continue sync with peer=%d\n",
-                            locator.vHave.front().ToString(), pfrom.GetId());
+                             locator.vHave.front().ToString(), pfrom.GetId());
                 }
             }
         }
@@ -2567,6 +2569,9 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro
 
 bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlockIndex* chain_start_header, std::vector<CBlockHeader>& headers)
 {
+    AssertLockNotHeld(peer.m_headers_sync_mutex);
+    AssertLockNotHeld(m_peer_mutex);
+    AssertLockNotHeld(m_headers_presync_mutex);
     // Calculate the total work on this chain.
     arith_uint256 total_work = chain_start_header->nChainWork + CalculateHeadersWork(headers);
 
@@ -2609,6 +2614,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
 
 bool PeerManagerImpl::IsAncestorOfBestHeaderOrTip(const CBlockIndex* header)
 {
+    AssertLockHeld(cs_main);
     if (header == nullptr) {
         return false;
     } else if (m_chainman.m_best_header != nullptr && header == m_chainman.m_best_header->GetAncestor(header->nHeight)) {
@@ -2767,7 +2773,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
                                             std::vector<CBlockHeader>&& headers,
                                             bool via_compact_block)
 {
+    AssertLockNotHeld(m_headers_presync_mutex);
     size_t nCount = headers.size();
 
     if (nCount == 0) {
@@ -3166,9 +3172,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                                      const std::chrono::microseconds time_received,
                                      const std::atomic<bool>& interruptMsgProc)
 {
+    AssertLockNotHeld(m_headers_presync_mutex);
 
     LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(msg_type), vRecv.size(), pfrom.GetId());

</p></details>

Feel free to ignore. I also have a branch ready at https://github.com/bitcoin/bitcoin/compare/master...jonatack:bitcoin:net-add-missing-thread-safety-runtime-assertions for all the net and net_processing code and mention above only the runtime lock assertions directly related to changes in #25717.

Edit: updated with additional missing lock assertions.

sdaftuar force-pushed on Aug 30, 2022

Bypass headers anti-DoS checks for NoBan peers e5982ecdc4

Fix typo from PR25717 94af3e43e2

sdaftuar force-pushed on Aug 30, 2022

in src/net_processing.cpp:2827 in e5982ecdc4 outdated

2819 | @@ -2820,6 +2820,13 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
2820 |          }
2821 |      }
2822 |  
2823 | +    // If our peer has NetPermissionFlags::NoBan privileges, then bypass our
2824 | +    // anti-DoS logic (this saves bandwidth when we connect to a trusted peer
2825 | +    // on startup).
2826 | +    if (pfrom.HasPermission(NetPermissionFlags::NoBan)) {
2827 | +        already_validated_work = true;

naumenkogs commented at 7:42 AM on August 31, 2022:

nit: after this changetrustworthy_work or something could be more accurate, although I'm not sure it actually benefits the readers.

Sjors commented at 9:32 AM on August 31, 2022: member

tACK 94af3e43e20aa00b18e7a3f6d0f5fe3ad9494d97

I checked that NoBan peers indeed sync in a single pass now.

The download permission might be more suitable for this (and it's implied by noban). In any case it's not a very useful optimisation, but I'm not opposed to it.

glozow added the label Utils/log/libs on Aug 31, 2022

ajtowns commented at 2:41 PM on August 31, 2022: contributor

The download permission might be more suitable for this

For what it's worth, I interpret download as being more in the other direction -- "this peer can download from me, even if I'm in IBD" rather than "I feel safe downloading from this peer", so it seemed less suitable to me. But :shrug:

in src/logging.h:68 in 132ed7eaaa outdated

  64 | @@ -65,7 +65,6 @@ namespace BCLog {
  65 |  #endif
  66 |          UTIL        = (1 << 25),
  67 |          BLOCKSTORE  = (1 << 26),
  68 | -        HEADERSSYNC = (1 << 27),

ajtowns commented at 2:53 PM on August 31, 2022:

You could make this a scripted diff:

-BEGIN VERIFY SCRIPT-
sed -i '/headerssync/Id' src/logging.h src/logging.cpp
sed -i 's/BCLog::HEADERSSYNC/BCLog::NET/g' $(git grep -l BCLog::HEADERSSYNC)
-END VERIFY SCRIPT-

Sjors commented at 3:46 PM on August 31, 2022: member

We should probably document what we mean by these permissions :-)

ajtowns commented at 3:55 PM on August 31, 2022: contributor

We should probably document what we mean by these permissions :-)

They're documented in bitcoin-cli help getpeerinfo:

    "permissions" : [                     (json array) Any special permissions that have been granted to this peer
      "str",                              (string) bloomfilter (allow requesting BIP37 filtered blocks and transactions),
                                          noban (do not ban for misbehavior; implies download),
                                          forcerelay (relay transactions that are already in the mempool; implies relay),
                                          relay (relay even in -blocksonly mode, and unlimited transaction announcements),
                                          mempool (allow requesting BIP35 mempool contents),
                                          download (allow getheaders during IBD, no disconnect after maxuploadtarget limit),
                                          addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info).
                                          
      ...
    ],

and in bitcoind -help under -whitebind.

Sjors commented at 4:00 PM on August 31, 2022: member

Ok, in that case I tend to agree noban is better. I guess download should have been called upload, but that depends on the perspective :-)

ajtowns commented at 6:35 PM on August 31, 2022: contributor

ACK 94af3e43e20aa00b18e7a3f6d0f5fe3ad9494d97

sipa commented at 7:00 PM on August 31, 2022: member

ACK 94af3e43e20aa00b18e7a3f6d0f5fe3ad9494d97

w0xlt approved

w0xlt commented at 7:22 PM on August 31, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25960/commits/94af3e43e20aa00b18e7a3f6d0f5fe3ad9494d97

fanquake added this to the milestone 24.0 on Aug 31, 2022

DrahtBot commented at 7:24 PM on August 31, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23443 (p2p: Erlay support signaling by naumenkogs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake requested review from instagibbs on Aug 31, 2022

naumenkogs commented at 6:02 AM on September 1, 2022: member

ACK 94af3e43e20aa00b18e7a3f6d0f5fe3ad9494d97

fanquake merged this on Sep 1, 2022

fanquake closed this on Sep 1, 2022

sidhujag referenced this in commit edb00a6bc8 on Sep 1, 2022

bitcoin locked this on Sep 1, 2023