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.
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.
65+
66+ # Mine 1 new-version blocks
67+ self.nodes[2].generate(1)
68+ self.sync_all()
69+ if (self.nodes[0].getblockcount() != cnt + 1051):
70+ raise AssertionError("Failed to mine a version=3 block")
38+ self.nodes[2].generate(50)
39+ self.sync_all()
40+ if (self.nodes[0].getblockcount() != cnt + 850):
41+ raise AssertionError("Failed to mine 750 version=4 blocks")
42+
43+ # TODO: check that new CHECKLOCKTIMEVERIFY rules are not enforced
0@@ -0,0 +1,175 @@
1+#!/usr/bin/env python2
2+#
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.
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 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.
1854 flags |= SCRIPT_VERIFY_DERSIG;
1855 }
1856
1857+ // Start enforcing CHECKLOCKTIMEVERIFY, (BIP65) for block.nVersion=4
1858+ // blocks, when 75% of the network has upgraded:
1859+ if (block.nVersion >= 4 && IsSuperMajority(4, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {
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%
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)
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
@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.
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).
Slush has found the first version 0x20000007 block on the mainnet. So there are only two options left for BIP65: 1. adopt the “version bit” BIP; 2. Use IsSuperMajority with vserion>0x20000007
Otherwise, there may be a premature birth of BIP65
In this case, I don’t support IsSuperMajority as it will permanently invalidate a large range of nVersion
FWIW currently BIP101/XT’s nVersion bits aren’t a concern to me, as the adoption is low enough to not trigger accidentally. Also, @gavinandresen confirmed w/ other devs recently that they’ll add CLTV support to XT.
In terms of deployment, this IsSuperMajority() deployment mechanism is well tested and works, and there’s no compatibility issue with it and a future nVersion bits deployment. (or subsequent IsSuperMajority() soft-forks) The only issue is if CLTV fails to gain acceptance, which right now I think is very unlikely.
So, ACK merge to get CLTV deployment on-track and in-progess.
@btcdrak So basically IMO the advantage of that approach is it results in fewer overall # of soft-forks. Personally I’m kinda “meh” on that, but it’s a reasonable position to hold.
Re: my schedule, I’m going to try to spend a day or two next week with the other locktime soft-forks and do a code review.
What I am a rather dubious about, is waiting until nVersion bits is finished, given that code hasn’t even begun to be written yet - there’s enough consensus for CLTV certainly to do another IsSuperMajority() soft-fork and get it there.
The only outstanding question on the other LT bips is whether there is sufficient justification to save some extra soft-fork bits in the sequence field, which has a somewhat messy solution. I’m available to do what is necessary to get these merged.
Oh, and ACK on merging this, version bits or no, but like @btcdrak would prefer the 4 lock time bips were done together.
@petertodd I’m definitely not suggesting to wait for Versionbits proposal. If it’s ready before the 0.12 release then of course we should use it, but otherwise we should deploy using IsSuperMajority().
Ideally we deploy CLTV+CSV(etc), unless they are not ready then we just go with CLTV. Until then, I would defer on merging this.
Ideally we deploy CLTV+CSV(etc), unless they are not ready then we just go with CLTV. Until then, I would defer on merging this.
ACK, but I’d prefer we had a hard time line on this, say, if CSV isn’t ready by the end of the month, then just push forward with CLTV alone?
@petertodd I’m definitely not suggesting to wait for Versionbits proposal. If it’s ready before the 0.12 release then of course we should use it, but otherwise we should deploy using IsSuperMajority().
Ideally we deploy CLTV+CSV(etc), unless they are not ready then we just go with CLTV. Until then, I would defer on merging this.
ACK this plan.
Looks good to me /. utACK
Nit: getblockchaininfo RPC in rpcblockchain.cpp needs to be updated to report this softfork as well, This is a trivial code change, but will allow users to see how far the fork is along.
NACK as-is. The optics, communication difficulties, and extra work of pulling off multiple lock-time related soft-forks are not being properly considered.
#6312 was proposed three months ago and has seen a good deal of review. The much simpler and less controversial #6564 and #6566 have been open for a month. Together these constitute a complete upgrade to the behavior of lock-time in bitcoin. Rolling out CLTV first will cause head-aches. In particular #6566 changes how lock-time validity is determined. Having to go back and fix infrastructure written against the current behavior if/when #6566 is rolled out will be a giant waste of everyone’s time.
There was a plan discussed on the mailing list, this very issue, and the IRC development meeting a week ago to get reviews of the above mentioned PR’s and do a point release just prior to 0.12. What happened to that plan?
@maaku I thought we already had a plan:
I believe we still have time to make all these 3 things for 0.12, I hope I’m not too optimistic. But if we are not able to do so, I don’t think BIP65 should wait more. I know softforks/hardforks don’t need to coincide with major releases, but still, I don’t see the problem with rolling the 2 changes in 2 different softforks if necessary. You mention that #6566 changes how lock time validity is determined. Maybe some of that can be incorporated to BIP65 to safe the later head-aches?
@maaku I have looked at it. Like I say, whether or not CLTV is implemented has no impact on it.
Remember the only thing this pull-req does is enforce CLTV during block validation - code that already exists in git HEAD. CSV is a separate issue. For that matter, the unit tests for CLTV don’t even change with any of these proposed changes!
I’m not sure that #6747 is safe to be honest… It moves a lot of code around in ways that aren’t obviously correct. (by the standards of consensus critical code that is)
To review this pull-req, I’d suggest you follow the instructions in the commit message and verify that it’s identical to the BIP66 pull-req; if you find any issues the BIP66 soft-fork would be similarly vulnerable.
On 7 October 2015 14:15:10 CEST, Eric Lombrozo notifications@github.com wrote:
I would like to merge #6747 first as it will make all soft fork deployments easier to review. I’d be happy to rebase this PR to #6747 myself. Otherwise ACK.
Reply to this email directly or view it on GitHub: #6351 (comment)
I’m not sure that #6747 is safe to be honest… It moves a lot of code around in ways that aren’t obviously correct. (by the standards of consensus critical code that is)
I checked consensus is maintained in #6747 by doing a full blockchain sync from the network on top of careful code review, but I’m not agreeing or disagreeing that #6747 needs to be part of this ISM roll-out.
@brcdrak Glad to hear! But again, its a big change… Anyway, I wrote the CLTV enable pull-req the way I did to make it maximally easy to review - if BIP66 was done right, this pull-req is done right.
On 7 October 2015 14:50:54 CEST, “฿tcDrak” notifications@github.com wrote:
I’m not sure that #6747 is safe to be honest… It moves a lot of code around in ways that aren’t obviously correct. (by the standards of consensus critical code that is)
I checked consensus is maintained in #6747 by doing a full blockchain sync from the network on top of careful code review.
Reply to this email directly or view it on GitHub: #6351 (comment)
It also has to reject everything the old code rejected.
How would that be tested?
@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.
Based on the earlier BIP66 soft-fork logic implemented by Pieter
Wuille's 5a47811da5158df763aa2fca09ce646ee0c51e7b
bip65-cltv.py is based on the earlier BIP66 soft-fork RPC test
implemented by Pieter Wuille's 819bcf9b9902319176cdb1d476cacfee9b3727ec
bip65-cltv-p2p.py is based on the earlier BIP66 P2P test by Suhas
Daftuar's d76412b068d95454732aa3def95decf35251759a
@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.
By making my test, I noticed something odd…
0// Now that we know we're comparing apples-to-apples, the
1// comparison is a simple numeric one.
2if(nLockTime > (long)txTo.LockTime)
3 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 :
0 if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
1 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
0// Now that we know we're comparing apples-to-apples, the
1// comparison is a simple numeric one.
2if(nLockTime > (long)txTo.LockTime - 1)
3 return false;
So the script “1000 OP_CLTV” only pass from block 1001.
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
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.
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:
- With “X OP_CLTV”, an output is spendable after block X.
No, with “X OP_CLTV” the output is spendable AT block X.
- 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 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.
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 !
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)