BIP-112: Mempool-only CHECKSEQUENCEVERIFY

[maaku]

Replace NOP3 with CHECKSEQUENCEVERIFY (BIP-112)

<nSequence> CHECKSEQUENCEVERIFY -> <nSequence>

Fails if relative lock-time encoded in txin.nSequence < relative lock-time represented by nSequence, allowing funds of a txout to be locked for a number of blocks or a duration of time after its inclusion in a block.

Transactions that fail CSV verification will be rejected from the mempool, making it easy to test the feature. However blocks containing "invalid" CSV-using transactions will still be accepted; this is not the soft-fork required to actually enable CSV for production use.

[btcdrak]

The BIP text is available at bitcoin/bips#179

Edit by @laanwj: or https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki

[kanzure]

Should mempools always reject CSV-failing transactions?

[dcousens]

Could this please read: Inverted sequence numbers above (or equal to) SEQUENCE_THRESHOLD are not consensus constrained.
No comment at all is less confusing than this IMHO.

[maaku]

@kanzure This pull request is implemented such that CSV-failing transactions with tx.nVersion>=2 are considered non-standard and therefore rejected from mempool and/or relay. Given that CSV re-purposes a presently unused NOP opcode, there shouldn't be any compatibility issues. Is there a case you have in mind where a CSV-failing transaction should be added to the mempool?

[maaku]

Rebased, and made compliant with latest version of #6312. Also merged @dcousens code duplication nit, and split the main commit into two for easier review.

Ready for review and merge.

[btcdrak]

s/inverse//

[maaku]

This email I got! Thanks I make the correction.
On Oct 1, 2015 11:12 AM, "฿tcDrak" notifications@github.com wrote:

In src/script/interpreter.cpp
#6564 (comment):

•                // 5-byte numeric operands.
  

•                const CScriptNum nSequence(stacktop(-1), fRequireMinimal, 5);
  

•                // In the rare event that the argument may be < 0 due to
  

•                // some arithmetic being done first, you can always use
  

•                // 0 MAX CHECKSEQUENCEVERIFY.
  

•                if (nSequence < 0)
  

•                    return set_error(serror, SCRIPT_ERR_NEGATIVE_LOCKTIME);
  

•                // To provide for future soft-fork extensibility, if the
  

•                // operand is too large to be treated as a relative lock-
  

•                // time, CHECKSEQUENCEVERIFY behaves as a NOP.
  

•                if (nSequence >= CTxIn::SEQUENCE_LOCKTIME_THRESHOLD)
  

•                    break;
  

•                // Actually compare the specified inverse sequence number
  

s/inverse//

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/6564/files#r40946716.

[kanzure]

utACK

[petertodd]

These changes would be easier to review if they were built on top of #6312 I think, showing the final state of the new CSV feature.

[petertodd]

Or, am I meant to be reviewing #6566? Rather confusing.

[rustyrussell]

utAck

This doesn't seem to make sense without BIP68. I tried to test it, got confused as it errored out comparing a BIP68 nSequence (1073742784, ie 0x400003C0) with a locktime of 500000030....

[petertodd]

Again, I think these pull-req's need to be reviewed as a single unit; they don't make sense apart from each other.

[btcdrak]

I'm leaning to agree with @petertodd about merging #6312 into this PR (or vice versa). #6312 has become quite polished, there are a couple of minor nits to polish off. It seems like the right time to merge both together now since they make sense as a package anyway.

[jtimon]

I agree it's hard to review #6566, #6564 and #6312 like this. What is supposed to come first?
If you want to maintain t\several PRs that's fine. But at least you could rebase one on top of the other.
What advantage there is in having them be completely "independent" from each other?
We will have a preferred merge order, right?

[btcdrak]

@jtimon #6312 (BIP68) and this PR are supposed to be independent of each other, but I think that's been pedantic since OP_CSV isn't useful without BIP68. Merge order doesnt matter.

I think it made sense to have two PRs while narrowing down the semantics of BIP68, but now that's done, it's better to have 1 PR. It will be more expedient for review if we rebase BIP68 onto this PR.

For reference, #6566 is independent and at the last meeting we discussed aiming to merge #6566 for inclusion with the BIP65 soft fork and for BIP68/BIP112 to be included in a separate release.

[jtimon]

Merge order doesn't matter.

Mhmm, that's strange to me, but anyway...Then let's just pick one and rebase one of top of the other. Do we gain anything from not deciding the merge order in advance and maintaining the 3 PRs independent?

[afk11]

We're not inverting the sequence number any more, should this line clarified/removed?

[btcdrak]

Fixed.

[rustyrussell]

Tested-by: Rusty Russell rusty@rustcorp.com.au

(I merged both this and #6312 for the test, with only a trivial conflict to resolve).

[ajtowns]

tested ACK (combined with #7184, #7187). https://github.com/ajtowns/op_csv-test -- lightning-style HTLCs using OP_CSV seem to perform as expected

[morcos]

Personally, I would find this code cleaner and safer if VerifyLockTime was not encapsulated for reuse for nLockTime and nSequence checks. The amount of repeated code is minimal and it would save the fact that we have to jump through a couple of hoops to reuse it now.

That said, if I'm the only one who feels this way, don't let me stand in the way.

[petertodd]

@morcos I'll second being dubious about reuse.

Anyway, ecosystem wide all this stuff will get re-written easily a dozen times in the various libraries...

[maaku]

I think you both missed the point I was trying to make. I'm not saying the
code should be unified, just pointing that out as an example of something
that would be made more difficult by differing semantics. For better or
worse the semantics of nLockTime are already firmly established, and it is
better that both locks use the same semantics as that is less likely to
cause problems than otherwise. Even if the existing semantics are a bit
weird and not what I would have done, at least they'll be consistent.
On Feb 2, 2016 9:28 AM, "Peter Todd" notifications@github.com wrote:

@morcos https://github.com/morcos I'll second being dubious about reuse.

Anyway, ecosystem wide all this stuff will get re-written easily a dozen
times in the various libraries...

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

[morcos]

@maaku I wasn't necessarily conflating these two things. I'm ok leaving the semantics for BIP 68 the same as they are now (and same as nLockTime). I can't decide myself which option I like better.

Separately I think, even if we leave the semantics the same in #7184, that this PR would be better served by not reusing VerifyLockTime in both places.

[btcdrak]

In any case, this is just a differing opinion about implementation, which is a non-blocking nit.

[josephpoon]

Tested ACK using btcdrak's BIP68+OP_CSV combined branch master...btcdrak:sequenceandcsv on regtest.

This pull request is very useful for Lightning Network channels without pre-set expiries. Thanks~~~!

[maaku]

This PR will be updated as soon as BIP 68 is merged.
On Feb 2, 2016 8:25 PM, "josephpoon" notifications@github.com wrote:

Tested ACK using btcdrak's BIP68+OP_CSV combined branch
master...btcdrak:sequenceandcsv
master...btcdrak:sequenceandcsv
on regtest.

This pull request is very useful for Lightning Network channels without
pre-set expiries. Thanks~~~!

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

[CodeShark]

Tested ACK using btcdrak's BIP68+OP_CSV combined branch master...btcdrak:sequenceandcsv on regtest.

[Roasbeef]

Tested ACK.

[NicolasDorier]

Tested ACK and as well replicated in NBitcoin (tests included)

[maaku]

Closing in favor of a future PR based on #7184 .

[btcdrak]

@maaku Why would you close a PR that has been getting heavy actual tested review? sigh

There is zero need to close this PR. Once #7184 is merged this can be merged with a trivial conflict (caused by line breaks) or it could have a quick rebase. The code for this PR has been untouched for months so it is literally thoroughly functionally tested.

[jtimon]

@btcdrak I don't see any lost in closing a PR.
PRs are just like branches: petnames (in the case, in decimal) for commits.
A link to the new PR and traceabilty problems are solved.
Tomorrow I'll reset HEAD^^^^^^^^ and commit a new PR with my name on it in a single commit. Once everybody has reviewed it there's no good reason why anybody would want to know who wrote the original commits or when. Trace-what?
It can even be reopened.

[petertodd]

@jtimon "there's no good reason why anybody would want to know who wrote the original commits or when" <- careful there, we should make sure all authors of the code in question are credited appropriately!

While I haven't run into this too often yet, I'd even make a point of crediting people if I build upon their pull-reqs with total rewrites of the code; even in cases where 100% of the original code gets thrown out such people often - if not always - have done valuable work exploring the problem and deserve credit for it.

[laanwj]

Of course people should be properly credited for their work! Why is this even a discussion?
Mentioning someone's name in the commit message and pull request is a good way to do this.

[btcdrak]

I think i'll just reopen this in a separate PR and push the original work rebased.

[maaku]

It was closed because there is no way I know of to change the repo a PR draws from, I will not be maintaining this patch, and for security reasons I can no longer be adding outside collaborators on my own repo to maintain these pulls. Someone who will be maintaining this patch needs to open a new PR. Blame Github.

[btcdrak]

@maaku sure, I'm just rebasing it and will open a new one shortly.

Edit it has been moved to #7524