Implement BIPXXX's new softfork rules (The Great Consensus Cleanup) #15482
Conversation
TheBlueMatt
force-pushed
the
2019-02-great-consensus-cleanup
branch
3 times, most recently
from
f3aba37 to
5e3d46c
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
TheBlueMatt
force-pushed
the
2019-02-great-consensus-cleanup
branch
2 times, most recently
from
5d1b3c4 to
3de8267
Compare
Increases timeout for travis after b6f0db6 (apparently) didn't increase it enough.
This prepares us for a potential future timewarp-fixing softfork by ensuring that we always refuse to mine blocks which refuse to exploit timewarp, no matter the behavior of other miners. Note that we allow timestamp to go backwards by 600 seconds on the difficulty-adjustment blocks to avoid bricking any existing hardware which relies on the ability to roll nTime by up to 600 seconds. Note that it is possible that the eventual softfork to fix timewarp has a looser resetriction than the 600 seconds enforced here, however it seems unlikely we will apply a tighter one, and its fine if we restrict things further on the mining end than in consensus.
Previously, if we fail both due to a STANDARD_NOT_MANDATORY_SCRIPT_VERIFY_FLAGS and a MANDATORY_SCRIPT_VERIFY_FLAGS, we would potentially return the non-mandatory error as the reason for mandatory-script-verify-flag-failed. This ensures we always return the correct error.
Some notes: * Three tests use non-push opcodes in scriptSigs, so those have the fork explicitly disabled. * The 64-byte transaction check is executed in a new ContextualBlockPreCheck which must be run before CheckBlock (at least in the final checking before writing the block to disk). This function is a bit awkward but is seemingly the simplest way to implement the new check, with the caveat that, because the new function is called before CheckBlock, it can never return a non-CorruptionPossible error state.
TheBlueMatt
force-pushed
the
2019-02-great-consensus-cleanup
branch
from
3de8267 to
1f63030
Compare
|
A small commit message typo: "eatuer_assumevalid" should be "feature_assumevalid" :-) |
| Needs rebase |
|
Given the issues with the mailing list, and the lack of a PR adding the BIP, I'm going to write some thoughts I have regarding the general idea of the proposal here. My apologies if this seems like the wrong forum. I'm a bit surprised that the possibility of using forward blocks to achieve future scaling is not mentioned in the BIP or PR, since the absolute removal of the time-warp attack vector closes off the possibility of using the bug for forward- and backwards-compatible soft-fork scaling in the future, as I covered in my talk at Scaling Bitcoin last year. While developing the idea of forward blocks I sent a vulnerability report email to some of the senior Bitcoin Core developers. In it I outlayed some analysis of the attack vectors, and a suggested solution which both prevents the sort of attacks which could be devastating to the network, while still allowing uses of the time-warp "feature" to achieve on-chain scaling when it becomes necessary at some point in the future. I'll quote that part of the email here:
This approach additionally has the advantage of being less disruptive than what is implemented in this PR. No rule changes are engaged at all until such time as the adjustment block's timestamp is 4 hours ahead of median time past, so there is no risk to un-upgraded miners outside of a time-warp regime because a block more than 2 hours in the future is considered invalid. I think this modified rule would make it safe to switch to BIP8 instead of BIP9 for deployment. |
|
This is not the appropriate forum for having that discussion. I'll include further motivation in the email to the mailing list, so lets wait for the mailing list to be online before we open it up further. |
|
Ok, I hope you do (I'm not on the bitcoin mailing list). |
|
@maaku e.a. FYI mailinglist threads: Can you split the soft-fork rules into separate commits, away from the activation logic? E.g. it's hard to spot the Shameless review plug for #12134, so we can test the interaction with older nodes. |
| There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened. |
|
Please tell me that we don’t have a bot that auto closes issues. |
|
We do, but this one should stay open, @MarcoFalke. |
|
I highly suggest you disable that functionality altogether. There is no circumstance where it should ever be used. Actual human maintainers need to be involved to evaluate whether issues have been fixed, or to explain a wontfix closure. Worse it drives contributors away: nothing is more frustrating and alienating than to submit an issue, have it sit for a while, then be robo-closed. There is absolutely no reason to have an issue-closing bot. |
|
@maaku AFAIK |
|
I don't see the relevant difference. But anyway, this'll be my last comment on the matter. |
|
This pull request is the wrong place for meta discussions. You can contact me directly or open a new issue if you have any concerns or suggestions for improvements. |
|
The BIP needs cleanup, so no use leaving this open. |
…SegWit ef72e9b doc: nChainTx needs to become a 64-bit earlier due to SegWit (Sjors Provoost) Pull request description: As of block 597,379 txcount is 460,596,047 (see `chainparams.cpp`), while `uint32` can handle up to 4,294,967,296. Pre segwit the [minimum transaction size](https://en.bitcoin.it/wiki/Maximum_transaction_rate) was 166 bytes, so the worst case number of transactions per block was ~6000. As the original source comment for `unsigned int nChainTx` says, that should last until the year 2030. With SegWit the smallest possible transaction is 60 bytes (potentially increased to 65 with a future soft fork, see #15482), without a witness: ``` 4 bytes version 1 byte input count 36 bytes outpoint 1 byte scriptSigLen (0x00) 0 bytes scriptSig 4 bytes sequence 1 byte output count 8 bytes value 1 byte scriptPubKeyLen 1 byte scriptPubKey (OP_TRUE) 4 bytes locktime ``` That puts the maximum number of transactions per block at 16,666 so we might have to deal with this as early as a block 827,450 in early 2024. Given that it's a memory-only thing and we want to allow users many years to upgrade, I would suggest fixing this in v0.20 and back-porting it. ACKs for top commit: practicalswift: re-ACK ef72e9b jarolrod: ACK ef72e9b theStack: ACK ef72e9b Tree-SHA512: d8509ba7641796cd82af156354ff3a12ff7ec0f7b11215edff6696e95f8ca0e3596f719f3492ac3acb4b0884ac4e5bddc76f107b656bc2ed95a8ef1b2b5d4f71
…due to SegWit ef72e9b doc: nChainTx needs to become a 64-bit earlier due to SegWit (Sjors Provoost) Pull request description: As of block 597,379 txcount is 460,596,047 (see `chainparams.cpp`), while `uint32` can handle up to 4,294,967,296. Pre segwit the [minimum transaction size](https://en.bitcoin.it/wiki/Maximum_transaction_rate) was 166 bytes, so the worst case number of transactions per block was ~6000. As the original source comment for `unsigned int nChainTx` says, that should last until the year 2030. With SegWit the smallest possible transaction is 60 bytes (potentially increased to 65 with a future soft fork, see bitcoin#15482), without a witness: ``` 4 bytes version 1 byte input count 36 bytes outpoint 1 byte scriptSigLen (0x00) 0 bytes scriptSig 4 bytes sequence 1 byte output count 8 bytes value 1 byte scriptPubKeyLen 1 byte scriptPubKey (OP_TRUE) 4 bytes locktime ``` That puts the maximum number of transactions per block at 16,666 so we might have to deal with this as early as a block 827,450 in early 2024. Given that it's a memory-only thing and we want to allow users many years to upgrade, I would suggest fixing this in v0.20 and back-porting it. ACKs for top commit: practicalswift: re-ACK ef72e9b jarolrod: ACK ef72e9b theStack: ACK ef72e9b Tree-SHA512: d8509ba7641796cd82af156354ff3a12ff7ec0f7b11215edff6696e95f8ca0e3596f719f3492ac3acb4b0884ac4e5bddc76f107b656bc2ed95a8ef1b2b5d4f71
7 participants
Some notes:
the fork explicitly disabled.
ContextualBlockPreCheck which must be run before CheckBlock (at
least in the final checking before writing the block to disk).
This function is a bit awkward but is seemingly the simplest way
to implement the new check, with the caveat that, because the
new function is called before CheckBlock, it can never return a
non-CorruptionPossible error state.
Based on #15481. BIP available at https://github.com/TheBlueMatt/bips/blob/cleanup-softfork/bip-XXXX.mediawiki (I'll post it on the mailing list later tonight, hopefully).