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 copy-paste ConflictsNo conflicts as of last run. |
Sjors
force-pushed
the
2025/01/timewarp-grief
branch
from
6c85bb7 to
6065b78
Compare
|
#31600 landed, ready for review |
test/functional/mining_basic.py
Outdated
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 <=
test/functional/mining_basic.py
Outdated
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.
test/functional/mining_basic.py
Outdated
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.
Sjors
force-pushed
the
2025/01/timewarp-grief
branch
from
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.
Sjors
force-pushed
the
2025/01/timewarp-grief
branch
from
d016d4f to
37d2246
Compare
|
@Prabhat1308 can you update your review to the latest version? I've rebased it just in case. |
|
re-ACK |
darosior
left a comment
darosior
left a comment
There was a problem hiding this comment.
This is adding a test that demonstrates how Bitcoin Core would reject a block from a broken mining pool software that:
- ignores the block chain and set the block timestamp using wall clock time;
- ignores the
mintimevalue provided by Bitcoin Core's block template; - ignores the
curtimevalue provided by Bitcoin Core's block template.
There is many, many ways a mining pool software that disregards consensus rules and values provided by Bitcoin Core's block template may create an invalid block. I don't think it's useful to exercise one such convoluted scenario in particular in Bitcoin Core's functional test suite. For this reason i am by default a (weakly held) Concept NACK on this change.
Weakly held because it's simply a test change and i'm happy to be convinced otherwise.
|
BIP54 has been updated to use a 2 hour grace period rather than the If you change Note that the list of three bullet items boils down to a single mistake: using wall time for |
5 participants
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.