Extract CTxIn::MAX_SEQUENCE_NONFINAL constant, rework BIP 65/68/112 docs #24136
Conversation
|
Sorry for the force push. Clarified tx.version >= 2, requested in comment https://github.com/bitcoin/bitcoin/pull/7184/files#r47233948 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
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.
+ * MA
8000
X_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_HAlso: s/All inputs/Using all inputs/
| /* Below flags apply in the context of BIP 68*/ | ||
| /* If this flag set, CTxIn::nSequence is NOT interpreted as a | ||
| * relative lock-time. */ | ||
| // Below flags apply in the context of BIP 68. BIP 68 requires the tx |
There was a problem hiding this comment.
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. */There was a problem hiding this comment.
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.
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, |
SGTM |
Extracting the constant makes it possible to attach documentation to it.
Also, rework the docs for the other "sequence constants".