ACK. This version is well tested.
There are some ACKs for this PR in #5496 from before it was changed to a parameterised version.

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.

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

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.

@maaku's Fixed nit. Also thanks for the review!

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:

• 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?

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):

*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" [email protected] 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" [email protected] 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)

@mruddy Thanks for reminding me! That's a good argument. :) I'll look into doing that then.

@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