p2p: Respond to getheaders if we have sufficient chainwork

sdaftuar commented at 1:50 pm on January 27, 2022: member

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).

fanquake added the label P2P on Jan 27, 2022

sdaftuar commented at 1:53 pm on January 27, 2022: member

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 commented at 3:07 pm on January 27, 2022: member

Concept/Approach ACK

 0diff --git a/test/functional/feature_minchainwork.py b/test/functional/feature_minchainwork.py
 1index d596464b27..637623706a 100755
 2--- a/test/functional/feature_minchainwork.py
 3+++ b/test/functional/feature_minchainwork.py
 4@@ -17,9 +17,9 @@ only succeeds past a given node once its nMinimumChainWork has been exceeded.
 5 
 6 import time
 7 
 8+from test_framework.p2p import P2PInterface, msg_getheaders
 9 from test_framework.test_framework import BitcoinTestFramework
10 from test_framework.util import assert_equal
11-from test_framework.p2p import P2PInterface, msg_getheaders
12 
13 # 2 hashes required per regtest block (with no difficulty adjustment)
14 REGTEST_WORK_PER_BLOCK = 2
15@@ -43,7 +43,7 @@ class MinimumChainWorkTest(BitcoinTestFramework):
16             self.connect_nodes(i+1, i)
17 
18         # Set clock of node2 2 days ahead, to keep it in IBD during this test.
19-        self.nodes[2].setmocktime(int(time.time())+48*60*60)
20+        self.nodes[2].setmocktime(int(time.time()) + 48 * 60 * 60)
21 
22     def run_test(self):
23         # Start building a chain on node0.  node2 shouldn't be able to sync until node1's
24@@ -75,13 +75,12 @@ class MinimumChainWorkTest(BitcoinTestFramework):
25         assert self.nodes[1].getbestblockhash() != self.nodes[0].getbestblockhash()
26         assert_equal(self.nodes[2].getblockcount(), starting_blockcount)
27 
28-        # Check that getheaders requests to node2 are ignored.
29+        self.log.info("Test that getheaders requests to node2 are ignored")
30         peer = self.nodes[2].add_p2p_connection(P2PInterface())
31         msg = msg_getheaders()
32         msg.locator.vHave = [int(self.nodes[2].getbestblockhash(), 16)]
33         msg.hashstop = 0
34-        peer.send_message(msg)
35-        peer.sync_with_ping()
36+        peer.send_and_ping(msg)
37         time.sleep(5)
38         assert "headers" not in peer.last_message
39 
40@@ -100,13 +99,13 @@ class MinimumChainWorkTest(BitcoinTestFramework):
41         self.log.info(f"Blockcounts: {[n.getblockcount() for n in self.nodes]}")
42 
43-        # Now getheaders requests should not be ignored
44-        peer.send_message(msg)
45-        peer.sync_with_ping()
46+        self.log.info("Test that getheaders requests are not ignored")
47+        peer.send_and_ping(msg)
48         assert "headers" in peer.last_message
49 
50         # Verify that node2 is in fact still in IBD (otherwise this test may
51         # not be exercising the logic we want!)
52-        assert self.nodes[2].getblockchaininfo()['initialblockdownload']
53+        assert_equal(self.nodes[2].getblockchaininfo()['initialblockdownload'], True)
54+
55 
56 if __name__ == '__main__':
57     MinimumChainWorkTest().main()

sdaftuar force-pushed on Jan 27, 2022

sdaftuar commented at 4:37 pm on January 27, 2022: member

Thanks @jonatack – I’ve made the test cleanups you suggested.

DrahtBot commented at 10:01 pm on January 27, 2022: member

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 commented at 11:47 pm on January 27, 2022: member

@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 commented at 1:52 pm on January 28, 2022: member

@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:

0    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
1        return false;
2    if (fImporting || fReindex)
3        return true;
4    if (m_chain.Tip() == nullptr)
5        return true;
6    if (m_chain.Tip()->nChainWork < nMinimumChainWork)
7        return true;
8    if (m_chain.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge))
9        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.

in src/net_processing.cpp:3201 in 0b015e4740 outdated

3198+        // checkpointed chain would cause that peer to disconnect us. Requiring
3199+        // that our chainwork exceed nMinimumChainWork is a protection against
3200+        // being fed a bogus chain when we started up for the first time and
3201+        // getting partitioned off the honest network for serving that chain to
3202+        // others.
3203+        if (m_chainman.ActiveTip()->nChainWork < nMinimumChainWork && !pfrom.HasPermission(NetPermissionFlags::Download)) {

Sjors commented at 12:17 pm on February 1, 2022:

Check ActiveTip()-> isn’t null?

sdaftuar commented at 4:36 pm on February 22, 2022:

Done.

Sjors commented at 12:18 pm on February 1, 2022: member

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

mzumsande commented at 8:54 pm on February 1, 2022: member

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.

Respond to getheaders if we have sufficient chainwork

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).

ef6dbe6863

Add test for getheaders behavior

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

a35f963edf

sdaftuar force-pushed on Feb 22, 2022

sdaftuar commented at 4:45 pm on February 22, 2022: member

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 commented at 4:10 pm on March 3, 2022: contributor

crACK a35f963edf1a14ee572abea106fb0147575e4694

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

klementtan approved

naumenkogs commented at 12:28 pm on March 11, 2022: member

ACK a35f963edf1a14ee572abea106fb0147575e4694

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).

MarcoFalke commented at 10:05 am on May 31, 2022: member

review ACK a35f963edf1a14ee572abea106fb0147575e4694 🗯

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK a35f963edf1a14ee572abea106fb0147575e4694 🗯
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUidVQwAq04QTdny/dXKC/IXLM4WtJLPJyOzOZ66IPwE3YH6ZjXj8AVsykPH2Jrg
 8xLZT5Mosb5B9NqvxrYAiouFyIAANwN64cyvPADYLPwE0rbkuoribQ9EDaqYQ5voe
 9P1TTfcMFcCSvjcZLICfvNDhioL6r07kyx7f4UEJv0jOuYe8DtVBzYsN9VBtCFTio
105h0l5NOuz1BgaSANU8maoheias8ACmQCIb+UeMsBg8r90MmKfQfEyZkwhwQrpEHY
11KwM/JcHd2Ez6NhzXAT2hOknrle+38ePPet9+RxLzeeQGWsartoUFtQUhqBkZGCsU
12zZNl8lEMQsFuqXdWmAMFDcPY6hn3+lA69VO+bUQhSxvnIOxK11v83RSBgNzLDEeI
134kuUDpP/PHpOmlvBxKjyVy1p2W4GloJUgmHZV9QRcLXPOX65WdMyKIeknko7y4fe
14txoDAy7bg701l+B0f4DsNy0kYLfirFWhoo3ZbKpZgkx/cBbaXnWHzwJH03DLuXWQ
15H8tISUSz
16=A+Bc
17-----END PGP SIGNATURE-----

MarcoFalke merged this on May 31, 2022

MarcoFalke closed this on May 31, 2022

sidhujag referenced this in commit de11b67957 on May 31, 2022

DrahtBot locked this on May 31, 2023

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

Conflicts