Extract CTxIn::MAX_SEQUENCE_NONFINAL constant, rework BIP 65/68/112 docs #24136

MarcoFalke commented at 8:56 AM on January 24, 2022: member

Extracting the constant makes it possible to attach documentation to it.

Also, rework the docs for the other "sequence constants".

DrahtBot added the label RPC/REST/ZMQ on Jan 24, 2022

DrahtBot added the label Wallet on Jan 24, 2022

MarcoFalke removed the label Wallet on Jan 24, 2022

MarcoFalke removed the label RPC/REST/ZMQ on Jan 24, 2022

MarcoFalke added the label Refactoring on Jan 24, 2022

MarcoFalke added the label Docs on Jan 24, 2022

prusnak approved

prusnak commented at 6:30 PM on January 24, 2022: contributor

Approach ACK fab3ff2

in src/primitives/transaction.h:95 in fab3ff2071 outdated

  93 | +    /**
  94 | +     * If this flag is set, CTxIn::nSequence is NOT interpreted as a
  95 | +     * relative lock-time.
  96 | +     * It skips SequenceLocks() for any input that has it set (BIP 68).
  97 | +     * It fails OP_CHECKSEQUENCEVERIFY/CheckSequence() for any input that has
  98 | +     * it set (BIP 112).

darosior commented at 8:56 AM on January 25, 2022:

You are using the same language here as for BIP65 timelocks, whereas in this case it's "local" (if an input needs both satisfy a BIP112 timelock and its nSequence has the disable flag) while in the other it's "global" (if any input of the spending transaction has a BIP65 timelock and any other input has its nSequence set to SEQUENCE_FINAL).

I don't have a better suggestion. Just a nit that it might be confusing to someone only from these comments.

MarcoFalke commented at 11:54 AM on January 26, 2022:

Sorry, can you explain this a bit better? Both OP_CHECKLOCKTIMEVERIFY and OP_CHECKSEQUENCEVERIFY are evaluated in a local context of the input.

Sure, the whole transaction fails "globally", if any input is invalid, but this is true for both opcodes as well.

darosior commented at 12:01 PM on January 26, 2022:

Brainfart, sorry. I for some reason mistakenly assumed that IsFinalTx() returned true if any input's nSequence was set to SEQUENCE_FINAL... And of course didn't double check before posting this confusing comment.

MarcoFalke commented at 12:25 PM on January 26, 2022:

No worries. Going to resolve the thread for now.

darosior commented at 8:58 AM on January 25, 2022: member

ACK fab3ff2071682caf77680be49e934b434e732988

w0xlt approved

w0xlt commented at 1:56 PM on January 26, 2022: contributor

ACK fab3ff2 for improving documentation.

Extract CTxIn::MAX_SEQUENCE_NONFINAL constant fa4339e4c1

MarcoFalke force-pushed on Jan 26, 2022

MarcoFalke commented at 2:12 PM on January 26, 2022: member

Sorry for the force push. Clarified tx.version >= 2, requested in comment https://github.com/bitcoin/bitcoin/pull/7184/files#r47233948

DrahtBot commented at 12:57 AM on January 27, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke)
#22871 (Discourage CSV as NOP when locktime disable is set & discourage unknown nSequence by JeremyRubin)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

w0xlt approved

w0xlt commented at 2:39 AM on January 27, 2022: contributor

reACK fa4339e for specifying the transaction version.

darosior commented at 4:02 PM on January 27, 2022: member

re-ACK fa4339e4c1bb60e0d9263d4f0fe65d03aad52f88

luke-jr approved

luke-jr commented at 8:43 PM on January 29, 2022: member

crACK fa4339e4c1bb60e0d9263d4f0fe65d03aad52f88

in src/primitives/transaction.h:88 in fa4339e4c1

  88 | +    static const uint32_t MAX_SEQUENCE_NONFINAL{SEQUENCE_FINAL - 1};
  89 |  
  90 | -    /* Below flags apply in the context of BIP 68*/
  91 | -    /* If this flag set, CTxIn::nSequence is NOT interpreted as a
  92 | -     * relative lock-time. */
  93 | +    // Below flags apply in the context of BIP 68. BIP 68 requires the tx

jonatack commented at 11:56 PM on January 30, 2022:

I see this is from BIP 68, but it could be better (may propose some fixups to the BIP), plus add a newline to denote that this comment applies to the next four sections of doxygen docs + constants

      * disables nLockTime. */
     static const uint32_t SEQUENCE_FINAL = 0xffffffff;
 
-    /* Below flags apply in the context of BIP 68*/
+    /* The following flags apply in the context of BIP 68: */
+
     /* If this flag set, CTxIn::nSequence is NOT interpreted as a
      * relative lock-time. */

MarcoFalke commented at 7:27 AM on January 31, 2022:

I think it should be clear that the comment, which uses plural ("flags") refers to the flags below. I think not using a newline also helps here, so going to leave as-is for now.

jonatack commented at 12:28 AM on January 31, 2022: member

Concept ACK. Another replacement to consider:

diff --git a/src/util/rbf.h b/src/util/rbf.h
index a957617389..33a38dad88 100644
--- a/src/util/rbf.h
+++ b/src/util/rbf.h
@@ -13,11 +13,12 @@ static constexpr uint32_t MAX_BIP125_RBF_SEQUENCE{0xfffffffd};
 
 /** Check whether the sequence numbers on this transaction are signaling opt-in to replace-by-fee,
  * according to BIP 125.  Allow opt-out of transaction replacement by setting nSequence >
- * MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs.
+ * MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL - 2) on all inputs.
 *
-* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by non-replaceable transactions. All
-* inputs rather than just one is for the sake of multi-party protocols, where we don't want a single
-* party to be able to disable replacement by opting out in their own input. */
+* CTxIn::MAX_SEQUENCE_NONFINAL (SEQUENCE_FINAL - 1) is picked to still allow use
+* of nLockTime by non-replaceable transactions. Using all inputs rather than just one
+* is for the sake of multi-party protocols, where we don't want a single party
+* to be able to disable replacement by opting out in their own input. */
 bool SignalsOptInRBF(const CTransaction& tx);
 
 #endif // BITCOIN_UTIL_RBF_H

Also: s/All inputs/Using all inputs/

MarcoFalke merged this on Jan 31, 2022

MarcoFalke closed this on Jan 31, 2022

MarcoFalke deleted the branch on Jan 31, 2022

MarcoFalke commented at 8:59 AM on January 31, 2022: member

Another replacement to consider:

The goal of this pull was to document consensus rules. If policy is documented, I am wondering if the following diff should also be applied:

diff --git a/src/util/rbf.h b/src/util/rbf.h
index a957617389..fc1fbb60de 100644
--- a/src/util/rbf.h
+++ b/src/util/rbf.h
@@ -9,6 +9,11 @@
 
 class CTransaction;
 
+/**
+ * This is the maximum sequence number that enables all of: BIP 125,
+ * nLockTime and OP_CHECKLOCKTIMEVERIFY (BIP 65).
+ * It has SEQUENCE_LOCKTIME_DISABLE_FLAG set (BIP 68/112).
+ */
 static constexpr uint32_t MAX_BIP125_RBF_SEQUENCE{0xfffffffd};
 
 /** Check whether the sequence numbers on this transaction are signaling opt-in to replace-by-fee,

jonatack commented at 12:51 PM on January 31, 2022: member

If policy is documented, I am wondering if the following diff should also be applied

SGTM

sidhujag referenced this in commit 2dbddc3931 on Feb 1, 2022

DrahtBot locked this on Jan 31, 2023