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

BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

Merged
merged 6 commits into from Mar 30, 2016

Conversation

Projects
None yet
@btcdrak
Member

btcdrak commented Mar 6, 2016

General

This PR softforks 3 lock-time related BIPs which are all implemented in master as mempool-only logic at the moment.

  1. BIP68 sequence locks for relative locktime;
  2. BIP112 CHECKSEQUENCEVERIFY;
  3. BIP113 Median Time Past.

Dependencies

  • BIP68 (sequence locks) independently enforces the same semantics BIP113 (MTP)
  • BIP112 (CSV) relies on BIP68

Relay policy for BIP113

BIP113 mempool-only policy was deployed with Bitcoin Core 0.11.2 at the same time as the BIP65 CLTV softfork so the policy is in wide use (at least 70% of nodes) as well as all the miners who upgraded to 0.11.2.

Relay policy for v2 transactions

BIP68/112 rely on v2 transactions. Currently only v1 transactions are relayed, so it is necessary to change the relay policy to allow v2. This will be done at the same time as softfork deployment and will have the net effect that once the softfork enforces, we can be pretty sure miners will mine v2 transactions.

At a later date once enough nodes upgrade and we're sure v2 transaction will be relayed efficiently, we can bump the default transaction version in core see #7562.

@btcdrak btcdrak force-pushed the btcdrak:vb_68_112_113_1 branch from dc0f10a Mar 15, 2016

@btcdrak btcdrak force-pushed the btcdrak:vb_68_112_113_1 branch 7 times, most recently Mar 15, 2016

btcdrak and others added some commits Feb 16, 2016

Policy: allow transaction version 2 relay policy.
This commit introduces a way to gracefully bump the default
transaction version in a two step process.
Add RPC test for BIP 68/112/113 soft fork.
This RPC test will test both the activation mechanism of the first versionbits soft fork as well as testing many code branches of the consensus logic for BIP's 68, 112, and 113.

@btcdrak btcdrak force-pushed the btcdrak:vb_68_112_113_1 branch to 19d73d5 Mar 18, 2016

@btcdrak

This comment has been minimized.

Member

btcdrak commented Mar 18, 2016

Rebase now that #7575 has been merged.

@afk11

This comment has been minimized.

Contributor

afk11 commented Mar 18, 2016

To aid review, here are the pull requests for each BIP:

#7184 - Sequence locks
#7524 - CSV
#6566 - Median time past

@morcos

This comment has been minimized.

Member

morcos commented Mar 18, 2016

ACK 19d73d5

@NicolasDorier

This comment has been minimized.

Member

NicolasDorier commented Mar 19, 2016

Can you add my commit, btcdrak#8 the current test does not cover exactly the activation part, I'm not comfortable with it.

My test can be used for testing segwit activation later also in few lines.

@NicolasDorier

This comment has been minimized.

Member

NicolasDorier commented Mar 21, 2016

tACK 71527a0

@instagibbs

This comment has been minimized.

Member

instagibbs commented Mar 22, 2016

utACK 71527a0

@jl2012

This comment has been minimized.

Member

jl2012 commented Mar 23, 2016

tACK 71527a0

@sipa

This comment has been minimized.

Member

sipa commented Mar 23, 2016

Concept ACK, utACK code changes; I only glanced over the test code

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 23, 2016

nit: The new RPC test is very noisy. Printing lines for passing tests is not necessary, at least by default, could be behind some verbose flag:

Test 1: PASS [143]
Test 2: PASS [287]
...
Test 124: PASS [582]
Test 125: PASS [582]
@morcos

This comment has been minimized.

Member

morcos commented Mar 23, 2016

@laanwj yeah, I noticed that too. Unfortunately, that's the underlying behavior of the ComparisonTestFramework that the RPC test is built off of. I agree it would be nice to make optional.

EDIT: I suppose it would be easy to just always surpress the PASS output, and still output on failures.

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 23, 2016

@btcdrak @morcos Sure, I'm fine with keeping it this way in this pull.

@afk11

This comment has been minimized.

Contributor

afk11 commented Mar 23, 2016

ACK

@CodeShark

This comment has been minimized.

Contributor

CodeShark commented Mar 25, 2016

ACK code changes, ran tests but did not review test code.

@@ -83,6 +83,7 @@
#Tests
testScripts = [
'bip68-112-113-p2p.py',

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 29, 2016

Member

Shouldn't this be an "extended test"?

This comment has been minimized.

@btcdrak

btcdrak Mar 29, 2016

Member

no, this is a softfork and certainly until there is activation, I want to test for regressions as a standard part of the CI process.

@@ -118,6 +119,7 @@
'p2p-versionbits-warning.py',
]
testScriptsExt = [
'bip9-softforks.py',

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 29, 2016

Member

IMO: bip9 itself is not a softfork and could therefore be in the "normal" testScripts array.

This comment has been minimized.

@btcdrak

btcdrak Mar 29, 2016

Member

It's testing the activation logic of bip9 softforks.

@paveljanik

This comment has been minimized.

Contributor

paveljanik commented Mar 29, 2016

ACK 71527a0

@@ -15,6 +15,7 @@ namespace Consensus {
enum DeploymentPos
{
DEPLOYMENT_TESTDUMMY,

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 29, 2016

Member

nit: probably to late now and kinda-unrelated to this PR, but instead of TESTDUMMY we could have used something like DEFAULT or DEFAULTVOID.

This comment has been minimized.

@btcdrak

btcdrak Mar 29, 2016

Member

Not added in PR though. That was #7575. I think the name is sufficient though, to indicate it's not a real softfork, but for testing purposes.

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 29, 2016

Member

Fair enough.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Mar 29, 2016

Tested ACK 71527a0

@jtimon

This comment has been minimized.

Member

jtimon commented Mar 29, 2016

utACK (although I didn't look at the rpc tests in depth ).

@sdaftuar

This comment has been minimized.

Member

sdaftuar commented Mar 29, 2016

I saw there was some discussion of this already (btcdrak#8), but I don't really see the point of the bip9-softforks.py test -- it seems like a subset of the testing done in bip68-112-113-p2p.py. I don't feel strongly if you want to include it here, in case it helps anyone's review, but I thought I'd mention that this seems like a candidate for removal from the repo in the future.

Also perhaps we should remove the code in bip68-sequence.py which tests that BIP68 is not consensus (

def test_bip68_not_consensus(self):
)? That test will continue to pass for the time being, because not enough blocks are generated to trigger the soft fork, but it's somewhat confusing to keep that test in the repo, and at any rate the RPC test bip68-112-113-p2p.py is comprehensively testing that BIP68 activates in the right way.

return row
raise IndexError ('key:"%s" not found' % key)
def create_bip68txs(self, bip68inputs, txversion, locktime_delta = 0):

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: locktime_delta appears to be unused in this function

# BIP 113 tests should now pass if the locktime is < MTP
bip113tx_v1.nLockTime = self.last_block_time - 600 * 5 - 1 # = MTP of prior block (not <) but < time put on current block
bip113signed1 = self.sign_transaction(self.nodes[0], bip113tx_v1)
bip113tx_v2.nLockTime = self.last_block_time - 600 * 5 - 1 # = MTP of prior block (not <) but < time put on current block

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: The comments at lines 386 and 388 appear to be incorrect. Perhaps # < MTP of prior block?

self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
## SEQUENCE_LOCKTIME_DISABLE_FLAG is unset in argument to OP_CSV for all remaining txs ##
# All txs with nSequence 11 should fail either due to earlier mismatch or failing the CSV check

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: I think this comment should be ... nSequence 9 ..., rather than nSequence 11?

for b25 in xrange(2):
for b22 in xrange(2):
for b18 in xrange(2):
fail_txs.append(bip112txs_vary_OP_CSV_9_v2[0][b25][b22][b18]) # 16/16 of vary_OP_CSV_9

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: I think this comment is off, should be # 8/16 of vary_OP_CSV_9?

for b25 in xrange(2):
for b18 in xrange(2):
fail_txs.append(bip112txs_vary_nSequence_v2[0][b25][1][b18]) # 12/16 of vary_nSequence
fail_txs.append(bip112txs_vary_OP_CSV_v2[0][b25][1][b18]) # 12/16 of vary_OP_CSV

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: I think these two comments should say # 4/16 ...

for b25 in xrange(2):
for b18 in xrange(2):
success_txs.append(bip112txs_vary_nSequence_v2[0][b25][0][b18]) # 16/16 of vary_nSequence
success_txs.append(bip112txs_vary_OP_CSV_v2[0][b25][0][b18]) # 16/16 of vary_OP_CSV

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: I think these two should also say # 4/16 ...

@NicolasDorier

This comment has been minimized.

Member

NicolasDorier commented Mar 29, 2016

@sdaftuar as explained in btcdrak#8 the test has a different purpose than bip68-112-113-p2p.py.

All bip are generally tested in their own PR, bip68-112-113-p2p.py is doing test that are already done but also testing the sf activation logic. This expand way more than the subject of this PR.

softforks.py has a different purpose, it is here to test ONLY the sf activation logic. This basically mean that in future softfork, you can test the activation correctly by adding a single line at https://github.com/bitcoin/bitcoin/pull/7648/files#diff-98a8abf7f80dbe5eda93bbbbb4348e80R190 .

For example, testing any new segwit sf activation will only be a matter of adding a function which change the scriptPubKey to be push.

The test is meant to make testing sf soft fork logic activation of future sf a breeze.

@btcdrak

This comment has been minimized.

Member

btcdrak commented Mar 29, 2016

bip68-sequence.py is a valid part of the mempool-only test suite and should remain in the same way we have bip65 mempool tests. In any case, it's out of scope for this PR.

assert_equal(self.get_bip9_status(bipName)['status'], 'locked_in')
# Test 5
# Check that the new rule is enforced

This comment has been minimized.

@sdaftuar

sdaftuar Mar 29, 2016

Member

nit: I think this comment should say # Check that the new rule is not enforced ?

@sdaftuar

This comment has been minimized.

Member

sdaftuar commented Mar 29, 2016

I left some comment nits in the RPC test, bip68-112-113-p2p.py, which I reviewed in depth in addition to the rest of the code. That test is pretty comprehensive, and I think that test coverage had been missing before, so thanks for including that in this pull.

I also manually tested the case that is mentioned as being missing from bip68-112-113-p2p.py, for an OP_CSV with an empty stack.

ACK 71527a0

@btcdrak To be clear, I'm not talking about removing the entire bip68-sequence.py test, just comment out the specific test within that one which is checking that BIP68 is not enforced as a consensus rule. That test made sense when we were deploying BIP68 as mempool-only, but doesn't make sense when we're proposing it as a soft fork. See the line of code I linked to above.

@Roasbeef

This comment has been minimized.

Roasbeef commented Mar 29, 2016

tACK 71527a0

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 30, 2016

it's out of scope for this PR.

Issues with the tests, unless they pinpoint issues in the code, or break Travis, should not hold up this pull. They can be fixed later.

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 30, 2016

@petertodd You said in #7184 (comment) that you wanted some nits addressed before this (the BIP68 part) would be acceptable as a softfork to you. Could you take a look whether this is the case now?

@petertodd

This comment has been minimized.

Contributor

petertodd commented Mar 30, 2016

@laanwj Those nits got fixed.

utACK 71527a0

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 30, 2016

utACK 71527a0

@laanwj laanwj merged commit 71527a0 into bitcoin:master Mar 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 30, 2016

Merge #7648: BIP9 versionbits softfork for BIP68, BIP112 and BIP113
71527a0 Test of BIP9 fork activation of mtp, csv, sequence_lock (NicolasDorier)
19d73d5 Add RPC test for BIP 68/112/113 soft fork. (Alex Morcos)
12c89c9 Policy: allow transaction version 2 relay policy. (BtcDrak)
02c2435 Soft fork logic for BIP68 (BtcDrak)
478fba6 Soft fork logic for BIP113 (BtcDrak)
65751a3 Add CHECKSEQUENCEVERIFY softfork through BIP9 (Pieter Wuille)

vlamer added a commit to vlamer/bitcoin that referenced this pull request Mar 30, 2016

@btcdrak btcdrak referenced this pull request Mar 30, 2016

Merged

Fix comments in tests #7773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment