p2p: Don't consider blocks mutated if they don't connect to known prev block #29524

instagibbs · 2024-03-01T11:50:19Z

Followup to #29412 to revert some of the behavior change that was likely unintentional.

Based on comments from #29412 (comment)

…v block

DrahtBot · 2024-03-01T11:50:22Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, Sjors, 0xB10C, sr-gi

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

instagibbs · 2024-03-01T11:56:41Z

Could this get a 27.0 milestone?

dergoegge

Code review ACK a1fbde0

Left my 2 sats on the tests but I can live with what you have here now.

dergoegge · 2024-03-01T12:07:06Z

test/functional/p2p_mutated_blocks.py

@@ -74,7 +79,7 @@ def self_transfer_requested():

        # Attempt to clear the honest relayer's download request by sending the
        # mutated block (as the attacker).
-        with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]):
+        with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]):


This is not necessary, right?

I added this assertion but asserting logs is not good practice: if it is really necessary then the interface under test should expose enough info to make the required assertion. Logs aren't a stable interface nor are they meant to serve as an interface for automated tests. Most of the time, duplicating them for test assertions just causes churn down the road. You don't want to force devs to "fix" a bunch of tests because they changed a user facing log message.

It's not strictly necessary, no, but if we somehow added a regression that missed the mutation check and only caught it in the AcceptBlock portion, this change would catch it? I guess this could be shortened to just catching "Block mutated:" since that's the actual test(making sure it's not making further in validation)?

I am not strongly opinionated on it.

but if we somehow added a regression that missed the mutation check and only caught it in the AcceptBlock portion, this change would catch it?

Sure but testing fine grained implementation details like this sounds more like the job of a unit test to me.

unit

The functional test are also testing a unit, which is Bitcoin Core ;)

if we don't care about why we're disconnecting, we could just not log read at all and disconnect? (or maybe we change this once a proper unit test covers it?)

I suggested that assertion in #29412 (comment) as a way of checking the disconnection reason in lack of a better way of doing so, but I do agree we shouldn't be relaying on asserting logs for this. I wouldn't mind if it gets dropped

dergoegge · 2024-03-01T12:21:45Z

test/functional/p2p_mutated_blocks.py

+        # Attacker gets a DoS score of 10, not immediately disconnected, so we do it 10 times to get to 100
+        for _ in range(10):
+            assert_equal(len(self.nodes[0].getpeerinfo()), 2)
+            with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]):
+                attacker.send_message(msg_block(block_missing_prev))
+        attacker.wait_for_disconnect(timeout=5)


I'm not sure if this is the right place for this test because it has nothing to do with mutated blocks. This test should have existed before my PR, so there is probably a better place for it.

Specifically the property we apparently care about is "a peer sending a disconnected block is assigned 10 DoS points" and a separate "if a peer has 100 DoS points it is disconnected". DoS points should be exposed through getpeerinfo (if they aren't already?) to test this properly which makes any log assertions superfluous.

it has nothing to do with mutated blocks

It kind of does, because we can't tell if the witness is mutated without knowing the height.

That said, perhaps p2p_unrequested_blocks.py is a better home.

(if they aren't already?)

That was to be my first attempt, but it's not exposed unless I'm having reading issues. Agreed it's better! I could also just have it run 10 times and make sure it disconnects, which does no log-reading and still gets the point across.

I would keep the log reading until there's another way to get the DoS points. It makes it easier to debug this test if if breaks.

I just opened a PR to add the misbehavior_score to getpeerinfo since this seems to be a recurring issue: #29530

Sjors

ACK a1fbde0

I checked that the test fails without this change.

I share @dergoegge's sentiment of decreasing our test dependency on logs, but that can wait for a followup.

Sjors · 2024-03-01T12:42:51Z

test/functional/p2p_mutated_blocks.py

+        # Attacker gets a DoS score of 10, not immediately disconnected, so we do it 10 times to get to 100
+        for _ in range(10):
+            assert_equal(len(self.nodes[0].getpeerinfo()), 2)
+            with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]):
+                attacker.send_message(msg_block(block_missing_prev))
+        attacker.wait_for_disconnect(timeout=5)


it has nothing to do with mutated blocks

It kind of does, because we can't tell if the witness is mutated without knowing the height.

instagibbs · 2024-03-01T17:18:13Z

For sake of a release I'm keeping things as-is unless I touch again.

0xB10C · 2024-03-01T17:35:40Z

utACK a1fbde0

sr-gi

tACK a1fbde0

I checked that the test would fail with AcceptBlock FAILED (bad-txnmrklroot, hashMerkleRoot mismatch) and the peer will be given 100 ban points before the change, while it would only be given 10 and the block will be rejected with AcceptBlock FAILED (prev-blk-not-found) after it.

I do agree that p2p_mutated_blocks.py is not the best place to include this last test though.

sr-gi · 2024-03-01T16:38:57Z

test/functional/p2p_mutated_blocks.py

@@ -74,7 +79,7 @@ def self_transfer_requested():

        # Attempt to clear the honest relayer's download request by sending the
        # mutated block (as the attacker).
-        with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]):
+        with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]):


I suggested that assertion in #29412 (comment) as a way of checking the disconnection reason in lack of a better way of doing so, but I do agree we shouldn't be relaying on asserting logs for this. I wouldn't mind if it gets dropped

…v block Github-Pull: bitcoin#29524 Rebased-From: a1fbde0

glozow · 2024-03-05T10:59:56Z

Backported for 26.x in #29509

…v block Github-Pull: bitcoin#29524 Rebased-From: a1fbde0

achow101 · 2024-03-05T17:19:08Z

Backported to 25.x in #29531

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

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

p2p: Don't consider blocks mutated if they don't connect to known pre…

a1fbde0

…v block

DrahtBot added the P2P label Mar 1, 2024

instagibbs mentioned this pull request Mar 1, 2024

p2p: Don't process mutated blocks #29412

Merged

dergoegge approved these changes Mar 1, 2024

View reviewed changes

Sjors approved these changes Mar 1, 2024

View reviewed changes

fanquake added this to the 27.0 milestone Mar 1, 2024

fanquake changed the title ~~p2p: Don't consider blocks mutated if they don't connect to known pre…~~ p2p: Don't consider blocks mutated if they don't connect to known prev block Mar 1, 2024

sr-gi reviewed Mar 1, 2024

View reviewed changes

sr-gi mentioned this pull request Mar 1, 2024

rpc/net: Adds misbehaving_score to getpeerinfo #29530

Closed

This was referenced Mar 2, 2024

doc: Clarify usage of checked witness commitment flag #29539

Closed

validation: Use witness maleation flag for non-segwit blocks #29540

Open

fanquake merged commit e60804f into bitcoin:master Mar 4, 2024
16 checks passed

0xB10C mentioned this pull request Mar 4, 2024

p2p: log mutated blocks when unknown & valid PoW #29549

Closed

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

p2p: Don't consider blocks mutated if they don't connect to known pre…

b5419ce

…v block Github-Pull: bitcoin#29524 Rebased-From: a1fbde0

glozow mentioned this pull request Mar 5, 2024

[26.x] backports and final changes for v26.1rc2 #29509

Merged

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

p2p: Don't consider blocks mutated if they don't connect to known pre…

cf7d3a8

…v block Github-Pull: bitcoin#29524 Rebased-From: a1fbde0

achow101 mentioned this pull request Mar 5, 2024

[25.x] backports #29531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: Don't consider blocks mutated if they don't connect to known prev block #29524

p2p: Don't consider blocks mutated if they don't connect to known prev block #29524

instagibbs commented Mar 1, 2024 •

edited by fanquake

DrahtBot commented Mar 1, 2024 •

edited

instagibbs commented Mar 1, 2024

dergoegge left a comment

dergoegge Mar 1, 2024

instagibbs Mar 1, 2024

dergoegge Mar 1, 2024

maflcko Mar 1, 2024

instagibbs Mar 1, 2024 •

edited

sr-gi Mar 1, 2024 •

edited

dergoegge Mar 1, 2024

Sjors Mar 1, 2024

Sjors Mar 1, 2024

instagibbs Mar 1, 2024 •

edited

Sjors Mar 1, 2024

sr-gi Mar 1, 2024

Sjors left a comment

Sjors Mar 1, 2024

instagibbs commented Mar 1, 2024

0xB10C commented Mar 1, 2024

sr-gi left a comment

sr-gi Mar 1, 2024 •

edited

glozow commented Mar 5, 2024

achow101 commented Mar 5, 2024

p2p: Don't consider blocks mutated if they don't connect to known prev block #29524

p2p: Don't consider blocks mutated if they don't connect to known prev block #29524

Conversation

instagibbs commented Mar 1, 2024 • edited by fanquake

DrahtBot commented Mar 1, 2024 • edited

Code Coverage

Reviews

instagibbs commented Mar 1, 2024

dergoegge 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

instagibbs Mar 1, 2024 • edited

Choose a reason for hiding this comment

sr-gi Mar 1, 2024 • edited

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 Mar 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Mar 1, 2024

0xB10C commented Mar 1, 2024

sr-gi left a comment

Choose a reason for hiding this comment

sr-gi Mar 1, 2024 • edited

Choose a reason for hiding this comment

glozow commented Mar 5, 2024

achow101 commented Mar 5, 2024

instagibbs commented Mar 1, 2024 •

edited by fanquake

DrahtBot commented Mar 1, 2024 •

edited

instagibbs Mar 1, 2024 •

edited

sr-gi Mar 1, 2024 •

edited

instagibbs Mar 1, 2024 •

edited

sr-gi Mar 1, 2024 •

edited