Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Feb 25, 2019

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

@TheBlueMatt TheBlueMatt force-pushed the 2019-02-great-consensus-cleanup branch 3 times, most recently from f3aba37 to 5e3d46c Feb 25, 2019
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Feb 26, 2019

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.

Loading

@bitcoin bitcoin deleted a comment Feb 26, 2019
@TheBlueMatt TheBlueMatt force-pushed the 2019-02-great-consensus-cleanup branch 2 times, most recently from 5d1b3c4 to 3de8267 Feb 26, 2019
TheBlueMatt added 5 commits Feb 26, 2019
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 TheBlueMatt force-pushed the 2019-02-great-consensus-cleanup branch from 3de8267 to 1f63030 Feb 26, 2019
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Feb 28, 2019

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

Loading

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Mar 5, 2019

Needs rebase

Loading

@maaku
Copy link
Contributor

@maaku maaku commented Mar 5, 2019

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.

Loading

@TheBlueMatt
Copy link
Contributor Author

@TheBlueMatt TheBlueMatt commented Mar 5, 2019

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.

Loading

@maaku
Copy link
Contributor

@maaku maaku commented Mar 5, 2019

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

Loading

@Sjors
Copy link
Member

@Sjors Sjors commented Mar 8, 2019

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

Loading

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 30, 2019

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.

Loading

@maaku
Copy link
Contributor

@maaku maaku commented Sep 30, 2019

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

Loading

@Sjors
Copy link
Member

@Sjors Sjors commented Sep 30, 2019

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

Loading

@maaku
Copy link
Contributor

@maaku maaku commented Sep 30, 2019

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.

Loading

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Sep 30, 2019

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

Loading

@maaku
Copy link
Contributor

@maaku maaku commented Sep 30, 2019

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

Loading

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Sep 30, 2019

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.

Loading

@TheBlueMatt
Copy link
Contributor Author

@TheBlueMatt TheBlueMatt commented Nov 12, 2019

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

Loading

laanwj added a commit that referenced this issue Oct 20, 2021
…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
sidhujag added a commit to syscoin/syscoin that referenced this issue Oct 20, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants