test: clarify timewarp grace period griefing attack #31725
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31725. 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. ConflictsNo conflicts as of last run. |
6c85bb7 to
6065b78
Compare
|
#31600 landed, ready for review |
test/functional/mining_basic.py
Outdated
| self.generate(self.wallet, 1, sync_fun=self.no_op) | ||
|
|
||
| self.log.info("Create block two hours in the future") | ||
| self.nodes[0].setmocktime(t + MAX_FUTURE_BLOCK_TIME) | ||
| self.log.info("Create block MAX_TIMEWARP < t < MAX_FUTURE_BLOCK_TIME in the future") |
There was a problem hiding this comment.
The log mentions t to be strictly less than MAX_FUTURE_BLOCK_TIME. But in line 165 , the assert statement is for future<=MAX_FUTURE_BLOCK_TIME.
There was a problem hiding this comment.
Changed the log statement to <=
| # timestamp to MTP + 1, but doesn't take the new timewarp rule into | ||
| # account (and ignores mintime). | ||
| mtp = node.getblock(node.getbestblockhash())["mediantime"] + 1 | ||
| bad_block.nTime = mtp + 1 |
There was a problem hiding this comment.
on previous line 199 , mtp is set to node.getblock(node.getbestblockhash())["mediantime"] + 1 . from the rpc commands , shouldnt mtp be just node.getblock(node.getbestblockhash())["mediantime"] ? and then nTime is correct with mtp + 1. Have checked it locally and the test passes.
| tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS) | ||
| assert_greater_than_or_equal(tmpl['curtime'], t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP) | ||
| assert_equal(tmpl['curtime'], future - MAX_TIMEWARP) |
There was a problem hiding this comment.
this is introducing a more stricter equality. Is it because we now have the liberty to choose future as we wish whereas previously it was t + MAX_FUTURE_BLOCK_TIME which not necessarily be equal to template's curtime ?
There was a problem hiding this comment.
more stricter equality.
Stricter than what? I don't fully remember what the original test was trying to do here. But we know the exact value to expect.
curtime is equal to nTime of the template, which is controlled by UpdateTime() in node/miner.cpp. It starts with the actual current time, but then increases if needed to account for the MTP+1 rule and timewarp - and no more than that.
6065b78 to
d016d4f
Compare
|
There hasn't been much activity lately. What is the status here? Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future. |
1 similar comment
|
There hasn't been much activity lately. What is the status here? Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future. |
On testnet4 with the timewarp mitigation active, when pool software ignores the curtime and mintime fields provided by the getblocktemplate RPC or by createNewBlock() in the Mining interface, they are vulnerable to a griefing attack. The test is expanded to illustrate this.
d016d4f to
37d2246
Compare
|
@Prabhat1308 can you update your review to the latest version? I've rebased it just in case. |
|
re-ACK |
Adjust the timewarp test to better illustrate the griefing attack discussed here: https://delvingbitcoin.org/t/timewarp-attack-600-second-grace-period/1326/19
Changing
MAX_TIMEWARPto something> MAX_FUTURE_BLOCK_TIMEinconsensus.handmining_basic.pywill cause the updated test to fail. I'm not proposing such a change here of course. The new test should be useful guidance for pool software developers, for why they really should usecurtime, or least not ignoremintime.Additionally, if the proposal is changed to make
MAX_TIMEWARP > MAX_FUTURE_BLOCK_TIMEthen this test will break, which could be used to demonstrate there's no such griefing attack anymore.Originally part of #31600.