[WIP] Implement BIP135 (generalized versionbits) #10437

sanch0panza commented at 11:38 AM on May 21, 2017: none

This is an implementation of https://github.com/bitcoin/bips/blob/master/bip-0135.mediawiki .

It extends the BIP9 state machine processing with configurable per-bit window size, activation threshold and grace period parameters (minlockedblocks and minlockedtime).

The built-in deployments are parameterized to maintain backward compatibility, and the existing BIP9 tests are retained unmodified except for a minor fix made to versionbits_tests to improve the disjointness checks for configured bits).

A bip135_forks section has been added to the getblockchaininfo RPC output, leaving the existing bip9_softforks section for backward compatibility.

The check for 'unknown versions being mined' has been altered to take into account that for unknown bits, we can no longer rely on a 95% activation threshold. The p2p-versionbits-warning test has been modified accordingly - the new logic warns when an unknown bit exceeds 50/100 of the last blocks.

NOTE: This implementation contains a specific feature which is not covered by the specification (and thus not strictly required for BIP135): the optional loading of fork configuration from a CSV file (using -forks=datafile command line option) and the ability to dump out the built-in configuration in CSV format (-dumpforks option). This has been retained from the original reference implementation since it makes testing and adaptation of the configuration easier.

sanch0panza commented at 12:03 PM on May 21, 2017: none

Could someone advise why the new subtree would not be visible to the Travis build? I don't have a problem building and testing this locally.

jonasschnelli commented at 12:18 PM on May 22, 2017: contributor

Why use CSV? We already have an internal JSON parser (UniValue) or a flexible configuration file reader...

in src/chainparams.cpp:360 in f1eb700562 outdated

 356 | @@ -339,6 +357,25 @@ const CChainParams &Params() {
 357 |      return *globalChainParams;
 358 |  }
 359 |  
 360 | +CChainParams& Params(const std::string& chain)

jtimon commented at 12:58 PM on May 22, 2017:

why not use CreateChainParams? when are the chainparams created here destroyed?

sanch0panza commented at 6:03 PM on May 22, 2017:

You're right, this function should not be needed and I will re-use CreateChainParams. The params created in Params() are not explicitly destroyed.

in src/consensus/params.h:2 in f1eb700562 outdated

   0 | @@ -1,5 +1,5 @@
   1 |  // Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | -// Copyright (c) 2009-2016 The Bitcoin Core developers
   3 | +// Copyright (c) 2009-2017 The Bitcoin Core developers

jtimon commented at 12:59 PM on May 22, 2017:

this kind of thing can be done separately

sanch0panza commented at 5:57 PM on May 22, 2017:

I have a habit of updating the copyright dates whenever I modify a file. What do you mean by done separately - should I not update them, or rather do it in a separate commit?

in src/versionbits.h:21 in f1eb700562 outdated

  21 | +/** Threshold to use for assessing warning of unknown bits */
  22 | +static const int BIT_WARNING_THRESHOLD = 50;
  23 |  
  24 | +// bip135: assigned numbers to these enum values
  25 |  enum ThresholdState {
  26 | -    THRESHOLD_DEFINED,

jtimon commented at 1:02 PM on May 22, 2017:

This is not necessary

sanch0panza commented at 5:56 PM on May 22, 2017:

Agreed - will remove. I might have wanted to set these to known values for debugging purposes.

in src/chainparams.h:62 in f1eb700562 outdated

  57 | @@ -58,6 +58,8 @@ class CChainParams
  58 |      };
  59 |  
  60 |      const Consensus::Params& GetConsensus() const { return consensus; }
  61 | +    /** Modifiable consensus parameters added by bip135 */
  62 | +    Consensus::Params& GetModifiableConsensus() { return consensus; }

jtimon commented at 1:04 PM on May 22, 2017:

NACK both this method and ModifiableParams, neither of them should be needed.

sanch0panza commented at 5:55 PM on May 22, 2017:

I agree, this can probably be improved by re-using existing code. Will look into that - I'm not so familiar with the recent Core code and so used some methods from existing implementation.

sanch0panza commented at 2:59 PM on May 22, 2017: none

@jonasschnelli : I used CSV as it seemed more suited to the type of configuration, with all data for one deployment fitting compactly on a single line, and most parsers can parse a comment header so it met the "self-documenting" request received during BIP135 review.

But I agree, JSON could well be used.

In fact, the loading of data from a file could be removed entirely to streamline this implementation - it is something that an implementation can choose to do whichever way it wants. Others have suggested to be able to specify it using multiple -bip135=<data> arguments, which would be another option.

I did not really want to mix this fork configuration with the users existing config file, as it seems something which which ordinary users should not really tinker.

sanch0panza commented at 3:06 PM on May 22, 2017: none

I would like to include some more improvement commits based on code review feedback I received so far (on code parts which are common to https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/458) .

I'll keep these as separate commits on top of this branch for now, for easier separate review of these additional changes.

jtimon changes_requested

Implement BIP135 (generalized versionbits)

This is an implementation of https://github.com/bitcoin/bips/blob/master/bip-0135.mediawiki .

It extends the BIP9 state machine processing with configurable per-bit
window size, activation threshold and grace period parameters
(minlockedblocks and minlockedtime).

The built-in deployments are parameterized to maintain backward compatibility,
and the existing BIP9 tests are retained unmodified except for a minor fix
made to `versionbits_tests` to improve the disjointness checks for configured bits).

A `bip135_forks` section has been added to the `getblockchaininfo` RPC output,
leaving the existing `bip9_softforks` section for backward compatibility.

The check for 'unknown versions being mined' has been altered to take into
account that for unknown bits, we can no longer rely on a 95% activation
threshold. The `p2p-versionbits-warning` test has been modified accordingly -
the new logic warns when an unknown bit exceeds 50/100 of the last blocks.

NOTE: This implementation contains a specific feature which is not covered by the
specification (and thus not strictly required for BIP135): the optional loading of
fork configuration from a CSV file (using `-forks=datafile` command line option)
and the ability to dump out the built-in configuration in CSV format
(`-dumpforks` option). This has been retained from the original reference
implementation since it makes testing and adaptation of the configuration easier.

e62ccde4f7

[review] Remove unnecessary enum values 7d9a32e93e

[ext review] Remove sleeps in test

Removed irrelevant part of header comment

3e014d73e6

[ext review] Shorten statements to single return expression 6db7ca76a9

Fix up comments, remove obsolete commented-out section 967c52b020

Renaming of BIP9Deployment* to ForkDeployment* 6d361c1b3e

Update BIP9 reference to BIP135 in comments 00e830d3e6

Squashed 'src/fast-cpp-csv-parser/' content from commit 769c042

git-subtree-dir: src/fast-cpp-csv-parser
git-subtree-split: 769c042c453bd94537bba7bb276453c13ec4db1b

54ffc57af9

Subtree import of Ben Strasser's fast CSV parser into src/fast-cpp-csv-parser

Merge commit '54ffc57af91917e7fafd9a24e1fcb38a42fee36c' as 'src/fast-cpp-csv-parser'

Command used:
git subtree add --squash --prefix src/fast-cpp-csv-parser https://github.com/ben-strasser/fast-cpp-csv-parser master

dfe8ce957f

sanch0panza commented at 10:01 AM on May 25, 2017: none

Changing this to [WIP] ahead of a rebasing which will contain GetStateStatisticsFor() from https://github.com/bitcoin/bitcoin/commit/557c9a68fb72aeb535f2efe3cc82d3f5e66c6ad3 which needs to be checked, maybe it requires further adaptation to BIP135 .

sanch0panza renamed this:
~~Implement BIP135 (generalized versionbits)~~
[WIP] Implement BIP135 (generalized versionbits)
on May 25, 2017

sanch0panza force-pushed on May 25, 2017

sanch0panza commented at 10:58 AM on May 25, 2017: none

Rebased.

The review comments addressing the "Modifiable" functions are still work in progress, as is revising the BIP9 statistics that were introduced by the rebase.

fanquake commented at 8:45 AM on January 22, 2018: member

Going to close this PR for now. It needs a large rebase, and hasn't generated much discussion.

fanquake closed this on Jan 22, 2018

fanquake added the label Consensus on Jan 22, 2018

DrahtBot locked this on Sep 8, 2021