test: Add time-timewarp-attack boundary cases #30698

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:2024-08-timewarp-boundary changing 1 files +9 −0
  1. instagibbs commented at 4:44 pm on August 22, 2024: member
    Basic addition to test case added in #30681
  2. test: Add time-timewarp-attack boundary cases 31378d44f4
  3. DrahtBot commented at 4:44 pm on August 22, 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 fjahr, tdb3, achow101
    Concept ACK Sjors

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

  4. instagibbs marked this as ready for review on Aug 22, 2024
  5. DrahtBot added the label Tests on Aug 22, 2024
  6. fjahr commented at 7:05 pm on August 22, 2024: contributor
    Code review ACK 31378d44f44cabc576bf92ddd61afde4b528de77
  7. fanquake added this to the milestone 28.0 on Aug 23, 2024
  8. in test/functional/mining_basic.py:160 in 31378d44f4
    158@@ -159,6 +159,15 @@ def test_timewarp(self):
    159         bad_block.solve()
    160         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
    


    tdb3 commented at 0:39 am on August 24, 2024:

    I’m not immediately seeing why the previous check (for the bad_block.nTime = t case) is needed anymore now that the check closer to the boundary is being added (bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1 case). Maybe I’m overlooking something?

    If not, then could add a commit that removes the check for the bad_block.nTime = t case.

  9. tdb3 approved
  10. tdb3 commented at 0:47 am on August 24, 2024: contributor

    ACK 31378d44f44cabc576bf92ddd61afde4b528de77

    Great addition (testing the boundary, i.e. MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP). Tested locally (and sanity checked that the assert would catch removing the -1 from bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1).

    Left one comment, but it doesn’t block the ACK.

  11. in test/functional/mining_basic.py:167 in 31378d44f4
    158@@ -159,6 +159,15 @@ def test_timewarp(self):
    159         bad_block.solve()
    160         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
    161 
    162+        self.log.info("Test timewarp protection boundary")
    163+        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1
    164+        bad_block.solve()
    165+        assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
    166+
    167+        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP
    


    Sjors commented at 9:09 am on August 26, 2024:

    Let’s use block here, since it’s not actually bad.

    Alternatively you could modify the test above to not bother copying block to bad_block.

  12. Sjors commented at 9:10 am on August 26, 2024: member
    Concept ACK
  13. achow101 commented at 6:28 pm on August 26, 2024: member

    ACK 31378d44f44cabc576bf92ddd61afde4b528de77

    Going to merge this now in the interest of having test coverage of the boundary before the 28.x branch off.

  14. DrahtBot requested review from Sjors on Aug 26, 2024
  15. achow101 merged this on Aug 26, 2024
  16. achow101 closed this on Aug 26, 2024


instagibbs DrahtBot fjahr tdb3 Sjors achow101


Sjors

Labels
Tests

Milestone
28.0


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-10-30 00:12 UTC

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