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

instagibbs commented at 11:50 am on March 1, 2024: member

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

Based on comments from #29412 (review)

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

DrahtBot commented at 11:50 am on March 1, 2024: contributor

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.

DrahtBot added the label P2P on Mar 1, 2024

instagibbs commented at 11:56 am on March 1, 2024: member

Could this get a 27.0 milestone?

in test/functional/p2p_mutated_blocks.py:82 in a1fbde0ef7

78@@ -74,7 +79,7 @@ def self_transfer_requested():
79 
80         # Attempt to clear the honest relayer's download request by sending the
81         # mutated block (as the attacker).
82-        with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]):
83+        with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]):

dergoegge commented at 12:07 pm on March 1, 2024:

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.

instagibbs commented at 12:59 pm on March 1, 2024:

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.

dergoegge commented at 1:26 pm on March 1, 2024:

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.

maflcko commented at 1:35 pm on March 1, 2024:

unit

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

instagibbs commented at 1:46 pm on March 1, 2024:

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

sr-gi commented at 4:38 pm on March 1, 2024:

I suggested that assertion in #29412 (review) 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

in test/functional/p2p_mutated_blocks.py:112 in a1fbde0ef7

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

dergoegge commented at 12:21 pm on March 1, 2024:

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.

Sjors commented at 12:42 pm on March 1, 2024:

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.

Sjors commented at 12:52 pm on March 1, 2024:

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

instagibbs commented at 1:01 pm on March 1, 2024:

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

Sjors commented at 2:07 pm on March 1, 2024:

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.

sr-gi commented at 9:54 pm on March 1, 2024:

I just opened a PR to add the misbehavior_score to getpeerinfo since this seems to be a recurring issue: https://github.com/bitcoin/bitcoin/pull/29530

dergoegge approved

dergoegge commented at 12:26 pm on March 1, 2024: member

Code review ACK a1fbde0ef7cf6c94d4a5181f8ceb327096713160

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

Sjors approved

Sjors commented at 12:49 pm on March 1, 2024: member

ACK a1fbde0ef7cf6c94d4a5181f8ceb327096713160

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.

fanquake added this to the milestone 27.0 on Mar 1, 2024

fanquake renamed this:
~~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
on Mar 1, 2024

instagibbs commented at 5:18 pm on March 1, 2024: member

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

0xB10C commented at 5:35 pm on March 1, 2024: contributor

utACK a1fbde0ef7cf6c94d4a5181f8ceb327096713160

sr-gi commented at 6:39 pm on March 1, 2024: member

tACK https://github.com/bitcoin/bitcoin/commit/a1fbde0ef7cf6c94d4a5181f8ceb327096713160

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.

fanquake merged this on Mar 4, 2024

fanquake closed this on Mar 4, 2024

glozow referenced this in commit b5419ce6b6 on Mar 5, 2024

glozow commented at 10:59 am on March 5, 2024: member

Backported for 26.x in #29509

achow101 referenced this in commit cf7d3a8cd0 on Mar 5, 2024

achow101 commented at 5:19 pm on March 5, 2024: member

Backported to 25.x in #29531

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

Code Coverage

Reviews