p2p: Respond to getheaders if we have sufficient chainwork #24178

sdaftuar · 2022-01-27T13:50:49Z

Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes are getting restarted during that time.

To make things simpler to reason about, just use nMinimumChainWork as our anti-DoS threshold for responding to a getheaders request; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed).

sdaftuar · 2022-01-27T13:53:28Z

For additional background on where this logic came from originally, please see: #5927 (comment), #6172, #21106 (comment) (as well as additional discussion in #21106 generally)

jonatack

Concept/Approach ACK

A few suggestions for the test, feel free to pick/choose

diff --git a/test/functional/feature_minchainwork.py b/test/functional/feature_minchainwork.py
index d596464b27..637623706a 100755
--- a/test/functional/feature_minchainwork.py
+++ b/test/functional/feature_minchainwork.py
@@ -17,9 +17,9 @@ only succeeds past a given node once its nMinimumChainWork has been exceeded.
 
 import time
 
+from test_framework.p2p import P2PInterface, msg_getheaders
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
-from test_framework.p2p import P2PInterface, msg_getheaders
 
 # 2 hashes required per regtest block (with no difficulty adjustment)
 REGTEST_WORK_PER_BLOCK = 2
@@ -43,7 +43,7 @@ class MinimumChainWorkTest(BitcoinTestFramework):
             self.connect_nodes(i+1, i)
 
         # Set clock of node2 2 days ahead, to keep it in IBD during this test.
-        self.nodes[2].setmocktime(int(time.time())+48*60*60)
+        self.nodes[2].setmocktime(int(time.time()) + 48 * 60 * 60)
 
     def run_test(self):
         # Start building a chain on node0.  node2 shouldn't be able to sync until node1's
@@ -75,13 +75,12 @@ class MinimumChainWorkTest(BitcoinTestFramework):
         assert self.nodes[1].getbestblockhash() != self.nodes[0].getbestblockhash()
         assert_equal(self.nodes[2].getblockcount(), starting_blockcount)
 
-        # Check that getheaders requests to node2 are ignored.
+        self.log.info("Test that getheaders requests to node2 are ignored")
         peer = self.nodes[2].add_p2p_connection(P2PInterface())
         msg = msg_getheaders()
         msg.locator.vHave = [int(self.nodes[2].getbestblockhash(), 16)]
         msg.hashstop = 0
-        peer.send_message(msg)
-        peer.sync_with_ping()
+        peer.send_and_ping(msg)
         time.sleep(5)
         assert "headers" not in peer.last_message
 
@@ -100,13 +99,13 @@ class MinimumChainWorkTest(BitcoinTestFramework):
         self.log.info(f"Blockcounts: {[n.getblockcount() for n in self.nodes]}")
 
-        # Now getheaders requests should not be ignored
-        peer.send_message(msg)
-        peer.sync_with_ping()
+        self.log.info("Test that getheaders requests are not ignored")
+        peer.send_and_ping(msg)
         assert "headers" in peer.last_message
 
         # Verify that node2 is in fact still in IBD (otherwise this test may
         # not be exercising the logic we want!)
-        assert self.nodes[2].getblockchaininfo()['initialblockdownload']
+        assert_equal(self.nodes[2].getblockchaininfo()['initialblockdownload'], True)
+
 
 if __name__ == '__main__':
     MinimumChainWorkTest().main()

sdaftuar · 2022-01-27T16:37:11Z

Thanks @jonatack -- I've made the test cleanups you suggested.

DrahtBot · 2022-01-27T22:01:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25253 (test: add coverage for non-hex value to -minimumchainwork by brunoerg)
#24008 (assumeutxo: net_processing changes by jamesob)

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.

sipa · 2022-01-27T23:47:15Z

@sdaftuar So when two nodes differ in their checkpoint value, the attack could in theory still apply (being fed a bogus chain during IBD, which would then cause you to be banned by other nodes), but that's just too expensive if it requires the attacker to either branch off long before the current nMinimumChainWork value (effectively requiring the attacker to rewrite history), or shortly before the current nMinimumChainWork (but require recent-ish difficulty blocks on top)?

sdaftuar · 2022-01-28T13:52:22Z

@sdaftuar So when two nodes differ in their checkpoint value, the attack could in theory still apply (being fed a bogus chain during IBD, which would then cause you to be banned by other nodes), but that's just too expensive if it requires the attacker to either branch off long before the current nMinimumChainWork value (effectively requiring the attacker to rewrite history), or shortly before the current nMinimumChainWork (but require recent-ish difficulty blocks on top)?

I'm interpreting "differ in their checkpoint value" as meaning "have chains that disagree about blocks before the checkpoint value", instead of "have software that disagrees about which blockhashes are checkpointed", because in that latter case I think it should be clear that all bets are off for such peers remaining in consensus with each other.

In the former case, which is what this logic is trying to protect against, then yes the point of the nMinimumChainWork check is that it is too expensive to rewrite history that forks off before the last checkpoint which has as much work as our nMinimumChainWork setting. I don't believe there's any meaningful DoS risk to us from an attacker branching the chain shortly before the current nMinimumChainWork, because even if we're fed a less work chain that forks from some recent-ish point, that won't cause us to be partitioned off the network (our peers will just think we're on some less-work but not consensus-invalid branch).

Maybe I should have reframed what this PR does a bit when I opened it. Right now, one of the criteria to leave IBD (so that IsIBD() returns false, and so that we would respond to getheaders requests) is having a chain with more work than nMinimumChainWork, so what this PR changes is that the other IsIBD() requirements no longer need to be met in order to start responding to getheaders requests. The other checks done in IsIBD() are:

    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
        return false;
    if (fImporting || fReindex)
        return true;
    if (m_chain.Tip() == nullptr)
        return true;
    if (m_chain.Tip()->nChainWork < nMinimumChainWork)
        return true;
    if (m_chain.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge))
        return true;

So what this PR is doing is not requiring the tip-age check to pass, and it's also not requiring that we're done reindexing or importing blocks from disk. Relaxing the tip-age check is intentional and I think a better behavior in the event that block creation stalled on the network for some reason. Now that I think about it, maybe I should re-add the guard about fImporting || fReindex so that we don't have peers asking to download blocks from us while we're still busy processing what we have on disk, but not sure how important that is.

Sjors

Concept ACK

Now that I think about it, maybe I should re-add the guard about fImporting || fReindex so that we don't have peers asking to download blocks from us while we're still busy processing what we have on disk, but not sure how important that is.

When either of those is true, Tip() is always going to be at or behind what we've already imported / reindexed. So the nMinimumChainWork check should be enough. There are two ways we decide with what headers to respond:

If locator is null, look up the hashStop block. That won't make it through BlockRequestAllowed if the block is outside the main chain an we haven't revalidated it (which we wouldn't with a reindex).
If locator is present, and has something in the main chain (FindForkInGlobalIndex); already covered by the nMinimumChainWork check

That said, it doesn't hurt to just guard that condition.

P.S. we should probably document the p2p messages in the source code so I don't have to google them, but here's a nice overview of how getheaders is normally used: https://developer.bitcoin.org/devguide/p2p_network.html?highlight=getheaders#headers-first

src/net_processing.cpp

mzumsande

Concept ACK

If I understand this correctly, this means that also under normal circumstances, if we are in IBD (and beyond the point of nMinimumChainWork), we could get inbound connections from peers that are also in IBD, and we would serve them headers and blocks, which would take away some bandwidth and could slow down our own IBD progress.

This doesn't seem like a probable scenario for new nodes that don't self-advertise their address until out of IBD, but a node that is well-known to the network and needs to do IBD again for some reason might want to disable incoming connections, especially on an older bitcoin core version with lots of blocks between nMinimumChainWork and the current network's height.

Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes getting restarted during that time. To make things simpler to reason about, just use nMinimumChainWork as our anti-DoS threshold; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed).

Expect responses to a getheaders iff the node has a chain with more than nMinimumChainWork

sdaftuar · 2022-02-22T16:45:02Z

If I understand this correctly, this means that also under normal circumstances, if we are in IBD (and beyond the point of nMinimumChainWork), we could get inbound connections from peers that are also in IBD, and we would serve them headers and blocks, which would take away some bandwidth and could slow down our own IBD progress.

Agreed. I think this is not such a big concern, as other peers would only be downloading from us if they also were new nodes, and the likelihood of a new node coming online, learning our address from someone else and then doing IBD from us in the minutes/hours(?) that we may be finishing our IBD seems small. I imagine it's more likely the inbounds we get in that window are other nodes that have been online already anyway, and won't be completing IBD from us (but instead relaying us transactions).

I updated this PR so that we won't respond to the getheaders if we're importing or reindexing, and I addressed @Sjors's comment about checking for nullptr as well.

klementtan · 2022-03-03T16:10:08Z

crACK a35f963

Verified that:

gethearders will be ignored when (fImporting || fReindex) or (nChainWork < nMinimumChainWork)
Functional tests are comprehensive. Covers:
- When (nChainWork < nMinimumChainWork), gethearders will be ignored
- When (nChainWork >= nMinimumChainWork), gethearders` will not be ignored but the node is still in IBD

naumenkogs · 2022-03-11T12:28:42Z

ACK a35f963

It is indeed easier to reason about IBD edge cases if we drop tip-age check (and also it just works better without the check).

maflcko · 2022-05-31T10:05:23Z

review ACK a35f963 🗯

Show signature

Signature:

… chainwork

fanquake added the P2P label Jan 27, 2022

sdaftuar mentioned this pull request Jan 27, 2022

p2p: Sync chain more readily from inbound peers during IBD #24171

Merged

jonatack reviewed Jan 27, 2022

View reviewed changes

sdaftuar force-pushed the 2022-01-headers-response-requires-minchainwork branch from ea335c1 to 7ee8f57 Compare January 27, 2022 16:30

DrahtBot mentioned this pull request Jan 28, 2022

assumeutxo: net_processing changes #24008

Closed

jonatack mentioned this pull request Jan 31, 2022

validation: don't re-acquire cs_main during IBD in CChainState::IsInitialBlockDownload() #24220

Closed

Sjors reviewed Feb 1, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

Sjors mentioned this pull request Feb 1, 2022

rpc: warn that nodes ignore requests for old stale blocks #24226

Merged

mzumsande reviewed Feb 1, 2022

View reviewed changes

This was referenced Feb 2, 2022

assumeutxo #15606

Closed

p2p: Split network logging into two categories #24247

Closed

p2p: Split network logging into two categories #24247 #24273

Closed

sdaftuar added 2 commits February 22, 2022 11:34

Add test for getheaders behavior

a35f963

Expect responses to a getheaders iff the node has a chain with more than nMinimumChainWork

sdaftuar force-pushed the 2022-01-headers-response-requires-minchainwork branch from 7ee8f57 to a35f963 Compare February 22, 2022 16:35

klementtan approved these changes Mar 3, 2022

View reviewed changes

DrahtBot mentioned this pull request May 31, 2022

test: add coverage for non-hex value to -minimumchainwork #25253

Merged

maflcko mentioned this pull request May 31, 2022

Node won't serve blocks when connect=0, listen=1 set due to IBD logic #25048

Closed

maflcko merged commit 5f65aff into bitcoin:master May 31, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 31, 2022

Merge bitcoin#24178: p2p: Respond to getheaders if we have sufficient…

de11b67

… chainwork

fanquake mentioned this pull request Sep 1, 2022

contrib: add vasild to trusted keys #25871

Closed

bitcoin locked and limited conversation to collaborators May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

p2p: Respond to getheaders if we have sufficient chainwork #24178

p2p: Respond to getheaders if we have sufficient chainwork #24178

Uh oh!

sdaftuar commented Jan 27, 2022

Uh oh!

sdaftuar commented Jan 27, 2022

Uh oh!

jonatack left a comment •

edited

Loading

Uh oh!

sdaftuar commented Jan 27, 2022

Uh oh!

DrahtBot commented Jan 27, 2022 •

edited

Loading

Uh oh!

sipa commented Jan 27, 2022

Uh oh!

sdaftuar commented Jan 28, 2022

Uh oh!

Sjors left a comment

Uh oh!

Uh oh!

mzumsande left a comment •

edited

Loading

Uh oh!

sdaftuar commented Feb 22, 2022

Uh oh!

klementtan commented Mar 3, 2022 •

edited

Loading

Uh oh!

naumenkogs commented Mar 11, 2022

Uh oh!

maflcko commented May 31, 2022

Uh oh!

Uh oh!

p2p: Respond to getheaders if we have sufficient chainwork #24178

p2p: Respond to getheaders if we have sufficient chainwork #24178

Uh oh!

Conversation

sdaftuar commented Jan 27, 2022

Uh oh!

sdaftuar commented Jan 27, 2022

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Jan 27, 2022

Uh oh!

DrahtBot commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa commented Jan 27, 2022

Uh oh!

sdaftuar commented Jan 28, 2022

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Feb 22, 2022

Uh oh!

klementtan commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naumenkogs commented Mar 11, 2022

Uh oh!

maflcko commented May 31, 2022

Uh oh!

Uh oh!

jonatack left a comment •

edited

Loading

DrahtBot commented Jan 27, 2022 •

edited

Loading

mzumsande left a comment •

edited

Loading

klementtan commented Mar 3, 2022 •

edited

Loading