Minimal BIP9 implementation #7575

sipa · 2016-02-21T22:18:12Z

This is an alternative to #6816 and #7566, implementing BIP9 with minimal code changes, including mining logic, softfork warning logic, and some unit tests.

It adds support for a begin time before version bits are counted, which is not in BIP9 currently, though it is compatible.

The last commit demonstrates how to add a simple softfork.

btcdrak · 2016-02-22T02:30:42Z

Could you add deployment status to the getblockchaininfo RPC?

dcousens · 2016-02-22T03:51:30Z

@sipa any reason not to just extend #7566?

once-over utACK 50278bb, concept ACK

btcdrak · 2016-02-22T04:13:30Z

Concept ACK, I prefer this BIP9 implementation.

sipa · 2016-02-22T04:19:06Z

@dcousens I wrote most of this before I saw #7566, which would need some refactoring anyway to support the warning logic. I did use the params structure from there.

jonasschnelli · 2016-02-22T16:18:21Z

There are some versionbits_test.cpp formatting/syntax issues (https://travis-ci.org/bitcoin/bitcoin/jobs/110833452#L1736)

sipa · 2016-02-24T21:43:29Z

Fixed tests.

jtimon · 2016-02-25T19:53:39Z

src/main.cpp

+        }
+    }
+
+    if (nVersion == 0) {


Why not simply start using the versionbit prefix without voting for any concrete bit instead?

Why not simply start using the versionbit prefix without voting for any
concrete bit instead?

That would mean that versionbits on itself is a change, and lack of
adoption of versionbits by miners would then cause the warning logic to
trigger on new nodes.

Using this logc here, versionbits gets rolled out simultaneously with the
first fork that uses it.

I mean, if we are going to wait to deploy the first SF to merge it, I think it makes even more sense that we never return Consensus::VERSIONBITS_LAST_OLD_BLOCK_VERSION here.

That would mean that versionbits on itself is a change, and lack of
adoption of versionbits by miners would then cause the warning logic to
trigger on new nodes.

Just like changing the version to 8 without doing anything else would be. But it doesn't affect functionality.
The new nodes would only get warnings if the miners, apart from adopting this change were adopting or implementing other change to vote on concrete bits (if they only set nVersion to Consensus::VERSIONBITS_TOP_BITS), no warnings should be raised (unless an unknown SF has been deployed using versionbits, in which case is good that they get the warnings).

Assume the start date for the first bip9 softfork is april 1st, but we release code with bip9 and that softfork on march 1st. That means that it's likely that during march no new-style nVersion will be mined. That also means that anyone who does run upgraded code will get warnings, because the nVersion they expect (0x20000000) differs from the one they see (4).

Ok, I get it, it's the old nodes seeing the "empty versionbits" version which would get (unnecessarily?) be told to upgrade and it doesn't matter whether we introduce a SF with BIP9 or not because the start date will be after the miners start using the new code to produce their new versions.

But I think I still disagree, though.
If in march 15th 95% of the miners are ready to deploy a softfork scheduled for april 1st, I believe old nodes should actually receive the upgrade warning already, even if the message is not accurate.

EDIT: anyway, this disagreement is minor in the sense that it cannot greatly affect the implementation, the difference will be about 3-lines. So don't hurry to answer this.

EDIT: nah, 95% of miner will use BIP9 later, never mind, question answered.

sipa · 2016-03-03T04:51:03Z

Updated to correspond to latest BIP9 (see bitcoin/bips#344, bitcoin/bips#345, bitcoin/bips#346), and fixed the tests.

I also have an alternate version that factors out the cache, see sipa@218d2fd).

sipa · 2016-03-03T23:49:12Z

Moved the versionbits logic out of consensus, and make it not depend on it is
Added (very basic) reporting of BIP9 deployment status through RPC
Switched the window size from 2016 to 144 for regtest
Factored the cache out of the versionbits logic classes

Should be ready for review.

jtimon · 2016-03-04T00:50:26Z

utACK 7662816
EDIT: To be clear, I didn't meant to rename consensus/versionbits.o to versionbits.o, I was just talking about the "consensus package" in the makefile (because otherwise the whole package would depend on chain.o and its dependencies). I'm sorry for the misunderstanding and I understand if you don't want to change it back again.

btcdrak · 2016-03-04T06:19:11Z

src/main.cpp

+                strMiscWarning = _("Warning: unknown softfork has activated");
+                CAlert::Notify(strMiscWarning, true);
+            } else {
+                LogPrintf("Warning: An unknown softfork is about to activate");


missing a trailing \n

sipa · 2016-03-04T20:10:06Z

Fixed a few bugs:

typo in the warning logic which made it trigger all the time
newline at the end of unknown softfork warning log message
bip9_softforks was not being added to the getblockchaininfo output

paveljanik · 2016-03-04T20:17:46Z

src/consensus/params.h

@@ -22,6 +43,14 @@ struct Params {
    /** Block height and hash at which BIP34 becomes active */
    int BIP34Height;
    uint256 BIP34Hash;
+    /**
+     * Minimum blocks including miner confirmation of the total of 2016 blocks in a retargetting period,
+     * (nPowTargetTimespan / nPowTargetSpacing) wich is also used for BIP9 deployments.


wich -> which

sdaftuar · 2016-03-15T13:42:42Z

src/main.cpp

+                }
+            }
+        }
+        int32_t nExpectedVersion = ComputeBlockVersion(pindex->pprev, chainParams.GetConsensus());


(Sorry if this is a repost, I think github ate my first attempt to comment)

I think we should move this line into the for loop below; otherwise we'd generate a warning after a known soft-fork activates.

Inspired by former implementations by Eric Lombrozo and Rusty Russell, and based on code by Jorge Timon.

sipa · 2016-03-15T15:55:56Z

All comments addressed, I think.

btcdrak · 2016-03-15T18:05:07Z

Tested ACK 532cbb2

sdaftuar · 2016-03-15T20:10:43Z

ACK

If anyone is interested I also tested the warning logic with an RPC test (sdaftuar@4c2e8ba)

sipa · 2016-03-16T14:10:29Z

Included @sdaftuar's warning logic RPC test.

jtimon · 2016-03-16T15:20:04Z

src/rpc/blockchain.cpp

@@ -604,6 +604,20 @@ static UniValue SoftForkDesc(const std::string &name, int version, CBlockIndex*
    return rv;
 }

+static UniValue BIP9SoftForkDesc(const std::string& name, const Consensus::Params& consensusParams, Consensus::DeploymentPos id)


Shouldn't this loop throught all the deployments defined in Consensus::Params and be called from somewhere?
Style: why is not indented?

It's called once for every deployment below, similar to how SoftForkMajorityDesc is. There just are no deployments just yet.

Making this function iterate through all deployments would require putting knowledge of the names of all deployments in the consensus params, and would make it inevitable that the test deployment is listed.

Style: what do you mean by not indented?

Mhmm, I can only find one instance of the string "BIP9SoftForkDesc" in the changes.

re itarate with knowledge of the names, what's wrong with

for (int i = 0; i < MAX_VERSION_BITS_DEPLOYMENTS; ++i)

?

You don't know what string name to give to the Object entry inside "bip9_softforks".

Yes you can't find any calls to it yet, because there are no BIP9 softforks defined. Future deployments using BIP9 are supposed to be added below, just like ISM softforks were supposed to be added below to "softforks".

Style, oh, never mind: https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L27

I see, I didn't thought about the name.

jtimon · 2016-03-16T15:26:15Z

re-utACK 8c74ced ~~modulo last nit~~.

morcos · 2016-03-16T19:06:56Z

Code review ACK

btcdrak · 2016-03-16T20:18:11Z

Tested ACK 8c74ced

morcos · 2016-03-16T21:14:26Z

@sipa I realize you might still be changing this, but in the event that you're not, I backported it to 0.11 along with the soft fork for BIP 68,112,113 here:
https://github.com/bitcoin/bitcoin/compare/0.11...morcos:backportBIP9SoftFork?expand=1

The RPC tests are annoying to backport, so I left 2 of them out, but tested using the recent RPC test bip68-112-113.py against the 0.11 binary.

sipa · 2016-03-16T23:35:01Z

@morcos I'm fine with the current state.

jonasschnelli · 2016-03-17T19:07:48Z

utACK

8c74ced RPC test for BIP9 warning logic (Suhas Daftuar) 7870deb Test versionbits deployments (Suhas Daftuar) 532cbb2 Add testing of ComputeBlockVersion (Suhas Daftuar) d23f6c6 Softfork status report in RPC (Pieter Wuille) 732e774 Versionbits tests (Pieter Wuille) 6851107 BIP9 Implementation (Pieter Wuille)

petertodd · 2016-03-24T14:41:34Z

src/consensus/params.h


 namespace Consensus {
+
+enum DeploymentPos


Why is this called "DeploymentPos"? It's not a bit position; the actual bit positions have another layer of indirection in the BIP9Deployment structure: https://github.com/sipa/bitcoin/blob/8c74cedef53ab791ed333f25794f8b9d2e9f51aa/src/consensus/params.h#L26

We should change this to DeploymentId

'position' is a very ambigious, yes. It's almost never a good choice.
Made a similar comment here on another pull: #7541 (comment)
Agree 'Id' seems be a better fit here.

Alright, I'll do a pull-req to fix that - but next week after the long weekend!

It is the position in Consensus::Params::vDeployments. Id could have been another option, but O disagree there's anything to fix here. Seems too late for bike-shedding this.

Adding a comment for doxygen would also help if the purpose is not immediately clear from the code alone.

I had the same confusion as @petertodd when initially reading the code, so for new developers it must be worse. I do think at least a comment should be added.

ACK on adding documentation.

Network upgrade activation mechanism Implements ZIP 200. Integration with `CChainParams` inspired by bitcoin/bitcoin#7575. Includes block index rewinding logic cherry-picked from bitcoin/bitcoin#8149. Closes #2286. Part of #2905.

jonasschnelli added the Consensus label Feb 22, 2016

kleetus mentioned this pull request Feb 23, 2016

WIP Block size adaptively calculated bitpay/bitcoin#5

Merged

sipa force-pushed the bip9 branch from 50278bb to 99f6632 Compare February 24, 2016 21:32

jtimon reviewed Feb 25, 2016
View reviewed changes

jtimon mentioned this pull request Mar 1, 2016

Support building without libevent #7339

Closed

sipa force-pushed the bip9 branch from 99f6632 to 2949261 Compare March 3, 2016 00:33

sipa force-pushed the bip9 branch from 2949261 to 7662816 Compare March 3, 2016 22:26

btcdrak reviewed Mar 4, 2016
View reviewed changes

sipa force-pushed the bip9 branch from 7662816 to 8cbed80 Compare March 4, 2016 20:08

paveljanik reviewed Mar 4, 2016
View reviewed changes

sdaftuar reviewed Mar 15, 2016
View reviewed changes

sipa and others added 5 commits March 15, 2016 16:54

BIP9 Implementation

6851107

Inspired by former implementations by Eric Lombrozo and Rusty Russell, and based on code by Jorge Timon.

Versionbits tests

732e774

Softfork status report in RPC

d23f6c6

Add testing of ComputeBlockVersion

532cbb2

Test versionbits deployments

7870deb

sipa force-pushed the bip9 branch from 293e928 to 7870deb Compare March 15, 2016 15:55

RPC test for BIP9 warning logic

8c74ced

jtimon reviewed Mar 16, 2016
View reviewed changes

This was referenced Mar 16, 2016

Cache CBlockIndex::GetMedianTimePast #7650

Closed

BIP9: versionbits #6816

Closed

Softforks unit #6747

Closed

laanwj merged commit 8c74ced into bitcoin:master Mar 18, 2016

btcdrak mentioned this pull request Mar 18, 2016

[0.12] Backport BIP9, BIP68 and BIP112 with softfork #7543

Merged

petertodd reviewed Mar 24, 2016
View reviewed changes

str4d mentioned this pull request Jan 29, 2018

Network upgrade activation mechanism zcash/zcash#2898

Merged

ajtowns mentioned this pull request Apr 11, 2021

Speedy trial support for versionbits #21377

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Minimal BIP9 implementation #7575

Minimal BIP9 implementation #7575

Conversation

sipa commented Feb 21, 2016

btcdrak commented Feb 22, 2016

dcousens commented Feb 22, 2016

btcdrak commented Feb 22, 2016

sipa commented Feb 22, 2016 via email

jonasschnelli commented Feb 22, 2016

sipa commented Feb 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa Feb 25, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Mar 3, 2016

sipa commented Mar 3, 2016

jtimon commented Mar 4, 2016

Choose a reason for hiding this comment

sipa commented Mar 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Mar 15, 2016

btcdrak commented Mar 15, 2016

sdaftuar commented Mar 15, 2016

sipa commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Mar 16, 2016

morcos commented Mar 16, 2016

btcdrak commented Mar 16, 2016

morcos commented Mar 16, 2016

sipa commented Mar 16, 2016

jonasschnelli commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment