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

Mempool only CHECKLOCKTIMEVERIFY (BIP65) verification, unparameterized version #6124

Merged
merged 4 commits into from Jun 26, 2015

Conversation

@petertodd
Copy link
Contributor

petertodd commented May 12, 2015

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.

petertodd added 2 commits Sep 29, 2014
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.
@btcdrak
Copy link
Contributor

btcdrak commented May 12, 2015

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

@NicolasDorier
Copy link
Contributor

NicolasDorier commented May 12, 2015

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 )

@gavinandresen
Copy link
Contributor

gavinandresen commented May 13, 2015

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.

@sipa
Copy link
Member

sipa commented May 13, 2015

@laanwj laanwj added the Consensus label May 15, 2015
//
// Thus as a special case we tell CScriptNum to accept up
// to 5-byte bignums, which are good until 2**32-1, the
// same limit as the nLockTime field itself.

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 15, 2015

Member

This is not clear: 5-byte CScriptNum should be good up to 2**39-1, beyond the limit of the nLockTime field.

This comment has been minimized.

Copy link
@petertodd

petertodd May 15, 2015

Author Contributor

Fixed.

@petertodd petertodd force-pushed the petertodd:unparameterized-cltv branch 2 times, most recently from 7978496 to afc4ff9 May 15, 2015
@jtimon
Copy link
Member

jtimon commented May 15, 2015

Yes, the whole point of BaseSignatureChecker is that EvalScript doesn't need to know anything about transactions.
Untested ACK

@NicolasDorier
Copy link
Contributor

NicolasDorier commented May 26, 2015

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.

@jtimon
Copy link
Member

jtimon commented May 26, 2015

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.

@rubensayshi
Copy link
Contributor

rubensayshi commented Jun 3, 2015

utACK

@@ -76,6 +76,10 @@ enum
// (softfork safe, BIP62 rule 6)
// Note: CLEANSTACK should never be used without P2SH.
SCRIPT_VERIFY_CLEANSTACK = (1U << 8),

// Verify CHECKLOCKTIMEVERIFY (BIP65)
//

This comment has been minimized.

Copy link
@maaku

maaku Jun 20, 2015

Contributor

Stupidest little nit... this empty comment line can be removed, no? Or better yet explain the feature like the other comments do.

@maaku
Copy link
Contributor

maaku commented Jun 20, 2015

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.

petertodd added 2 commits Sep 29, 2014
<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.
@petertodd petertodd force-pushed the petertodd:unparameterized-cltv branch from afc4ff9 to ffd75ad Jun 22, 2015
@petertodd
Copy link
Contributor Author

petertodd commented Jun 22, 2015

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

@jgarzik
Copy link
Contributor

jgarzik commented Jun 22, 2015

code review ACK

Are there any remaining blockers to merging this?

@petertodd
Copy link
Contributor Author

petertodd commented Jun 22, 2015

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

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 22, 2015

code review ACK.

Will test and write some example RPC regtests within the next days.

@petertodd
Copy link
Contributor Author

petertodd commented Jun 22, 2015

@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

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 22, 2015

@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.

@petertodd
Copy link
Contributor Author

petertodd commented Jun 22, 2015

@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.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 22, 2015

Sure. I didn't mention the rpc test to hold this PR back.
Merging is totally fine for me.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 22, 2015

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";

@petertodd
Copy link
Contributor Author

petertodd commented Jun 22, 2015

@jonasschnelli Good point.

Anyone have any idea what doing that will break? :/

@btcdrak
Copy link
Contributor

btcdrak commented Jun 22, 2015

@petertodd the tests...

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 22, 2015

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.

@mruddy
Copy link
Contributor

mruddy commented Jun 22, 2015

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?

@mruddy
Copy link
Contributor

mruddy commented Jun 22, 2015

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.

@maaku
Copy link
Contributor

maaku commented Jun 22, 2015

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

@mruddy
Copy link
Contributor

mruddy commented Jun 22, 2015

@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.

@maaku
Copy link
Contributor

maaku commented Jun 22, 2015

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

@mruddy
Copy link
Contributor

mruddy commented Jun 22, 2015

@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
Copy link
Contributor Author

petertodd commented Jun 22, 2015

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

@btcdrak
Copy link
Contributor

btcdrak commented Jun 22, 2015

@petertodd @mruddy Isn't that sort of out of scope for this particular PR?

@mruddy
Copy link
Contributor

mruddy commented Jun 23, 2015

@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.

@petertodd
Copy link
Contributor Author

petertodd commented Jun 23, 2015

@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.

@mruddy
Copy link
Contributor

mruddy commented Jun 23, 2015

@petertodd I'm good with that.

Tested ACK

@afk11
Copy link
Contributor

afk11 commented Jun 25, 2015

utACK

1 similar comment
@laanwj
Copy link
Member

laanwj commented Jun 26, 2015

utACK

@laanwj laanwj merged commit ffd75ad into bitcoin:master Jun 26, 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 Jun 26, 2015
ffd75ad Enable CHECKLOCKTIMEVERIFY as a standard script verify flag (Peter Todd)
bc60b2b Replace NOP2 with CHECKLOCKTIMEVERIFY (BIP65) (Peter Todd)
48e9c57 Move LOCKTIME_THRESHOLD to src/script/script.h (Peter Todd)
99088d6 Make CScriptNum() take nMaxNumSize as an argument (Peter Todd)
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.