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

CHECKLOCKTIMEVERIFY (BIP65) IsSuperMajority() soft-fork #6351

Merged
merged 3 commits into from Oct 23, 2015

Conversation

@petertodd
Copy link
Contributor

petertodd commented Jun 28, 2015

Final step towards CLTV deployment on mainnet.

I've copied the logic and tests from the previous BIP66 (DERSIG) soft-fork line-by-line for ease of review; any code review applicable to BIP66 should be applicable to BIP65.

Once merged I'll prepare a backport of the soft-fork logic for the v0.10.x branch as well.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 29, 2015

IMO bip65-cltv-p2p.py should be included in extended list in pull-tester/rpc-tests.sh. Adding it in the "normal" not extended list makes not much sense even if the script execution time is acceptable (<1min on a decent system).

Tested ACK.

@jtimon
Copy link
Member

jtimon commented Jun 29, 2015

I didn't reviewed the python tests, just the c++ part, but utACK on that part.

@btcdrak
Copy link
Contributor

btcdrak commented Jun 29, 2015

utACK

@petertodd petertodd force-pushed the petertodd:cltv-is-super-majority-soft-fork branch Jun 29, 2015
@petertodd
Copy link
Contributor Author

petertodd commented Jun 29, 2015

@jonasschnelli Added the tests to the extended list. I didn't actually know about the pull-tester test list before, thanks!

@mruddy
mruddy reviewed Jul 1, 2015
View changes
qa/rpc-tests/bip65-cltv.py Outdated
self.nodes[2].generate(1)
self.sync_all()
if (self.nodes[0].getblockcount() != cnt + 1051):
raise AssertionError("Failed to mine a version=3 block")

This comment has been minimized.

Copy link
@mruddy

mruddy Jul 1, 2015

Contributor

Line 70 should be: Failed to mine a version=4 block

This comment has been minimized.

Copy link
@petertodd

petertodd Jul 1, 2015

Author Contributor

Fixed.

if (self.nodes[0].getblockcount() != cnt + 850):
raise AssertionError("Failed to mine 750 version=4 blocks")

# TODO: check that new CHECKLOCKTIMEVERIFY rules are not enforced

This comment has been minimized.

Copy link
@mruddy

mruddy Jul 1, 2015

Contributor

In order to fill out the test for this "not enforced" TODO, you'll need to add a testable transaction to the 750th v4 block. As in your other script, you'd want to check that the new CLTV rules are not enforced on the 750th v4 block and that they are enforced on the 751st v4 block.

@@ -0,0 +1,175 @@
#!/usr/bin/env python2
#

This comment has been minimized.

Copy link
@mruddy

mruddy Jul 1, 2015

Contributor

minor nit, just noticed that the copyright header was different in this file than the other

@mruddy
Copy link
Contributor

mruddy commented Jul 1, 2015

Looks pretty good. Minor todo's with the python test scripts.

Needs some release notes, but you might have been waiting on those until later in the process. When you get to them, perhaps add a disclaimer that we may change the OP_NOP2 name decode to OP_CHECKLOCKTIMEVERIFY in script "asm" outputs in the future. That way we'll have the flexibility to make that change if/when we get to it.

@petertodd
Copy link
Contributor Author

petertodd commented Jul 1, 2015

So, @mruddy brought up two issues that are really the same as with the BIP66 tests: out of date copyright headers and TODO items that are out of date. The latter issue is pretty simple: bipdersig-p2p.py covers the missing test cases marked as "TODO" in bipdersig.py

I wanted to show I've replicated the exact same testing procedure done on BIP66 given that the code changes itself are also replicated exactly; if I'm going to fix these two nits I think it makes sense to fix them in the BIP66 tests first, in a separate commit that'll go first in this pull-req, so the two sets of tests can still be diffed directly to show exact equivalence. Makes sense?

@mruddy Yup, I'll add release notes later, and similarly, will look at what changing the NOP2 name will entail later. Right now I'm focused on making the minimal possible change that gets the job done so backporting the changes to v0.10.x (and probably v0.11.x) will be easy and clearly correct. v0.12.x can do the more invasive stuff of changing opcode names and what-not.

@petertodd petertodd force-pushed the petertodd:cltv-is-super-majority-soft-fork branch to 3082578 Jul 1, 2015
@mruddy
Copy link
Contributor

mruddy commented Jul 2, 2015

@petertodd I see what you're saying about trying to keep the diff compares as close and easy as possible. But, the diffs on the tests are necessarily going to be somewhat different. I wouldn't make updated BIP66 test cases part of this change set. My choice would be to make the minor cleanups to these tests so that they are cleaner when reviewed in isolation (and in the future, not within the context of how this change set compares to that of BIP66). I can see how line-only diffs vs some block or structural diffs might have some value to reduce dissonance though. So, however you want to handle it is OK with me.

About the decode name update, I agree, leave that until later. I was just saying that if you mention the upcoming change in the notes now, then it makes actually pushing it through later, easier. It's more of a busy work change anyways, so leave it for someone newer to the project than yourself to work on. It's good for someone newer to work on because of how it may affect various parts of the code base.

@laanwj laanwj added the Validation label Jul 3, 2015
@petertodd
Copy link
Contributor Author

petertodd commented Jul 4, 2015

@mruddy Thanks! It'd be awesome for you to do that, and it'd be a good opportunity to have more eyes on the code too of course.

Anyway, I'll see what the other reviewers think re: the tests.

@mruddy
Copy link
Contributor

mruddy commented Jul 4, 2015

I probably will, it's on my todo list.

@mruddy
Copy link
Contributor

mruddy commented Aug 2, 2015

What do we need to do now to move this forward? Is this waiting on coordination with some other work, in need of more testing, or just baking and giving time for more review (it's been about a month since last update)?

if (block.nVersion >= 3 && IsSuperMajority(3, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {
flags |= SCRIPT_VERIFY_DERSIG;
}

// Start enforcing CHECKLOCKTIMEVERIFY, (BIP65) for block.nVersion=4
// blocks, when 75% of the network has upgraded:
if (block.nVersion >= 4 && IsSuperMajority(4, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {

This comment has been minimized.

Copy link
@jtimon

jtimon Aug 4, 2015

Member

I still don't know why we wait for 75% to start using a policy rule.
I would do it from the start but only enforce it as a consensus rule after 95%.
I don't want to slow this down since it is a general softfork question (even if I'm right it can be solved after this PR), but I haven't been able to find an answer anywhere.

This comment has been minimized.

Copy link
@sipa

sipa Aug 4, 2015

Member

This is not a policy rule.

This comment has been minimized.

Copy link
@jtimon

jtimon Aug 4, 2015

Member

While individual miners apply it but still not enforce it as a consensus rule (ie reject blocks that don't apply it), it is just mining policy.
What am I missing?

This comment has been minimized.

Copy link
@jtimon

jtimon Oct 15, 2015

Member

Seriously, what am I missing here? Why wait for 75% for enforcing the new rule in your own blocks (which by the way you are marking as being version 4)?

This comment has been minimized.

Copy link
@petertodd

petertodd Oct 16, 2015

Author Contributor

This code is executed against all blocks, not just blocks you created. If another miner creates a nVersion=4 block with an invalid use of CHECKLOCKTIMEVERIFY if you reject it without a majority of hashing power also rejecting it you'll get forked.

In fact, it might be a good idea to go the other way and remove the 75% rule, accepting invalid nVersion=4 blocks until 95%

This comment has been minimized.

Copy link
@jtimon

jtimon Oct 16, 2015

Member

Thanks, for some reason I was fixated on this 75% threshold only affecting your own blocks, but this is ConnectBlock so it obviously affects all blocks. Yes, I wouldn't oppose to wait for the 95% here too. But with versionbits this disappears so is probably not worth to discuss this much.

This comment has been minimized.

Copy link
@petertodd

petertodd Oct 16, 2015

Author Contributor

Yeah, I probably would have changed it to remove 75% a year ago... but at this stage I think it's harmless to leave in.

@jtimon
Copy link
Member

jtimon commented Aug 4, 2015

@mruddy I would say that, as usual, we're just waiting for more review, but IMO the harder-to-review parts have already been merged. I would really like that we move forward on this.

@petertodd
Copy link
Contributor Author

petertodd commented Aug 4, 2015

I think what's holding this up is a lack of political will to do yet another non-nVersion bits soft-fork upgrade; @gmaxwell has indicated a strong preference for never using IsSuperMajority() again.

On 4 August 2015 07:04:50 GMT-04:00, "Jorge Timón" notifications@github.com wrote:

@mruddy I would say that, as usual, we're just waiting for more review,
but IMO the harder-to-review parts have already been merged. I would
really like that we move forward on this.


Reply to this email directly or view it on GitHub:
#6351 (comment)

@btcdrak
Copy link
Contributor

btcdrak commented Aug 4, 2015

It would seem a better plan to deploy this softfork now rather than waiting for nVersionbits code which isnt done yet. We know this method works and it doesnt get in the way of nVersionbits deployment later. Many players in the ecosystem are waiting for CLTV, I dont see why we should wait for perfection when we can deploy a much anticipated feature now. @sipa @gmaxwell @laanwj

@jl2012
Copy link
Contributor

jl2012 commented Aug 4, 2015

Do we have any softfork ready to deploy in the coming 4 months? If no I don't think we need to wait for nVerionbits. I guess BIP62 is not ready yet.

Is there any plan to prevent a fork due to reckless SPV mining?

By the way, if BIP101 is implemented in Bitcoin XT, blocks will have a version number with bits 1,2,3 and 14 set (0x20000007 in hex). In that case, it is not possible to support BIP101 without also supporting BIP65. @gavinandresen @mikehearn

@btcdrak
Copy link
Contributor

btcdrak commented Aug 4, 2015

Just for the record, @sipa ACK'd for IsSuperMajority() on the ML

@dcousens
Copy link
Contributor

dcousens commented Aug 5, 2015

@gmaxwell has indicated a strong preference for never using IsSuperMajority() again.

@gmaxwell can you confirm this?

@maaku
Copy link
Contributor

maaku commented Aug 5, 2015

@dcousens this pull request shouldn't be held up waiting for a response from someone who has never participated in it.

@jl2012 there is BIP68, and the as-yet unnumbered CHECKSEQUENCEVERIFY and median-lock-time-past BIPs, all of which are related to CHECKTIMELOCKVERIFY and could be rolled out together...

@jl2012
Copy link
Contributor

jl2012 commented Aug 6, 2015

@maaku I think we could go with IsSuperMajority(). If BIP68 or other related BIPs are ready before BIP65 is completed, they could be considered as "addon" of BIP65. People will use nVersion=5 to support BIP65 and those addon.

@btcdrak
Copy link
Contributor

btcdrak commented Aug 6, 2015

@maaku I was thinking the same thing recently that rolling out CLTV and CSV opcodes as one upgrade would be ideal and most efficient but when are you planning to make the PR for CSV? Seems like the code is there in your fork...

If this is going to take you a while then in my opinion we should not hold up CLTV, but I think it would be a wasted opportunity not to bundle the features you list as one soft-fork.

@maaku
Copy link
Contributor

maaku commented Aug 6, 2015

The code is done, ready, tested, deployed to alpha testnet and known to
work.

What remains to be done is writing a BIP... Assistance is welcome if
someone wants to help speed this along. I'm being pulled in 10 different
directions and having trouble allocating the time necessary.

On Thu, Aug 6, 2015 at 3:42 PM, ฿tcDrak notifications@github.com wrote:

@maaku https://github.com/maaku I was thinking the same thing recently
that rolling out CLTV and CSV opcodes as one upgrade would be ideal and
most efficient but when are you planning to make the PR for CSV? Seems like
the code is there in your fork...

If this is going to take you a while then in my opinion we should not hold
up CLTV, but I think it would be a wasted opportunity not to bundle the
features you list as one soft-fork.


Reply to this email directly or view it on GitHub
#6351 (comment).

@jtimon
Copy link
Member

jtimon commented Aug 6, 2015

+1 to @btcdrak 's thoughts.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 8, 2015

Concept ACK and +1 to @btcdrak. I'd suggest holding off merging this until either CSV is ready or a week or two before a new release is going to freeze.

@sipa
Copy link
Member

sipa commented Oct 7, 2015

@dcousens
Copy link
Contributor

dcousens commented Oct 7, 2015

It also has to reject everything the old code rejected.

How would that be tested?

@petertodd
Copy link
Contributor Author

petertodd commented Oct 7, 2015

@dcousens With great difficulty.... That's one of the biggest challenges in changing consensus critical code.

@CodeShark
Copy link
Contributor

CodeShark commented Oct 7, 2015

@petertodd I created a new PR #6774 and rebased #6747 atop it to make the relevant code changes as explicit as possible.I also added the mechanism for your PR to it without activating it yet so that we're ready to go with BIP65.

In any case, I don't want this to hold up deploying BIP65 and I understand you patterned it after BIP66. My intention was to make it even easier to compare the two (or any other soft fork deployments we make in the future, for that matter).

Consensus-critical code does indeed present challenges, but I hope the relatively few code movements in these PRs are very simple to follow, now. Checking its correctness should basically amount to ensuring there were no copy/paste errors.

I ran the comparison tool locally and it passed. It also passed Travis' barrage of tests.

petertodd added 2 commits Jun 28, 2015
Based on the earlier BIP66 soft-fork logic implemented by Pieter
Wuille's 5a47811
bip65-cltv.py is based on the earlier BIP66 soft-fork RPC test
implemented by Pieter Wuille's 819bcf9

bip65-cltv-p2p.py is based on the earlier BIP66 P2P test by Suhas
Daftuar's d76412b
@petertodd petertodd force-pushed the petertodd:cltv-is-super-majority-soft-fork branch from 3082578 Oct 8, 2015
@petertodd petertodd force-pushed the petertodd:cltv-is-super-majority-soft-fork branch to 65ef372 Oct 8, 2015
@petertodd
Copy link
Contributor Author

petertodd commented Oct 8, 2015

Rebased and added BIP65 to getblockchaininfo softforks list.

@CodeShark
Copy link
Contributor

CodeShark commented Oct 9, 2015

@petertodd I created a branch demonstrating how we will deploy soft forks using #6747. The deployment logic for BIP65 can be conveniently compared side-by-side with that for BIP66, and future soft forks will also be possible to compare in this fashion regardless of whether we use IsSuperMajority or versionbits.

grep for "DEPLOY BIP65" to see the the hooks.

@btcdrak
Copy link
Contributor

btcdrak commented Oct 22, 2015

ACK

@laanwj laanwj merged commit 65ef372 into bitcoin:master Oct 23, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Oct 23, 2015
65ef372 Add BIP65 to getblockchaininfo softforks list (Peter Todd)
cde7ab2 Add RPC tests for the CHECKLOCKTIMEVERIFY (BIP65) soft-fork (Peter Todd)
287f54f Add CHECKLOCKTIMEVERIFY (BIP65) soft-fork logic (Peter Todd)
NicolasDorier added a commit to MetacoSA/NBitcoin that referenced this pull request Oct 24, 2015
@NicolasDorier
Copy link
Contributor

NicolasDorier commented Oct 24, 2015

By making my test, I noticed something odd...

// Now that we know we're comparing apples-to-apples, the
// comparison is a simple numeric one.
if(nLockTime > (long)txTo.LockTime)
    return false;

This means that if nLockTime == txTo.LockTime, then the script evaluate to true, which made me believe that I can spend "1000 OP_CLTV" from exactly block 1000.

However, such a transaction would not be final as IsFinalTx show :

    if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
        return true;

We can see that if such transaction is propagated at block 1000 it will be rejected as NonFinal !
This mean that 1000 OP_CLTV can only be spent from block 1001, even if the script pass at block 1000.

Is this the expected behavior ? This seems kind of wierd...
I would have changed to

// Now that we know we're comparing apples-to-apples, the
// comparison is a simple numeric one.
if(nLockTime > (long)txTo.LockTime - 1)
    return false;

So the script "1000 OP_CLTV" only pass from block 1001.

@jl2012
Copy link
Contributor

jl2012 commented Oct 24, 2015

@NicolasDorier

So the script "1000 OP_CLTV" only pass from block 1001.

It is consistent with the meaning of nLockTime, i.e. a tx is final only after (not at or before) the nLockTime. "1000 OP_CLTV" means the output is spendable after block 1000.

1000 OP_CLTV can only be spent from block 1001, even if the script pass at block 1000.

It is always possible to create a valid script at block X, but is not final at block X. For example if the current height is 380000, a script of "390000 OP_CLTV" with nLockTime = 391000 is still valid but not final

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Oct 24, 2015

It is consistent with the meaning of nLockTime, i.e. a tx is final only after (not at or before) the nLockTime. "1000 OP_CLTV" means the output is spendable after block 1000.

This is what is not consistent ! "1000 OP_CLTV" is currently true if nLockTime is 1000. (1000 included)

Whereas a nLocktime of 1000 at block 1000 is not final as you rightly said " tx is final only after (not at or before)"

A consistent behavior would be "1000 OP_CLTV" false with a nLockTime of 1000.

@jl2012
Copy link
Contributor

jl2012 commented Oct 24, 2015

A consistent behavior would be "1000 OP_CLTV" false with a nLockTime of 1000.

In that case you need a nLockTime of 1001 to spend "1000 OP_CLTV", which means unspendable until block 1002.

Anyway, by "consistent" I mean:

  1. With "X OP_CLTV", an output is spendable after block X.
  2. With nLockTime = X, the tx is valid after block X
@NicolasDorier
Copy link
Contributor

NicolasDorier commented Oct 25, 2015

  1. With "X OP_CLTV", an output is spendable after block X.

No, with "X OP_CLTV" the output is spendable AT block X.

  1. With nLockTime = X, the tx is valid after block X

Yes, in other words nLockTime=X the tx is valid at block X+1.

This is the inconsistency.
Because of 2., it means that X OP_CLTV is spendable at X+1 even if the script returns true at X. This is what is not very coherent.

@gmaxwell
Copy link
Contributor

gmaxwell commented Oct 25, 2015

@NicolasDorier it sounds like you are misunderstanding the design of OP_CLTV. There is no "return true" in it. It is a VERIFY op_code. It checks that the nlocktime field in the transaction agrees with the value on the stack, if it does-- it does nothing. If it does not, it terminates execution. It has no clue about the height of the blockchain, and if it did it would make script execution no long a pure function of the transaction, violate the layering between transactions and the blockchain, make it possible for a script to become invalid that was valid before, etc.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Oct 25, 2015

@gmaxwell I understand that.
What I am saying is that from a script perpective "X OP_CLTV" means "can spend AT and AFTER block X".

While nLockTime=X means "can broadcast AT and AFTER block X+1".

Both of these means that "X OP_CLTV" will always been able to me spent at X+1, even if the X OP_CLTV will pass with nLockTime of X.

Well, I don't mind though, if it is the expected behavior, I have nothing to complain, sounds weird for me but this seems to be just a personal perspective rather than a mistake in the code. So I'm fine with it. I was just worried it was a mistake.

@gmaxwell
Copy link
Contributor

gmaxwell commented Oct 25, 2015

@NicolasDorier CLTV has nothing to do with blocks, X OP_CLTV means is "this transaction is invalid if X > the transaction's nLocktime field". What effect the nlocktime field has is largely a mystery to OP_CLTV.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Oct 25, 2015

Ok now by thinking about it like you said,
If I want to say "Can spend this output at block X" then I need to use :
"X-1 OP_CTLV" and
at least nLockTime = X-1 when spending which is coherent.

I was indeed making things more complicated than they needed to be, I'm fine now !

@petertodd
Copy link
Contributor Author

petertodd commented Oct 25, 2015

Great!

If you think there's a way to better explain CLTV in the BIP, pull-reqs welcome.

On 25 October 2015 12:24:41 GMT-04:00, Nicolas Dorier notifications@github.com wrote:

Ok now by thinking about it like you said,
If I want to say "Can spend this output at block X" then I need to use
:
"X-1 OP_CTLV" and
at least nLockTime = X-1 when spending which is coherent indeed.

I was indeed making things more complicated than they needed to be, I'm
fine now !


Reply to this email directly or view it on GitHub:
#6351 (comment)

bokobza pushed a commit to bokobza/StratisBitcoinFullNode that referenced this pull request Nov 5, 2017
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

You can’t perform that action at this time.