BIP112: CHECKSEQUENCEVERIFY #179

pull btcdrak wants to merge 3 commits into bitcoin:master from btcdrak:bip-checksequenceverify changing 2 files +252 −0
  1. btcdrak commented at 6:21 PM on August 13, 2015: contributor

    Todo

    • BIP number assignment
    • Amend text for median-past-timelock BIP number when assigned
  2. btcdrak force-pushed on Aug 13, 2015
  3. in bip-csv.mediawiki:None in 3cd470635c outdated 
     111 | +        // Sequence number must be inverted to convert it into a
 112 | +        // relative lock-time.
 113 | +        txToInvSequence = (int64_t)~txTo->vin[nIn].nSequence;
 114 | +        
 115 | +        // Sequence numbers under SEQUENCE_THRESHOLD are not consensus
 116 | +        // constrained.
    dcousens commented at 3:03 AM on August 14, 2015:

    Is this meant to be above, or am I misunderstanding what is meant here by consensus constrained? If I am misunderstanding, then perhaps the conditional should also be switched such that it aligns with comment.

    Sequence numbers equal to or above the SEQUENCE_THRESHOLD are not treated as relative lock-times, nor are they given any consensus-enforced meaning at this point.

    From BIP68. I assume therefore that this comment is wrong?

  4. in bip-csv.mediawiki:None in 3cd470635c outdated 
      98 | +        break;
  99 | +    }
 100 | +    
 101 | +    bool CheckSequence(const CScriptNum& nInvSequence) const
 102 | +    {
 103 | +        int64_t txToInvSequence;
    dcousens commented at 3:07 AM on August 14, 2015:

    Why is this declared at the top of the scope?

    edit: seems to be an artefact of https://github.com/bitcoin/bitcoin/commit/bb0b59eb5fed7555eed705cad6e360a7617daf0e#diff-be2905e2f5218ecdbe4e55637dac75f3R1176

  5. in bip-csv.mediawiki:None in 3cd470635c outdated 
     130 | +        ))
 131 | +            return false;
 132 | +        
 133 | +        // Now that we know we're comparing apples-to-apples, the
 134 | +        // comparison is a simple numeric one.
 135 | +        if (nInvSequence > txInvToSequence)
    dcousens commented at 3:38 AM on August 14, 2015:

    Would return nInvSequence <= txInvToSequence suffice?

    Otherwise, ACK, LGTM.

    btcdrak commented at 7:26 AM on August 14, 2015:

    It might, but in practice this code is integrated into the existing TransactionSignatureChecker::CheckLockTime so the optimisation wouldn't persist.

    dcousens commented at 8:53 AM on August 14, 2015:

    Ah! I see that in @maaku's commit https://github.com/bitcoin/bitcoin/commit/bb0b59eb5fed7555eed705cad6e360a7617daf0e.

    unknown commented at 2:16 PM on August 18, 2015:

    slight spelling error in the variable name if (nInvSequence > txInvToSequence) should read if (nInvSequence > txToInvSequence)

  6. in bip-csv.mediawiki:None in 3cd470635c outdated 
     123 | +        //
 124 | +        // We want to compare apples to apples, so fail the script
 125 | +        // unless the type of lock-time being tested is the same as
 126 | +        // the lock-time in the transaction input.
 127 | +        if (!(
 128 | +            (txToInvSequence <  LOCKTIME_THRESHOLD && nInvSequence <  LOCKTIME_THRESHOLD) ||
    dcousens commented at 8:44 AM on August 14, 2015:

    What is LOCKTIME_THRESHOLD in this context? It might be useful to specify this so that the example below is more clear.

    edit: It seems LOCKTIME_THRESHOLD is a constant that has been long declared in Bitcoin. IMHO it would be useful to specify that so that this proposal is easier to implement without having to look up information in the core implementation.

    btcdrak commented at 9:06 AM on August 14, 2015:

    Maybe I could add this at the top of the code snippet for reference.

    // Threshold for nLockTime: below this value it is interpreted as block number,
// otherwise as UNIX timestamp.
static const unsigned int LOCKTIME_THRESHOLD = 500000000; // Tue Nov  5 00:53:20 1985 UTC

// Threshold for inverted CTxIn::nSequence: below this value it is interpreted
// as a relative lock-time, otherwise ignored.
static const uint32_t SEQUENCE_THRESHOLD = (1 << 31);
    dcousens commented at 9:19 AM on August 14, 2015:

    I think that would help a lot.

    btcdrak commented at 1:00 PM on August 14, 2015:

    OK I have added these.

  7. btcdrak cross-referenced this on Aug 17, 2015 from issue BIP-112: Mempool-only CHECKSEQUENCEVERIFY by maaku
  8. jtimon commented at 10:36 AM on August 19, 2015: contributor

    I extremely dislike the inversion to preserve the "previous nSequence semantics". The "previous nSequence semantics" were consensus-unenforceable but we can cover the same use cases (or the realistic ones at least) with nMaturity. Let's face it and move on without technical debt we don't need and may regret. If we do this inversion we will likely carry it for very long if not forever. I don't think we will ever regret not having done the inversion.

    As a side effect of this proposed change, I believe documentation (not just this BIP but also doxygen's) can become much clearer (maybe even shorter simultaneously).

  9. maaku commented at 4:21 PM on August 19, 2015: contributor

    I am indifferent on this issue (the bit inversion), but so far only Jorge has spoken up. I opted for this detail during implementation in order to preserve existing semantics, even if those semantics are not commonly used. This was the conservative choice, driven in part because I didn't want the proposal to be held up by the other side saying "this is confusing because it changes how sequence numbers work! it used to count up but now it counts down!"

    I can see both sides and as I said I'm indifferent, so I went with the conservative choice of not messing with existing semantics. However if there is strong preferences from multiple people on this matter it is not too late to change. If anyone feels strongly about this, please speak up.

  10. jtimon commented at 5:09 PM on August 19, 2015: contributor

    If I'm the only one opposed to the inversion, I certainly prefer this with the inversion than not having the new op at all. But as you say the code change is relatively simple, and I don't think sufficient reasons have been provided to justify the bit inversion to "preserve the old semantics" (specially given that the old semantics were not enforceable). The new semantics are better with or without the inversion. I think that even if the inversion is maintained the BIP should contain the motivations of that decision. Ideally with concrete example use cases that would be broken instead of a vague "to preserve the old semantics" (which would still be better than what BIP draft currently has).

    But, yes, please, more "I prefer to do the inversion" (0), "I'm indifferent" (1) and "I prefer not to do the inversion" (1) from other people would be appreciated. I suspect most people will, like you, be indifferent, but I'm specially interested on what arguments (and "this is not what Satoshi had in mind for nSequence" is not a logical argument) people would have to "preserve the current semantics".

    Maybe @mikehearn can help providing those reasons? I don't understand his points about "resource scheduling and update flood damping" in http://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-May/008293.html

    So, to be completely clear, this is not a NACK, just a nit. If the inversion is going to be used, please document why. If there's no good reason to use the inversion, just not using it will make everything simpler and more readable.

  11. NicolasDorier commented at 10:51 AM on August 23, 2015: contributor

    Just want to mention that nSequence is used by a colored coin protocol EPOBC. I don't know myself the detail of this protocol very well, but if we don't keep nSequence semantic it may be very detrimental to chromaway. The inversion may fix the issue. (not checked that into the details)

  12. maaku commented at 3:16 PM on August 23, 2015: contributor

    There is an exception explicitly for such purposes. Just make sure you set bit 31. On Aug 23, 2015 3:51 AM, "Nicolas Dorier" notifications@github.com wrote:

    Just want to mention that nSequence is used by a colored coin protocol EPOBC https://github.com/chromaway/ngcccbase/wiki/EPOBC. I don't know myself the detail of this protocol very well, but if we don't keep nSequence semantic it may be very detrimental to chromaway.

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

  13. btcdrak renamed this:
    BIP CHECKSEQUENCEVERIFY
    BIP112: CHECKSEQUENCEVERIFY
    on Aug 24, 2015
  14. gmaxwell commented at 11:04 PM on September 3, 2015: contributor

    Whats the current status here?

  15. btcdrak force-pushed on Sep 3, 2015
  16. btcdrak force-pushed on Sep 3, 2015
  17. btcdrak commented at 11:30 PM on September 3, 2015: contributor

    @gmaxwell I believe there was some question of whether or not to use the inversion. http://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-August/010686.html

  18. CodeShark commented at 12:51 AM on September 6, 2015: contributor

    Unless changing the semantics breaks something, I'm for using the most transparent semantics here. Given that almost nobody uses the nSequence field at all, it's probably less painful to change the semantics now than to have to document this quirk later on when everyone is using it.

  19. maaku commented at 1:12 AM on September 6, 2015: contributor

    There seems to be a general consensus in favor of dropping the bit inversion.

    The sequencenumbers3 branch contains future soft-fork bits that are shifted out of register, as per @gmaxwell's suggestion. This adds future expansion capability at the cost of relative lock times being limited to about 2 years (not an issue in foreseeable use cases) and quite a bit more code complexity (a bit more of an issue...)

    So far Rusty is the only one that had commented on this. Both he and I are mildly against this due to being considerably more complicated. If we want to remove the bit inversion to make relative lock times semantically simpler it doesn't make sense to then add a bunch of bit shifts.

    But I would like a more clear consensus before making a call on this and changing the text and code. Can a few people review that branch and express an opinion? On Sep 5, 2015 5:51 PM, "Eric Lombrozo" notifications@github.com wrote:

    Unless bit inversion breaks something, I'm for using the most transparent semantics here. Given that almost nobody uses the nSequence field at all, it's probably less painful to change the semantics now than to have to document this quirk later on.

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

  20. jtimon commented at 1:15 AM on September 6, 2015: contributor

    Is there a branch that drops the bit inversion without making any further changes?

  21. maaku commented at 1:16 AM on September 6, 2015: contributor

    sequencenumbers2 On Sep 5, 2015 6:15 PM, "Jorge Timón" notifications@github.com wrote:

    Is there a branch that drops the bit inversion without making any further changes?

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

  22. luke-jr commented at 4:59 AM on September 6, 2015: member

    Why not mask them with a logical AND instead of bit shifts? Wouldn't that keep it semantically simple?

    Also, I feel like I've brought this up before, but: I really think this should use a new name besides "sequence" since it is completely changing the semantics from what the original purpose was.

  23. jtimon commented at 5:45 AM on September 6, 2015: contributor

    I would rename nSequence to nMaturity. In fact, if I remember correctly the first time we talked about something similar to rcltv/csv we called it "op_maturity" which in my opinion is much more clear and would probably simplify the documentation.

  24. maaku commented at 5:55 AM on September 6, 2015: contributor

    I'm going to be the odd one out here and claim the sequence numbers actually describes exactly what the field's semantics is, regardless of whether you are counting up or down. Given two transactions, the sequence number can be used to determine which came after the other. On Sep 5, 2015 10:45 PM, "Jorge Timón" notifications@github.com wrote:

    I would rename nSequence to nMaturity. In fact, if I remember correctly the first time we talked about something similar to rcltv/csv we called it "op_maturity" which in my opinion is much more clear and would probably simplify the documentation.

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

  25. CodeShark commented at 6:17 AM on September 6, 2015: contributor

    @maaku Technically, that is true - but I think the bigger point is that the use cases we're after don't really care about sequence...that the original semantics are preserved is sort of a side effect of the more important feature which is relative time lock which is what is actually enforced by the consensus rules.

  26. CodeShark commented at 6:26 AM on September 6, 2015: contributor

    Also, it's possible to think of pathological cases where the sequence numbers don't necessarily reflect the sequence. i.e. a higher sequence number transaction with no fee and a lower sequence number transaction with a very high fee that spends the same output.

  27. CodeShark commented at 7:07 AM on September 6, 2015: contributor

    In other words, since nSequence isn't being used at all, we have a free field we can repurpose for RCLTV which we already know has many good use cases. The fact we can preserve the semantics via a little hackery, while a clever observation, isn't really what's important.

  28. jtimon commented at 4:40 PM on September 6, 2015: contributor

    I don't want to be repetitive, but this has exactly the same semantics as the 100 blocks maturity for coinbase inputs. In fact, I would prefer this check https://github.com/bitcoin/bitcoin/compare/master...maaku:sequencenumbers2#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR943 to be done in the same place where the coinbase maturity is checked, here: https://github.com/jtimon/bitcoin/blob/consensus-txinputs-0.12.99/src/main.cpp#L1301

  29. btcdrak commented at 10:13 PM on September 6, 2015: contributor

    I'm in agreement with @maaku

  30. btcdrak force-pushed on Sep 6, 2015
  31. BIP for OP_CHECKSEQUENCEVERIFY ae46943f11
  32. Comprehensive editing to clarify meaning and intent of existing text. 6150b3a857
  33. Add constant definitions for clarity
    Formatting Fix typos

Update with BIP 112 assignment

Update references to BIP113

Update deployment methodology

Add references
    6902f790f1
  34. btcdrak force-pushed on Sep 6, 2015
  35. btcdrak commented at 11:05 PM on September 6, 2015: contributor

    Just a heads up, I've rebased and added to the index.

  36. luke-jr referenced this in commit 063a8ad82a on Sep 19, 2015
  37. luke-jr merged this on Sep 19, 2015
  38. luke-jr closed this on Sep 19, 2015
  39. btcdrak deleted the branch on Sep 20, 2015
  40. luke-jr referenced this in commit 95e7196560 on Jun 6, 2017
  41. pinheadmz referenced this in commit 7c7aead1c1 on Dec 15, 2019
Contributors
btcdrakdcousensjtimonmaakuNicolasDoriergmaxwellCodeSharkluke-jr
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-27 15:10 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me