Same as #5496, but with without the "type" parameter. In this pull-req, a future relative CLTV or other new modes would need to be implemented with another opcode. (e.g. the original proposal)
I personally have a preference for this version, as we're in no danger of running out of NOPs.
While the existing numeric opcodes are all limited to 4-byte bignum
arguments, new opcodes will need different limits.
Will now be needed by CHECKLOCKTIMEVERIFY code.
As stated, I prefer this one, it is smaller code, has been more tested, save one byte.
My aesthetic argument is that we will get nicer human readable script. (< locktime > OP_CLTV and < push > OP_RCLTV, instead of cryptic < locktime > < mode > OP_CLTV )
Nit: I think CheckLockTime's full implementation would conceptually fit better implemented as part of BaseSignatureChecker (and it would remove a few lines of code), because the purpose of the BaseSignatureChecker/TransactionSignatureChecker split is just to have a checker that skips expensive ECDSA validation. Checking the locktime is fast, and it feels safer to have BaseSignatureChecker do the check.
In practice, the above nit doesn't matter, the only place BaseSignatureChecker is used at the moment is checking for non-standard scriptSigs.
Besides that, code review ACK.
BaseSignatureChecker does not know that the data being signed is a transaction.
362 | + // themselves is uint32 which only becomes meaningless 363 | + // after the year 2106. 364 | + // 365 | + // Thus as a special case we tell CScriptNum to accept up 366 | + // to 5-byte bignums, which are good until 2**32-1, the 367 | + // same limit as the nLockTime field itself.
This is not clear: 5-byte CScriptNum should be good up to 2**39-1, beyond the limit of the nLockTime field.
Fixed.
Yes, the whole point of BaseSignatureChecker is that EvalScript doesn't need to know anything about transactions. Untested ACK
any plan to merge that for 0.11 ? I know I can play with it with viacoin/testnet with mempool rule, but I think there is no point in delaying it anymore since there is no controversial problems anymore with it.
I don't see any reason not to do it either. maaku prefers the other version but I don't think he is opposed to this one, which seems to be preferred by almost everyone else.
utACK
75 | @@ -76,6 +76,10 @@ enum 76 | // (softfork safe, BIP62 rule 6) 77 | // Note: CLEANSTACK should never be used without P2SH. 78 | SCRIPT_VERIFY_CLEANSTACK = (1U << 8), 79 | + 80 | + // Verify CHECKLOCKTIMEVERIFY (BIP65) 81 | + //
Stupidest little nit... this empty comment line can be removed, no? Or better yet explain the feature like the other comments do.
My objections about the unparametrized version should not be interpreted as release blocking. I prefer the original form for a couple of reasons.
Tested ACK. I thoroughly reviewed and tested this code as part of the Project Elements release.
<nLockTime> CHECKLOCKTIMEVERIFY -> <nLockTime>
Fails if tx.nLockTime < nLockTime, allowing the funds in a txout to be
locked until some block height or block time in the future is reached.
Only the logic and unittests are implemented; this commit does not have
any actual soft-fork logic in it.
Thanks to Pieter Wuille for rebase.
Credit goes to Gregory Maxwell for the suggestion of comparing the
argument against the transaction nLockTime rather than the current
time/blockheight directly.
Transactions that fail CLTV verification will be rejected from the
mempool, making it easy to test the feature. However blocks containing
"invalid" CLTV-using transactions will still be accepted; this is *not*
the soft-fork required to actually enable CLTV for production use.
code review ACK
Are there any remaining blockers to merging this?
@jgarzik Nothing I can think of.
If for some reason we find we totally screwed up plan is to just change the behavior, use a new NOP, and go from there. (as was discussed a few months ago with no objections: #5496 (comment))
code review ACK.
Will test and write some example RPC regtests within the next days.
@jonasschnelli RPC regtests? What would those regtests actually do?
I do have very simple demo's of CLTV here: https://github.com/petertodd/checklocktimeverify-demos
@petertodd: some simple CLTV real-world examples. Creating some CLTV transactions (needs hex fiddling because the wallet doesn't support it right now), broadcast, mine some blocks, see what's in there. Like the other scripts in (/qa/rpc-tests). This is a way to test a feature while also creating a test script that help to sustain quality.
@jonasschnelli Hmm, I suspect the actual testing value of doing that is relatively low, at least until the soft-fork code goes in; the actual opcode itself is very well tested in the script tests, and there's no IsStandard() changes associated. (yet) I personally would add that kind of thing to cltv-demos because I greatly prefer the python library it uses. That said, I certainly have no objection to doing that - and it might come in handy when we do add block tests for the soft-fork code - although I'd ask that we don't wait on it before merging this pull-req.
Sure. I didn't mention the rpc test to hold this PR back. Merging is totally fine for me.
What about changing script.cppL144 from
case OP_NOP2 : return "OP_NOP2";
to
case OP_NOP2 : return "OP_CHECKLOCKTIMEVERIFY";
or
case OP_CHECKLOCKTIMEVERIFY : return "OP_CHECKLOCKTIMEVERIFY";
@jonasschnelli Good point.
Anyone have any idea what doing that will break? :/
@petertodd the tests...
I came up with this because it's exposed in the RPC call decoderawtransaction (scriptPubKey['asm']).
Changing this would probably only harm users who rely on OP_NOP2 (which is not in use) in this field. If someone uses the asm field and parse the content he needs to get bitchslapped anyway.
Nice work on this @petertodd. It'll be very useful once it's rolled out!
I ran some CLTV style micropayment channel deposit and refund tests on testnet.
Overall, the changes look good.
I'm wondering why you chose to make OP_CHECKLOCKTIMEVERIFY not pop the input argument off the top of the stack in the success case?
In the success case, by not popping, the semantic of the original no-op definition of OP_NOP2 is preserved. I assume that maintaining the no-op semantic for old clients, so that they could still validate scripts using the new semantic of the opcode, was your reason. If so, did you have any other reason, like you were also trying to save an upgradable nopcode by not using one for a matching non-verify OP_CHECKLOCKTIME (to fit the pattern of OP_CHECKSIG+OP_CHECKSIGVERIFY, OP_EQUAL+OP_EQUALVERIFY, etc...)?
During the transition, I don't think preserving the no-op semantic in this case is desirable for the same reason that it is not desirable to start using CLTV outputs prior to the 95% lock-in during a soft fork -- the refund path in CLTV scripts could be interpreted as no-op's on non-time-locked transactions to claim the refund outputs early.
If backwards compatibility for older clients was the only reason, then I think that changing to popping the input stack argument should be considered because:
Make sense?
Here's a table of the combinations of test cases that I tried. The rows represent the three vintages of clients, and the columns are the two possible intended semantics of the OP_NOP2 opcode within scripts. The cells are outcomes with the current implementation of this PR (not popping the top stack item):
| script expected no-op semantic | script expected cltv semantic | |
|---|---|---|
| pre-SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS client (NOP2 execution treated as NOP) | script validates | script validates (seems problematic pre-lock-in) |
| SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS client (NOP2 execution fails) | script fails (NOPx reserved) | script fails (NOPx reserved) |
| CLTV aware client (NOP2 execution treated as CLTV) | script usually fails* | script validates |
*Note: Failures are caused by trying to get a CScriptNum from the top of the stack and by checks within TransactionSignatureChecker::CheckLockTime. Outcomes are script dependent, but for the cases I tried, always failed.
If you pop the argument off the stack it becomes a hard fork change. On Jun 22, 2015 06:21, "mruddy" notifications@github.com wrote:
Nice work on this @petertodd https://github.com/petertodd. It'll be very useful once it's rolled out!
I ran some CLTV style micropayment channel deposit and refund tests on testnet.
Overall, the changes look good.
I'm wondering why you chose to make OP_CHECKLOCKTIMEVERIFY not pop the input argument off the top of the stack in the success case?
In the success case, by not popping, the semantic of the original no-op definition of OP_NOP2 is preserved. I assume that maintaining the no-op semantic for old clients, so that they could still validate scripts using the new semantic of the opcode, was your reason. If so, did you have any other reason, like you were also trying to save an upgradable nopcode by not using one for a matching non-verify OP_CHECKLOCKTIME (to fit the pattern of OP_CHECKSIG+OP_CHECKSIGVERIFY, OP_EQUAL+OP_EQUALVERIFY, etc...)?
During the transition, I don't think preserving the no-op semantic in this case is desirable for the same reason that it is not desirable to start using CLTV outputs prior to the 95% lock-in during a soft fork -- the refund path in CLTV scripts could be interpreted as no-op's on non-time-locked transactions to claim the refund outputs early.
If backwards compatibility for older clients was the only reason, then I think that changing to popping the input stack argument should be considered because:
- Doing so would be consistent with the other "verify" opcodes (i.e.- OP_VERIFY, OP_EQUALVERIFY, OP_NUMEQUALVERIFY, OP_CHECKSIGVERIFY, and OP_CHECKMULTISIGVERIFY).
- Doing so is still soft-fork compatible
- During a soft-fork transition, before 95% lock-in, breaking validation of the new scripts in old clients seems preferable, since, for example, we'd like to avoid having premature adopters of CLTV bond refund scripts get early refunds going through (via finalized inputs or earlier than expected transaction nLockTimes). Having new scripts expect that the stack is popped on success would break the new scripts on older validators.
- After the semantic transition is locked-in, it seems that most of the uses of OP_CHECKLOCKTIMEVERIFY would not need the argument left on the stack. Having to repeat the OP_DROP everytime seems kludgy. People that need the argument to remain on the stack could precede the OP_CHECKLOCKTIMEVERIFY with an OP_DUP.
Make sense?
— Reply to this email directly or view it on GitHub #6124 (comment).
@maaku: Doh, after re-thinking, I think you are right about that.
I think I confused myself by asking the wrong question when I was contemplating whether having it pop would then be a hard vs. soft fork change.
Now, when I ask myself: "Would the change cause a transaction to be seen as invalid by older clients, while simultaneously being seen as valid by newer clients?"
The answer is, yes. I even alluded to that in my previous message, "Having new scripts expect that the stack is popped on success would break the new scripts on older validators."
Does that match with what you were thinking? If so, then yes, please forget my suggestion of making the CLTV pop.
Correct. On Jun 22, 2015 10:31 AM, "mruddy" notifications@github.com wrote:
@maaku https://github.com/maaku: Doh, after re-thinking, I think you are right about that.
I think I confused myself by asking the wrong question when I was contemplating whether having it pop would then be a hard vs. soft fork change.
Now, when I ask myself: "Would the change cause a transaction to be seen as invalid by older clients, while simultaneously being seen as valid by newer clients?"
The answer is, yes. I even alluded to that in my previous message, "Having new scripts expect that the stack is popped on success would break the new scripts on older validators."
Does that match with what you were thinking? If so, then yes, please forget my suggestion of making the CLTV pop.
— Reply to this email directly or view it on GitHub #6124 (comment).
@maaku cool, thanks for correcting me on that. @jonasschnelli and @petertodd : About the "asm" translation update, when we were talking about removing the "OP_" prefixes and adding SIGHASH decodes for #5264 and #5392, we did not come up with too much to worry about breaking when changing the "asm" results. Back then, it seemed like breaking dependencies on "asm", if they existed, was perceived as a potentially desirable outcome anyways. For example, #5264 (comment)
@petertodd @mruddy Isn't that sort of out of scope for this particular PR?
@btcdrak @petertodd I think updating the opcode name decode is justifiable in the interest of being complete. People do look at the "asm" in the raw transaction and script decodes. Having it read as OP_NOP2 could be misleading, especially to casual users, after the new semantic is in effect. As far as timing, it's not critical to update the opcode name decode right now (especially if it pushes this pull request out further). The decode update could be done along with the soft-fork updates, or even later as a part of an opcode decode name cleanup (where the "OP_" prefixes get dropped, etc...). But, since there is the justification of being complete with the semantic change, I think it is acceptable to break any possible ill-advised external dependencies on the existing "asm" decode.
@mruddy That all sounds reasonable. After all, if changing the name isn't a big deal, then changing the name with another pull-req shouldn't be a big deal, and will cut down on the amount of unnecessary code changes in the more pull-req that does change consensus-critical code.
IMO, this pull-req is ready for merging as-is.
@petertodd I'm good with that.
Tested ACK
utACK
utACK