Implement BIPXXX's new softfork rules (The Great Consensus Cleanup)

TheBlueMatt commented at 11:08 pm on February 25, 2019: member

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.

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).

fanquake added the label Consensus on Feb 25, 2019

TheBlueMatt force-pushed on Feb 25, 2019

DrahtBot commented at 2:32 am on February 26, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15481 (Restrict timestamp when mining a diff-adjustment block to prev-600 by TheBlueMatt)
#15141 (Rewrite DoS interface between validation and net_processing by sdaftuar)
#14954 (build: Require python 3.5 by MarcoFalke)
#13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)
#13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
#13533 ([tests] Reduced number of validations in tx_validationcache_tests by lucash-dev)
#13360 ([Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012)

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.

fanquake deleted a comment on Feb 26, 2019

TheBlueMatt force-pushed on Feb 26, 2019

Increase timeout of featuer_assumevalid test to fix flaky tests

Increases timeout for travis after
b6f0db69a9c9cdf101371720351935121590d3aa (apparently) didn't
increase it enough.

0bf0b3c412

Restrict timestamp when mining a diff-adjustment block to prev-600

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.

1d72dd8b2d

Ensure the script-fail reason is correct on mandatory flags

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.

570eae7762

Avoid creating <= 64-byte transactions in most functional tests. f9b3940544

Implement BIPXXX's new softfork rules (The Great Consensus Cleanup)

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.

1f63030817

TheBlueMatt force-pushed on Feb 26, 2019

practicalswift commented at 8:09 pm on February 28, 2019: contributor

A small commit message typo: “eatuer_assumevalid” should be “feature_assumevalid” :-)

DrahtBot commented at 4:13 pm on March 5, 2019: member

DrahtBot added the label Needs rebase on Mar 5, 2019

maaku commented at 7:21 pm on March 5, 2019: contributor

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:

…It turns out this is to the advantage of bitcoin users as the time warp is an integral part of how we can make a block size increase and/or PoW change without a hard fork, and thereby allow such proposals be decided on their own merits rather than as a yea or nay decision on hard forks generally.

…This is an issue that has been known about for some time and persists as unfixed because, as I understand it, the likelihood of attack was low and likelihood of coordinated user response high. And since ideas like Forward Blocks require the time-warp bug to achieve forwards-compatible scaling of settlement capacity, I would think it short sighted to advocate for “fixing” the bug now.

However in discussing forward blocks with another developer, I happened upon a solution that would allow for the worst scenarios to be avoided while not blocking off the later deployment of forward blocks: If the current block timestamp is more than 4 hours beyond the median time of the past 11 blocks, then the difficulty adjustment errors if the adjustment would exceed a smaller limiting factor than the normal +/- 4x. Specifically I recommend ensuring that the block timestamp is within the range [start + 600*2015 - 4800, start + 600*2015 + 4800]. 4800 = 80 * 60 = 1 hour 20 minutes. This would allow decreases in the inter-block time of no more than 12.6% the first year of purposeful adjustments, and is easy to implement.

(Potentially the lower limit could be removed as it would be nice for the difficulty to be able to quickly reset, but asymmetric adjustments have been a source of error in the past.)

This would prevent … [censored bug disclosure] … while still allowing a rate of growth for forward blocks or other proposals to that is conservative slow enough as to not cause catastrophic failure. Requiring the block timestamp to be 4 hours ahead of the median time past ensures this rule is very unlikely to be engaged during normal operation of a healthy network, for which normal [unconstrained] difficulty adjustments can still occur.

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.

TheBlueMatt commented at 7:57 pm on March 5, 2019: member

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.

maaku commented at 8:03 pm on March 5, 2019: contributor

Ok, I hope you do (I’m not on the bitcoin mailing list).

Sjors commented at 7:12 pm on March 8, 2019: member

@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 OP_CODESEPERATOR change if you’re not familiar with #11423 that made it non-standard.

Shameless review plug for #12134, so we can test the interaction with older nodes.

DrahtBot commented at 12:45 pm on September 30, 2019: member

DrahtBot added the label Up for grabs on Sep 30, 2019

DrahtBot closed this on Sep 30, 2019

maaku commented at 12:51 pm on September 30, 2019: contributor

Please tell me that we don’t have a bot that auto closes issues.

Sjors commented at 1:29 pm on September 30, 2019: member

We do, but this one should stay open, @MarcoFalke.

maaku commented at 1:41 pm on September 30, 2019: contributor

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.

practicalswift commented at 1:47 pm on September 30, 2019: contributor

@maaku AFAIK DrahtBot does not close issues - only stale PR:s :)

maaku commented at 1:51 pm on September 30, 2019: contributor

I don’t see the relevant difference. But anyway, this’ll be my last comment on the matter.

MarcoFalke commented at 2:08 pm on September 30, 2019: member

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.

MarcoFalke reopened this on Sep 30, 2019

MarcoFalke removed the label Up for grabs on Sep 30, 2019

TheBlueMatt commented at 8:23 pm on November 12, 2019: member

The BIP needs cleanup, so no use leaving this open.

TheBlueMatt closed this on Nov 12, 2019

fanquake removed the label Needs rebase on Aug 20, 2020

laanwj referenced this in commit c8e68b418f on Oct 20, 2021

sidhujag referenced this in commit d11a7497a7 on Oct 20, 2021

DrahtBot locked this on Feb 15, 2022

Implement BIPXXX’s new softfork rules (The Great Consensus Cleanup) #15482

Conflicts