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

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:2024-02-disconnected-mutated changing 2 files +23 −2
  1. 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)

  2. p2p: Don't consider blocks mutated if they don't connect to known prev block a1fbde0ef7
  3. 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.

  4. DrahtBot added the label P2P on Mar 1, 2024
  5. instagibbs commented at 11:56 am on March 1, 2024: member
    Could this get a 27.0 milestone?
  6. 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
  7. 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
  8. dergoegge approved
  9. 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.

  10. Sjors approved
  11. 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.

  12. fanquake added this to the milestone 27.0 on Mar 1, 2024
  13. 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
  14. 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.
  15. 0xB10C commented at 5:35 pm on March 1, 2024: contributor
    utACK a1fbde0ef7cf6c94d4a5181f8ceb327096713160
  16. 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.

  17. fanquake merged this on Mar 4, 2024
  18. fanquake closed this on Mar 4, 2024

  19. glozow referenced this in commit b5419ce6b6 on Mar 5, 2024
  20. glozow commented at 10:59 am on March 5, 2024: member
    Backported for 26.x in #29509
  21. achow101 referenced this in commit cf7d3a8cd0 on Mar 5, 2024
  22. achow101 commented at 5:19 pm on March 5, 2024: member
    Backported to 25.x in #29531

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me