BIP-112: Mempool-only CHECKSEQUENCEVERIFY

maaku commented at 7:18 pm on August 17, 2015: contributor

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 commented at 7:21 pm on August 17, 2015: contributor

The BIP text is available at https://github.com/bitcoin/bips/pull/179

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

kanzure commented at 7:47 pm on August 17, 2015: contributor

Should mempools always reject CSV-failing transactions?

in src/script/interpreter.cpp: in a597a33187 outdated

1182+            return false;
1183+        // Sequence number must be inverted to convert it into a
1184+        // relative lock-time.
1185+        txToLockTime = (int64_t)~txTo->vin[nIn].nSequence;
1186+        // Sequence numbers under SEQUENCE_THRESHOLD are not consensus
1187+        // constrained.

dcousens commented at 11:51 pm on August 17, 2015:

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.

in src/script/interpreter.cpp: in a597a33187 outdated

1171 {
1172+    // Relative lock times are supported by comparing the passed
1173+    // in lock time to the sequence number of the input. All other
1174+    // logic is the same, all that differs is what we are comparing
1175+    // the lock time to.
1176+    int64_t txToLockTime;

dcousens commented at 11:53 pm on August 17, 2015:

Since this is repeatedly referred to as a relative lock time, can we reflect that in the code? relativeLockTime?

in src/script/interpreter.cpp: in a597a33187 outdated

1165@@ -1126,8 +1166,30 @@ bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn
1166     return true;
1167 }
1168 
1169-bool TransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime) const
1170+bool TransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime, bool fSequence) const

dcousens commented at 0:00 am on August 18, 2015:

Why is this being put in the same function anyway? It only shares 10 lines of code, and adds another 10. It could be easily a 10 line function and another 10 line function which then calls CheckLockTime.

This would have less branching, would be simpler to test, and have no default variables.

checker.CheckSequenceNumber(... Would also make https://github.com/bitcoin/bitcoin/pull/6564/files#diff-be2905e2f5218ecdbe4e55637dac75f3R416 clearer.

maaku commented at 11:25 pm on August 18, 2015: contributor

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

laanwj added the label Mempool on Aug 20, 2015

laanwj added the label Consensus on Aug 20, 2015

maaku renamed this:
~~Mempool-only CHECKSEQUENCEVERIFY~~
BIP-112: Mempool-only CHECKSEQUENCEVERIFY
on Sep 24, 2015

maaku force-pushed on Sep 25, 2015

maaku commented at 0:10 am on September 26, 2015: contributor

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.

maaku force-pushed on Sep 26, 2015

maaku force-pushed on Sep 27, 2015

maaku force-pushed on Sep 29, 2015

in src/script/interpreter.cpp: in abfa03ade2 outdated

408+                    // operand is too large to be treated as a relative lock-
409+                    // time, CHECKSEQUENCEVERIFY behaves as a NOP.
410+                    if (nSequence >= CTxIn::SEQUENCE_LOCKTIME_THRESHOLD)
411+                        break;
412+
413+                    // Actually compare the specified inverse sequence number

btcdrak commented at 6:11 pm on October 1, 2015:

s/inverse//

maaku commented at 7:10 pm on October 1, 2015:

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

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

in src/test/data/tx_valid.json: in abfa03ade2 outdated

285+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 NOP3 1"]],
286+"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffff7f0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"],
287+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 NOP3 1"]],
288+"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"],
289+
290+["5 byte non-minimally-encoded operandss are valid"],

kanzure commented at 10:01 pm on October 1, 2015:

nit: typo (“operandss”)

kanzure commented at 10:10 pm on October 1, 2015: contributor

utACK

petertodd commented at 5:12 pm on October 8, 2015: contributor

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 commented at 5:26 pm on October 8, 2015: contributor

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

rustyrussell commented at 1:43 am on October 12, 2015: contributor

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 commented at 1:53 am on October 12, 2015: contributor

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 commented at 8:52 pm on October 15, 2015: contributor

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 commented at 10:15 am on October 19, 2015: contributor

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 commented at 6:12 pm on October 19, 2015: contributor

@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 commented at 6:23 pm on October 19, 2015: contributor

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?

maaku commented at 10:00 pm on October 19, 2015: contributor

Consistent feedback from the bitcoin pull request process has been to break up pulls to do one minimal thing / to not have multi-feature pulls, and to not have dependent pull requests when at all possible. In light of this and since this pull and #6312 don’t actually share any code (except for 3 constants in src/primitives/transaction.h), they were submitted separately.

maaku force-pushed on Oct 20, 2015

maaku commented at 1:28 am on October 20, 2015: contributor

Rebased with new semantics.

btcdrak commented at 4:06 am on October 20, 2015: contributor

utACK

in src/script/script.h: in e4436267e0 outdated

251@@ -251,6 +252,11 @@ class CScriptNum
252     inline CScriptNum& operator+=( const CScriptNum& rhs)       { return operator+=(rhs.m_value);  }
253     inline CScriptNum& operator-=( const CScriptNum& rhs)       { return operator-=(rhs.m_value);  }
254 
255+    inline CScriptNum operator&(   const int64_t& rhs)    const { return CScriptNum(m_value & rhs);}
256+    inline CScriptNum operator&(   const CScriptNum& rhs) const { return operator&(rhs.m_value);   }
257+
258+    inline CScriptNum& operator&=( const CScriptNum& rhs)       { return operator+=(rhs.m_value);  }

instagibbs commented at 3:59 pm on October 21, 2015:

Should be &= not += I assume

instagibbs commented at 4:24 pm on October 21, 2015: member

utACK otherwise

maaku force-pushed on Oct 21, 2015

dcousens commented at 10:34 pm on October 21, 2015: contributor

utACK

rubensayshi commented at 9:51 am on October 22, 2015: contributor

utACK

in src/primitives/transaction.h: in 58d21922fa outdated

60@@ -61,6 +61,18 @@ class CTxIn
61     CScript scriptSig;
62     uint32_t nSequence;
63 
64+    /* If this flag set, CTxIn::nSequence is NOT interpreted as a
65+     * relative lock-time. Setting the most significant bit of a
66+     * sequence number disabled relative lock-time. */

jmcorgan commented at 3:30 pm on October 22, 2015:

s/disabled/disables/

afk11 commented at 3:37 pm on October 22, 2015: contributor

tACK

jmcorgan commented at 3:40 pm on October 22, 2015: contributor

utACK

dcousens commented at 0:32 am on October 23, 2015: contributor

Needs rebase

btcdrak force-pushed on Oct 23, 2015

btcdrak commented at 7:37 pm on October 24, 2015: contributor

A couple of people have requested to see the #6312 and #6564 rebased together in one branch for review. I have done this at https://github.com/bitcoin/bitcoin/compare/master...btcdrak:sequenceandcsv. Please note there is one less commit 78370bb827320660e2016ea7e42fedc5a66677dd was duplicated in both. I’m happy to roll that into a PR if necessary, although the purpose is really just to assist the testers who asked for it.

btcdrak force-pushed on Oct 27, 2015

btcdrak commented at 0:34 am on October 27, 2015: contributor

The BIP68+OP_CSV combined branch has been updated https://github.com/bitcoin/bitcoin/compare/master...btcdrak:sequenceandcsv

jgarzik commented at 8:01 am on November 13, 2015: contributor

ut ACK

greenaddress commented at 12:05 pm on November 19, 2015: contributor

utACK

Add CTxIn::SEQUENCE_LOCKTIME_DISABLED_FLAG, CTxIn::SEQUENCE_LOCKTIME_SECONDS_FLAG, and CTxIn::SEQUENCE_LOCKTIME_MASK to primitive/transaction.h

Will be needed by CHECKSEQUENCEVERIFY code.

85a4a2e328

Pull most of CheckLockTime() out into VerifyLockTime(), a local function that will be reused for CheckSequence() 66a7ef3e13

Add bitwise AND operator to CScriptNum 46e8638609

Replace NOP3 with CHECKSEQUENCEVERIFY (BIP112)

<nSequence> CHECKSEQUENCEVERIFY -> <nSequence>

Fails if txin.nSequence < 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.

Only the logic and unit tests are implemented; this commit does not have changes to policy or consensus behavior.

585ddfe756

Enable CHECKSEQUENCEVERIFY as a standard script verify flag

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.

569737d75f

Typo fixes and added comments cc58b71f15

btcdrak force-pushed on Nov 21, 2015

Rename flags

_SECONDS_FLAG to _TYPE_FLAG
_DISABLED_FLAG to _DISABLE_FLAG

e8ae3ffbd2

btcdrak force-pushed on Nov 21, 2015

in src/script/interpreter.cpp: in 3fef18f1aa outdated

399+                    // operand has the disabled lock-time flag set,
400+                    // CHECKSEQUENCEVERIFY behaves as a NOP.
401+                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0)
402+                        break;
403+
404+                    // Actually compare the specified inverse sequence number

afk11 commented at 4:21 pm on November 24, 2015:

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

btcdrak commented at 5:45 pm on November 24, 2015:

Fixed.

btcdrak force-pushed on Nov 24, 2015

Fixup comments 2ce146b87e

btcdrak force-pushed on Nov 25, 2015

rustyrussell commented at 4:24 am on January 7, 2016: contributor

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 commented at 4:15 pm on January 28, 2016: member

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 commented at 3:30 pm on February 2, 2016: member

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 commented at 5:27 pm on February 2, 2016: contributor

@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 commented at 5:50 pm on February 2, 2016: contributor

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 commented at 5:57 pm on February 2, 2016: member

@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 commented at 5:59 pm on February 2, 2016: contributor

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

josephpoon commented at 4:24 am on February 3, 2016: none

Tested ACK using btcdrak’s BIP68+OP_CSV combined branch https://github.com/bitcoin/bitcoin/compare/master...btcdrak:sequenceandcsv on regtest.

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

maaku commented at 4:51 am on February 3, 2016: contributor

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 https://github.com/bitcoin/bitcoin/compare/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 commented at 8:36 pm on February 6, 2016: contributor

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

Roasbeef commented at 9:35 pm on February 6, 2016: none

Tested ACK.

NicolasDorier commented at 11:28 am on February 10, 2016: contributor

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

maaku commented at 8:08 pm on February 11, 2016: contributor

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

maaku closed this on Feb 11, 2016

btcdrak commented at 1:15 am on February 12, 2016: contributor

@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 commented at 2:40 am on February 12, 2016: contributor

@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 commented at 3:48 pm on February 12, 2016: contributor

@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 commented at 3:51 pm on February 12, 2016: member

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 commented at 4:17 pm on February 12, 2016: contributor

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

maaku commented at 4:19 pm on February 12, 2016: contributor

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 commented at 5:07 pm on February 12, 2016: contributor

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

Edit it has been moved to #7524

MarcoFalke locked this on Sep 8, 2021

BIP-112: Mempool-only CHECKSEQUENCEVERIFY #6564