test: Add time-timewarp-attack boundary cases #30698
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Code review ACK 31378d4 |
There was a problem hiding this comment.
ACK 31378d4
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.
| @@ -159,6 +159,15 @@ def test_timewarp(self): | |||
| bad_block.solve() | |||
| assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex())) | |||
There was a problem hiding this comment.
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.
| bad_block.solve() | ||
| assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex())) | ||
|
|
||
| bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP |
There was a problem hiding this comment.
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.
|
Concept ACK |
|
ACK 31378d4 Going to merge this now in the interest of having test coverage of the boundary before the 28.x branch off. |
Basic addition to test case added in #30681