p2p: Avoid prematurely clearing download state for other peers #27608

sdaftuar · 2023-05-09T17:37:42Z

Avoid letting one peer send us data that clears out the download request (and related timers etc) from another peer.

The exception is if a block is definitely stored to disk, in which case we'll clear the download state (just as we do for compact blocks).

DrahtBot · 2023-05-09T17:37:46Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jamesob, instagibbs, fjahr, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26969 (net, refactor: net_processing, add ProcessCompactBlockTxns by brunoerg)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)

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.

jamesob · 2023-05-09T17:56:17Z

Concept ACK. Will start running this when CI checks out.

instagibbs

first glance, looks right to me. I was literally writing the same code as part of as learn-as-I-go resurrection of #10984

this would be required for any parallel download implementation as well

src/net_processing.cpp

jamesob

ACK 52e5207 (jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl)

Built, reviewed code locally. Deployed to b-02.slug on https://bmon.info; all
metrics there look normal (peer count, tip connection).

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4 ([`jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl`](https://github.com/jamesob/bitcoin/tree/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl))

Built, reviewed code locally. Deployed to b-02.slug on https://bmon.info; all
metrics there look normal (peer count, tip connection).

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmRalB8ACgkQepNdrbLE
TwXObg//dcajHcXotfGB5mdQGv0a+jXDv1mjXlXr551R1Lv+rhXMbyipB80GNdmu
IqOXGauUUeqnK2dwxwMMgYlJDa92ErQzAOB7vm44m1iNuSStGh9aqupbznl3D2WK
1D3DMiO5+ZfPmMRhGgVPHwr8ydsuJJ3suna6I5SEF4cbET0x6aLfkMrJ0NK11TfY
IUz+1XFpxMNiosUxWroUSUKlkZMI83LqjP9JFFyTwCNrfO3jEYTQRT+BABbTC2yk
k7OWkWBNdoeCSEgVyJ0fOWS5Fr438sL7xpNbWx3MOvU6EC6fV0oLxlE/R6TovQzN
U/s28D/Z/BlEGFYZbW/dB5JkMfllF8sj8SGOL9b/3u7AC6zJI5i3Q6/89f20lhVK
8iD1eo+iM7hdwUnQkgkd4D6Gh3e3GVf1643pVI3ioqWrDRpDrCzThSW4mUKic2vJ
0xDEuqHO/RWsUCoL+6uV3Rw7henmAZsaHzmV6Qf0G4T1apeBq21U7z8YjJmfLDvg
3q1rmvwYciGJzVkLdBHACgdLPpyL+aMDAY22mM3XQqafnNI2hG5qbB/ScueDF0H3
WLHdHkcsymY1pMUVyJc2CX9AiuckxW4vJX/t/D7RawsgO4KW78vkx/igBOI/QJYG
hMDyOm4kKIRhdXbqSZphxDwaFxcX+8wc6WJAUkEB5DbLN0tYx0g=
=6fob
-----END PGP SIGNATURE-----

instagibbs

ACK 52e5207

fjahr

Code review ACK 52e5207

fjahr · 2023-05-09T21:18:39Z

src/net_processing.cpp

+        // In case this block came from a different peer than we requested
+        // from, we can erase the block request now anyway (as we just stored
+        // this block to disk).
+        LOCK(cs_main);


nit: Could have moved the lock outside the if..else and removed the duplicate line in the else below.

mzumsande

Code Review ACK 52e5207

mzumsande · 2023-05-09T23:00:08Z

src/net_processing.cpp

@@ -1129,6 +1132,12 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash)
    }

    auto [node_id, list_it] = it->second;
+
+    if (from_peer && node_id != *from_peer) {
+        // Block was requested by another peer


nit: requested from another peer

Oops -- will fix in a followup. Thanks for catching.

"by another peer" reads fine to me?

peer didn't request, WE requested that peer

ah right, suggested text is also ambiguous....

"Block was requested by us from another peer" ?

Github-Pull: bitcoin#27608 Rebased-From: 52e5207

…or other peers 52e5207 p2p: Avoid prematurely clearing download state for other peers (Suhas Daftuar) Pull request description: Avoid letting one peer send us data that clears out the download request (and related timers etc) from another peer. The exception is if a block is definitely stored to disk, in which case we'll clear the download state (just as we do for compact blocks). ACKs for top commit: jamesob: ACK 52e5207 ([`jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl`](https://github.com/jamesob/bitcoin/tree/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl)) instagibbs: ACK 52e5207 fjahr: Code review ACK 52e5207 mzumsande: Code Review ACK 52e5207 Tree-SHA512: 3ee92507edc3303c16c70ca44ba6c28c104afe95196e4b9167032590ed23d4f569f654f8eb8758940bd6536bc9ca810d2a77d2739db386b927e8b3f3cf55cb16

instagibbs · 2023-05-10T15:06:28Z

src/net_processing.cpp

@@ -4400,7 +4414,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                // process from some other peer.  We do this after calling
                // ProcessNewBlock so that a malleated cmpctblock announcement
                // can't be used to interfere with block relay.
-                RemoveBlockRequest(pblock->GetHash());
+                RemoveBlockRequest(pblock->GetHash(), std::nullopt);


Is this block required anymore now that ProcessBlock will call it for us the first time when it's new?

Can't we have a race condition where the block is not new? In any case this function is extremely fast if called twice.

Github-Pull: bitcoin#27608 Rebased-From: 52e5207

dergoegge · 2023-05-11T10:54:31Z

post-merge Code review ACK 52e5207

Github-Pull: bitcoin#27608 Rebased-From: 52e5207

Sjors

utACK 52e5207

Sjors · 2023-05-11T09:39:33Z

src/net_processing.cpp

@@ -1164,7 +1173,7 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st
    }

    // Make sure it's not listed somewhere already.


This comment could be improved:

// When processing block related messages from our peers, // we treat requested blocks different from unsolicited ones. // When making this distinction we only keep track of the // last peer we requested from.

Sjors · 2023-05-11T14:34:28Z

src/net_processing.cpp

@@ -4400,7 +4414,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                // process from some other peer.  We do this after calling
                // ProcessNewBlock so that a malleated cmpctblock announcement
                // can't be used to interfere with block relay.
-                RemoveBlockRequest(pblock->GetHash());
+                RemoveBlockRequest(pblock->GetHash(), std::nullopt);


Can't we have a race condition where the block is not new? In any case this function is extremely fast if called twice.

97f5e28 doc: update release notes for 24.1rc3 (fanquake) 7e9c7ae doc: update manual pages for v24.1rc3 (fanquake) abb9fa0 build: bump version to v24.1rc3 (fanquake) 128da6e net_processing: Boost inv trickle rate (Anthony Towns) a9a861a txmempool: have CompareDepthAndScore sort missing txs first (Anthony Towns) ec7cd33 p2p: Avoid prematurely clearing download state for other peers (Suhas Daftuar) Pull request description: Backports for rc3. Currently: * #27608 * #27610 ACKs for top commit: josibake: ACK 97f5e28 dergoegge: ACK 97f5e28 achow101: ACK 97f5e28 glozow: ACK 97f5e28 brunoerg: ACK 97f5e28 hebasto: ACK 97f5e28, commits were backported locally, got zero diff. Tree-SHA512: 09572285ed1e8169d7e77d12ec438586dab54c86064de85d0e743564e601686f884bf74f2bf8ed1be73bddcd7db6da4277c6dd6b9732e7eca383e108f8f37d58

49a2d66 doc: update manual pages for v25.0rc2 (fanquake) 3ea4a11 build: bump version to v25.0rc2 (fanquake) 7ef71e3 net_processing: Boost inv trickle rate (Anthony Towns) 1adbcd3 txmempool: have CompareDepthAndScore sort missing txs first (Anthony Towns) 9a23079 p2p: Avoid prematurely clearing download state for other peers (Suhas Daftuar) Pull request description: Backports for rc2. Currently: * #27608 * #27610 ACKs for top commit: achow101: ACK 49a2d66 Tree-SHA512: a1a7678e16136636ec8a232d12630529639bae3b577769b5a5fd204dda234a5e588f3d4dfebf4d7abe7111d13cc0714f9ccdea0a858fe821a7146e6a697308d3

We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin#27608).

d8087ad [test] IsBlockMutated unit tests (dergoegge) 1ed2c98 Add transaction_identifier::size to allow Span conversion (dergoegge) 1ec6bbe [validation] Cache merkle root and witness commitment checks (dergoegge) 5bf4f5b [test] Add regression test for #27608 (dergoegge) 49257c0 [net processing] Don't process mutated blocks (dergoegge) 2d8495e [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) 66abce1 [validation] Introduce IsBlockMutated (dergoegge) e7669e1 [refactor] Cleanup merkle root checks (dergoegge) 95bddb9 [validation] Isolate merkle root checks (dergoegge) Pull request description: This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks. We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message. We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. #27608 for which a regression test is included in this PR). ACKs for top commit: achow101: ACK d8087ad maflcko: ACK d8087ad 🏄 fjahr: Code review ACK d8087ad sr-gi: Code review ACK d8087ad Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f

We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin#27608). Github-Pull: bitcoin#29412 Rebased-From: 49257c0

Github-Pull: bitcoin#29412 Rebased-From: 5bf4f5b

We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin#27608). Github-Pull: bitcoin#29412 Rebased-From: 49257c0

Github-Pull: bitcoin#29412 Rebased-From: 5bf4f5b

c68d4d0 [doc] update manual pages for v26.1rc2 (glozow) bd715bf [build] bump version to v26.1rc2 (glozow) b6d006d update release notes 26.1 (glozow) fce992b fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) 40c56a4 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 7c82b27 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) b5419ce p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) 0535c25 [test] IsBlockMutated unit tests (dergoegge) 8141498 [validation] Cache merkle root and witness commitment checks (dergoegge) 0c5c596 [test] Add regression test for #27608 (dergoegge) 2473635 [net processing] Don't process mutated blocks (dergoegge) 50c0b61 [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) aff368f [validation] Introduce IsBlockMutated (dergoegge) 076c67c [refactor] Cleanup merkle root checks (dergoegge) 97a1d0a [validation] Isolate merkle root checks (dergoegge) 4ac0eb5 test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds (Hennadii Stepanov) Pull request description: Includes: - #29357 - #29412 - #29524 - #29510 - #29529 Also does: - update to release notes - bump to rc2 - manpages - (no changes to bitcoin.conf) ACKs for top commit: achow101: ACK c68d4d0 Tree-SHA512: 2f8c3dd705e3f9f33403b3cc17e8006510ff827d7dbd609b09732a1669964e9b001cfecdc63d8d8daeb8f39c652e1e4ad0aac873d44d259c21803de85688ed2b

We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin#27608).

27cfda1 doc: Update release notes for 25.2rc2 (Ava Chow) daba5e2 doc: Update manpages for 25.2rc2 (Ava Chow) 8a0c980 build: Bump to 25.2rc2 (Ava Chow) cf7d3a8 p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) 3eaaafa [test] IsBlockMutated unit tests (dergoegge) 0667441 [validation] Cache merkle root and witness commitment checks (dergoegge) de97ecf [test] Add regression test for #27608 (dergoegge) 8cc4b24 [net processing] Don't process mutated blocks (dergoegge) 098f07d [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) 8804c36 [validation] Introduce IsBlockMutated (dergoegge) 4f5baac [validation] Isolate merkle root checks (dergoegge) f93be01 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 7c08ccf wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) Pull request description: Backport: * #29510 * #29412 * #29524 ACKs for top commit: glozow: utACK 27cfda1 Tree-SHA512: 37feadd65d9ea55c0a92c9d2a6f74f87cafed3bc67f8deeaaafc5b7042f954e55ea34816612e1a49088f4f1906f104e00c7c3bec7affd1c1f48220b57a8769c5

We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin/bitcoin#27608).

DrahtBot added the P2P label May 9, 2023

sdaftuar force-pushed the 2023-05-download-bug branch from 6857b1b to a9c0ea5 Compare May 9, 2023 17:47

DrahtBot added the CI failed label May 9, 2023

p2p: Avoid prematurely clearing download state for other peers

52e5207

sdaftuar force-pushed the 2023-05-download-bug branch from a9c0ea5 to 52e5207 Compare May 9, 2023 17:52

instagibbs reviewed May 9, 2023

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

jamesob added a commit to chaincodelabs/bmon that referenced this pull request May 9, 2023

deploy Suhas' bitcoin/bitcoin#27608

93f220f

jamesob approved these changes May 9, 2023

View reviewed changes

DrahtBot removed the CI failed label May 9, 2023

instagibbs approved these changes May 9, 2023

View reviewed changes

DrahtBot mentioned this pull request May 9, 2023

net, refactor: net_processing, add ProcessCompactBlockTxns #26969

Merged

fjahr reviewed May 9, 2023

View reviewed changes

This was referenced May 9, 2023

refactor: Introduce EvictionManager and use it for the inbound eviction logic #25572

Closed

refactor: Introduce EvictionManager #25268

Closed

mzumsande reviewed May 9, 2023

View reviewed changes

fanquake merged commit dbfc748 into bitcoin:master May 10, 2023
16 checks passed

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 10, 2023

p2p: Avoid prematurely clearing download state for other peers

9a23079

Github-Pull: bitcoin#27608 Rebased-From: 52e5207

This was referenced May 10, 2023

[25.0] Backports for rc2 #27613

Merged

[24.1] Backports for rc3 #27614

Merged

instagibbs reviewed May 10, 2023

View reviewed changes

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 11, 2023

p2p: Avoid prematurely clearing download state for other peers

ce8f812

Github-Pull: bitcoin#27608 Rebased-From: 52e5207

fanquake mentioned this pull request May 11, 2023

[23.2] Backports for rc1 #27624

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 11, 2023

p2p: Avoid prematurely clearing download state for other peers

ec7cd33

Github-Pull: bitcoin#27608 Rebased-From: 52e5207

Sjors reviewed May 11, 2023

View reviewed changes

dergoegge added a commit to dergoegge/bitcoin that referenced this pull request Feb 6, 2024

[test] Add regression test for bitcoin#27608

9e27f86

dergoegge added a commit to dergoegge/bitcoin that referenced this pull request Feb 8, 2024

[test] Add regression test for bitcoin#27608

2b2cc96

dergoegge mentioned this pull request Feb 8, 2024

p2p: Don't process mutated blocks #29412

Merged

dergoegge added a commit to dergoegge/bitcoin that referenced this pull request Feb 8, 2024

[test] Add regression test for bitcoin#27608

5f27c9b

dergoegge added a commit to dergoegge/bitcoin that referenced this pull request Feb 22, 2024

[test] Add regression test for bitcoin#27608

e2d1eb2

0xB10C pushed a commit to 0xB10C/bitcoin that referenced this pull request Feb 23, 2024

[test] Add regression test for bitcoin#27608

51d818c

0xB10C pushed a commit to 0xB10C/bitcoin that referenced this pull request Feb 23, 2024

[test] Add regression test for bitcoin#27608

8552e0d

dergoegge added a commit to dergoegge/bitcoin that referenced this pull request Feb 23, 2024

[test] Add regression test for bitcoin#27608

e46829f

dergoegge added a commit to dergoegge/bitcoin that referenced this pull request Feb 27, 2024

[test] Add regression test for bitcoin#27608

5bf4f5b

glozow pushed a commit to glozow/bitcoin that referenced this pull request Mar 5, 2024

[test] Add regression test for bitcoin#27608

0c5c596

Github-Pull: bitcoin#29412 Rebased-From: 5bf4f5b

achow101 pushed a commit to achow101/bitcoin that referenced this pull request Mar 5, 2024

[test] Add regression test for bitcoin#27608

de97ecf

Github-Pull: bitcoin#29412 Rebased-From: 5bf4f5b

kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024

[test] Add regression test for bitcoin#27608

faffa6f

kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024

[test] Add regression test for bitcoin#27608

766231a

bitcoin locked and limited conversation to collaborators May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: Avoid prematurely clearing download state for other peers #27608

p2p: Avoid prematurely clearing download state for other peers #27608

sdaftuar commented May 9, 2023

DrahtBot commented May 9, 2023 •

edited

Loading

jamesob commented May 9, 2023

instagibbs left a comment

jamesob left a comment

instagibbs left a comment

fjahr left a comment

fjahr May 9, 2023

mzumsande left a comment

mzumsande May 9, 2023

sdaftuar May 10, 2023

jamesob May 10, 2023

instagibbs May 10, 2023

instagibbs May 10, 2023

instagibbs May 10, 2023 •

edited

Loading

Sjors May 11, 2023

dergoegge commented May 11, 2023

Sjors left a comment

Sjors May 11, 2023

Sjors May 11, 2023

		@@ -1164,7 +1173,7 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st
		}

		// Make sure it's not listed somewhere already.

p2p: Avoid prematurely clearing download state for other peers #27608

p2p: Avoid prematurely clearing download state for other peers #27608

Conversation

sdaftuar commented May 9, 2023

DrahtBot commented May 9, 2023 • edited Loading

Reviews

Conflicts

jamesob commented May 9, 2023

instagibbs left a comment

Choose a reason for hiding this comment

jamesob left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzumsande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dergoegge commented May 11, 2023

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented May 9, 2023 •

edited

Loading

instagibbs May 10, 2023 •

edited

Loading