[WIP] Implement BIP135 (generalized versionbits) #10437

sanch0panza · 2017-05-21T11:38:01Z

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 · 2017-05-21T12:03:56Z

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 · 2017-05-22T12:18:33Z

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

sanch0panza · 2017-05-22T14:59:40Z

@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 · 2017-05-22T15:06:09Z

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 BitcoinUnlimited/BitcoinUnlimited#458) .

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

jtimon · 2017-05-22T12:58:16Z

src/chainparams.cpp

@@ -339,6 +357,25 @@ const CChainParams &Params() {
    return *globalChainParams;
 }

+CChainParams& Params(const std::string& chain)


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

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

jtimon · 2017-05-22T12:59:44Z

src/consensus/params.h

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


this kind of thing can be done separately

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?

jtimon · 2017-05-22T13:02:25Z

src/versionbits.h

 enum ThresholdState {
-    THRESHOLD_DEFINED,


This is not necessary

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

jtimon · 2017-05-22T13:04:28Z

src/chainparams.h

@@ -58,6 +58,8 @@ class CChainParams
    };

    const Consensus::Params& GetConsensus() const { return consensus; }
+    /** Modifiable consensus parameters added by bip135 */
+    Consensus::Params& GetModifiableConsensus() { return consensus; }


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

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.

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.

Removed irrelevant part of header comment

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

…v-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

sanch0panza · 2017-05-25T10:01:32Z

Changing this to [WIP] ahead of a rebasing which will contain GetStateStatisticsFor() from 557c9a6 which needs to be checked, maybe it requires further adaptation to BIP135 .

sanch0panza · 2017-05-25T10:58:05Z

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 · 2018-01-22T08:45:03Z

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

sanch0panza mentioned this pull request May 22, 2017

Reference implementation of BIP135 (generalized versionbits) - deprecated by #1088 BitcoinUnlimited/BitcoinUnlimited#458

Closed

jtimon suggested changes May 22, 2017

View reviewed changes

sanch0panza added 9 commits May 25, 2017 11:40

[review] Remove unnecessary enum values

7d9a32e

[ext review] Remove sleeps in test

3e014d7

Removed irrelevant part of header comment

[ext review] Shorten statements to single return expression

6db7ca7

Fix up comments, remove obsolete commented-out section

967c52b

Renaming of BIP9Deployment* to ForkDeployment*

6d361c1

Update BIP9 reference to BIP135 in comments

00e830d

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

54ffc57

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

sanch0panza changed the title ~~Implement BIP135 (generalized versionbits)~~ [WIP] Implement BIP135 (generalized versionbits) May 25, 2017

sanch0panza force-pushed the bip135-core-dev-clean1 branch from f458a09 to dfe8ce9 Compare May 25, 2017 10:55

ajtowns mentioned this pull request Dec 10, 2017

Add Travis check for unused Python imports #11835

Merged

fanquake closed this Jan 22, 2018

fanquake added the Consensus label Jan 22, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implement BIP135 (generalized versionbits) #10437

[WIP] Implement BIP135 (generalized versionbits) #10437

sanch0panza commented May 21, 2017

sanch0panza commented May 21, 2017

jonasschnelli commented May 22, 2017

sanch0panza commented May 22, 2017

sanch0panza commented May 22, 2017

jtimon May 22, 2017

sanch0panza May 22, 2017

jtimon May 22, 2017

sanch0panza May 22, 2017 •

edited

Loading

jtimon May 22, 2017

sanch0panza May 22, 2017

jtimon May 22, 2017

sanch0panza May 22, 2017

sanch0panza commented May 25, 2017

sanch0panza commented May 25, 2017

fanquake commented Jan 22, 2018

[WIP] Implement BIP135 (generalized versionbits) #10437

[WIP] Implement BIP135 (generalized versionbits) #10437

Conversation

sanch0panza commented May 21, 2017

sanch0panza commented May 21, 2017

jonasschnelli commented May 22, 2017

sanch0panza commented May 22, 2017

sanch0panza commented May 22, 2017

jtimon May 22, 2017

Choose a reason for hiding this comment

sanch0panza May 22, 2017

Choose a reason for hiding this comment

jtimon May 22, 2017

Choose a reason for hiding this comment

sanch0panza May 22, 2017 • edited Loading

Choose a reason for hiding this comment

jtimon May 22, 2017

Choose a reason for hiding this comment

sanch0panza May 22, 2017

Choose a reason for hiding this comment

jtimon May 22, 2017

Choose a reason for hiding this comment

sanch0panza May 22, 2017

Choose a reason for hiding this comment

sanch0panza commented May 25, 2017

sanch0panza commented May 25, 2017

fanquake commented Jan 22, 2018

sanch0panza May 22, 2017 •

edited

Loading