Discourage CSV as NOP when locktime disable is set & discourage unknown nSequence

JeremyRubin commented at 0:20 am on September 3, 2021: contributor

This patch helps preserve upgradability of CSVs by discouraging the broadcast of transactions which set unknown values in the argument to CSV during script execution and in the nSequence field.

DrahtBot added the label Consensus on Sep 3, 2021

JeremyRubin commented at 1:08 am on September 3, 2021: contributor

will likely need to rebase on top of #22870 and change the failing tests to exempt them from DISCOURAGE_UPGRADABLE_NOPS

bitcoin deleted a comment on Sep 3, 2021

in src/script/interpreter.cpp:613 in 59008cfb0b outdated

605@@ -606,8 +606,11 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
606                     // To provide for future soft-fork extensibility, if the
607                     // operand has the disabled lock-time flag set,
608                     // CHECKSEQUENCEVERIFY behaves as a NOP.
609-                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0)
610+                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0) {
611+                        if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS)

benthecarman commented at 2:23 am on September 3, 2021:

probably worth adding a comment like: // CHECKSEQUENCEVERIFY behaving as NOP, set error appropriately

JeremyRubin force-pushed on Sep 4, 2021

JeremyRubin commented at 4:29 pm on September 4, 2021: contributor

Expanded the semantics to reject in transaction standardness as well.

Discourage CSV as NOP when locktime disable is set 1bd2b15d82

in src/script/interpreter.cpp:614 in 4a63b2ccff outdated

610+                    // If uninterpreted fields are set, also treat as a NOP.
611+                    // This also covers CTxIn::SEQUENCE_FINAL since all bits are
612+                    // set there.
613+                    const CScriptNum extra_fields = nSequence &
614+                        ~(CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | CTxIn::SEQUENCE_LOCKTIME_MASK);
615+                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 || extra_fields != 0) {

benthecarman commented at 8:56 pm on September 4, 2021:

adding this extra_fields check should result in a fork now, no?

JeremyRubin commented at 10:02 pm on September 4, 2021:

You’re totally right, my bad.

We need to handle it separately and still pass it to CheckSequence even though the fields are ignored.

JeremyRubin commented at 10:19 pm on September 4, 2021:

Ok I think the patch i added properly only checks the extra flags during standardness interpreter rules.

JeremyRubin commented at 11:41 pm on September 4, 2021:

I was wrong on that; the prior policy.cpp i had was not exempting final sequence. new version should be (hopefully) correct.

JeremyRubin force-pushed on Sep 4, 2021

Add a policy to reject unknown sequences. 825fa06812

[Tests] Fix tx_valid.json to properly account for discourage rules e5b2a82414

JeremyRubin force-pushed on Sep 5, 2021

JeremyRubin commented at 2:29 am on September 5, 2021: contributor

The plot thickens: I’ve now set the policy transaction level rule to allow 0xffffffff-2 or greater since it seems that 0xffffffff-1 is used to mean “no RBF; but allow absolute locktimes” and 0xffffffff -2 is used to mean “yes RBF; allow absolute locktimes and no relative locktimes” in one script test.

Since folks might use the script test value as canon for that use case, we should treat it as permitted and reserved.

maaku commented at 6:23 am on September 5, 2021: contributor

As the original author of this offending code, I concept-ACK this change if it turns out that nobody is currently using nSequence for any other purpose. However at the time that CHECKSEQUENCEVERIFY was implemented, there were was at least one parasitic protocol (Counterparty? Colored coins? I’m sorry I forget which) using nSequence for other purposes, and in production too IIRC. They wanted to be able to continue doing what they were doing without interruption, and having SEQUENCE_LOCKTIME_DISABLE_FLAG be available for them was the compromise–meaning that it was critically important that setting this flag not result in their transactions being excluded from the mempool.

Now I haven’t seen anyone use nSequence for anything else since then, but then I don’t keep up to date on all the projects in this space. Maybe someone should scan the blockchain and see if SEQUENCE_LOCKTIME_DISABLE_FLAG has even been used in the past 6 years?

[TESTS] Patch Taproot test to use only standard sequences 19557ee684

JeremyRubin force-pushed on Sep 5, 2021

maaku commented at 7:13 am on September 5, 2021: contributor

Alex Mizrahi confirms it was the EPOBC colored coins protocol which was using nSequence at the time:

https://twitter.com/killerstorm/status/1434412779713220610

JeremyRubin commented at 4:04 pm on September 5, 2021: contributor

We could add support for the values listed in https://en.bitcoin.it/wiki/Colored_Coins which are 0x25 and 0x33 irrespective of if the DISABLED flag is set? That way, either your coins are slow to move or you can set the DISABLED flag. This is compatible with long standing presigned transactions that might be using this…

OTOH it’s probably a small enough number that a combination of advertising that new standardness rules will go into effect in a new release (or we could change DISCOURAGE_UPGRADABLE_NOPS as DISCOURAGE_UPGRADABLE_SEQUENCE and do a flag day since it’s just standardness), and contacting miners one off to include a txn in mempool should be sufficient to cover any holdouts.

in test/functional/feature_taproot.py:1391 in 19557ee684 outdated

1387@@ -1389,7 +1388,7 @@ def test_spenders(self, node, spenders, input_counts):
1388             amount = sum(utxo.output.nValue for utxo in input_utxos)
1389             fee = min(random.randrange(MIN_FEE * 2, MIN_FEE * 4), amount - DUST_LIMIT)  # 10000-20000 sat fee
1390             in_value = amount - fee
1391-            tx.vin = [CTxIn(outpoint=utxo.outpoint, nSequence=random.randint(min_sequence, 0xffffffff)) for utxo in input_utxos]
1392+            tx.vin = [CTxIn(outpoint=utxo.outpoint, nSequence=random.randint(0xffffffff-2, 0xffffffff)) for utxo in input_utxos]

JeremyRubin commented at 5:48 pm on September 5, 2021:

@sipa can you confirm that changing this test like this is OK – perhaps you’d prefer to preserve the test as is but set is_standard_tx to false when appropriate?

in src/script/interpreter.cpp:621 in 1bd2b15d82 outdated

617+                    }
618+
619+                    // Only when discouraging un-upgraded semantics,
620+                    // If uninterpreted fields are set, treat and reject as a NOP.
621+                    if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS &&
622+                        (nSequence & ~(CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | CTxIn::SEQUENCE_LOCKTIME_MASK)) != 0) {

ariard commented at 0:16 am on September 6, 2021:

I wonder if this second discouraging check is safe w.r.t to propagation of CSV’ed inputs ?

If the spending wallet has decided to explicitly signal RBF with a range of bits in the uninterpreted fields, this transaction would be rejected as non-standard ? Note, such weird range of bits could be a protocol watermark such as the Lightning obscured commitment number in commitment transactions.

Further, I’ve a second wonder about this discouraging effectiveness w.r.t to hypothetical future semantic. If we assume the upgrade semantics to be well-designed, they will set bit31 to disable bip69 semantic. Doing so, if they need more than the 14 uninterpreted bits, they might reuse the interpreted ones to encode their new field ? If they do so, the SEQUENCE_LOCKTIME_MASK bits might be set and not catch by this discouraging check.

I think those wonders express a trade-off, if we make this discouraging check more efficient to better preserve upgradeability, we’re increasing the risk to silently the broadcast of any Bitcoin software interpreting nSequence for a special purpose ?

JeremyRubin commented at 3:08 am on September 6, 2021:

Well, any time CSV is enabled, RBF is enabled, right? So I’m not sure why they’d add something in the uninterpreted section for that.

I’m not concerned with the future upgrading checks here. We’ll simply need to write new policy/standard rules in the appropriate places to permit those sequences to be used. In the future, we don’t want any sequence in our mempool we don’t understand because that might make us (if un-upgraded) mine a bad block.

If someone is using nSequence for a special purpose they should write a BIP to document it or otherwise communicate the use (e.g., via writing tests and test vectors for core to comply with). Otherwise there are many ways we might accidentally break someones weird software.

ariard commented at 0:39 am on September 6, 2021: member

Good catch!

I’m Concept ACK on the goal as I could foresee nSequence being reused for newer types of lock such as feerate-lock based ones (“If the last N blocks have a median-fee under Y, reject this transaction”). Of course, we could reuse the Taproot annex field to encode such transaction field extension but that would a bit of the waste as we might have those 4 bytes not used.

That said, see my comment, I think we should be far more conservative on the procedural side. This is a strong tightening of our policy which might have safety implications for some class of Bitcoin software. I think one step could be just to move to full-rbf, announce the nSequence field is now reserved and then reject any transaction with a bit set in the remaining ones ? Maybe better than subtle relay restrictions based on nSequence bits patterns.

EDIT: imo this change should combine well with full-rbf

JeremyRubin commented at 3:04 am on September 6, 2021: contributor

I agree this isn’t a simple patch and see given the risks, but note it is policy, so transactions can still be valid in consensus and this behavior could be e.g. conf file disabled safely. I think most discussion should be on the mailing list.

I have actually been sitting on a draft email on the full RBF stuff, letting it stew before I stick my nose in that. The basics of my thought process is that we should really only be using comitted transaction data for consensus purposes, not to signal mempool policies. This would also let us get rid of the annoying RBF 0xfffffffe and 0xfffffffd check since those have no consensus meaning. So I’m for it from a ‘unfuck the code’ perspective.

I think one point that’s also worth noting is that there are two different standardness rules here, one applies to transaction nSeq the other to script CSV argument. The policies can (and do in this PR) differ.

I think it would make sense to merge something like this early in the release cycle (to signal it is going to be released, giving people time to complain) and potentially even activate the standardness rule as a flag day or go full BIP-9 if there is a strong worry about it. It’s a standardness flag, not consensus, so we can do whatever we want. But we should be sure that people have time to complain. The only thing I don’t want to happen is for someone to see this issue, and then start using nSequence in a way that would limit the upgradability because they can. That would be annoying, so I think we’d want to ensure any arguments are presented in good faith (with e.g. testnet or mainnet examples of the behavior being used before we act).

maaku commented at 7:30 am on September 6, 2021: contributor

To be clear, the original code isn’t a mistake. BIP68-disabled nSequence values are very purposefully allowed by the IsStandard rules. nSequence was always intended as a client-interpreted field to be used when merging or updating transactions, and therefore it would be expected to take on unexpected values. However as we all know this original naive idea didn’t work and it took a combination of BIP68 consensus rules and RBF mempool policy to actually implement a useful form of transaction replacement in the mempool. At this time nSequence took on consensus meaning, but there was a lot of push back from 3rd party protocol developers who were already treating nSequence as a 4-byte-per-input private use field. Thus SEQUENCE_LOCKTIME_DISABLE_FLAG was added, and in a way that very purposefully did not impact non-consensus use of nSequence.

That said, bitcoin is now twice as old as when this decision was made. Does anyone actually use nSequence for non-consensus purposes anymore? If not, then this patch makes some sense.

JeremyRubin commented at 9:53 am on September 6, 2021: contributor

The original code is a mistake IMO since the upgradable semantics issue isn’t just in tx relay, but also in the stack values permitted to be passed to CSV. As a brand new opcode, the semantics there can be totally different from the nSeqeuence value in the transaction (e.g., when Disable is set we could add a rule to check a sequence lock in the taproot annex). To underscore that the differing semantics exist: 0x80000000 CSV script can be used with any txin.nSequence you like.

And as @darosior points out on the mailing list, Lightning does use 0x80, so we need to accomodate that. I will push some patches shortly.

maaku commented at 10:04 am on September 6, 2021: contributor

Mistake or no, this again was intentional, if I understand you correctly. Allowing values outside of the checked range to pass to CSV allows for soft-fork upgradeability of the CSV opcode itself. But here the case can be more clearly made to treat such stack values as discouraged-NOPs.

JeremyRubin commented at 10:08 am on September 6, 2021: contributor

Yes – the allowing outside values to pass consensus was intention, no doubt there. That it wasn’t discouraged was a mistake (the discouraging code predates CSV by a bit) because it impacts the ability to actually use new values with CSV in a soft fork. E.g., the lightning folks could be using uninterpreted fields in CSV stack argument for metadata today and then it’d be bricked for upgrades.

Clean up handling of known txin.nSeqeunce types; add type for UNCHECKED_METADATA for Lightning BOLT-3 compatability e8eab74719

ariard commented at 0:04 am on September 9, 2021: member

See my answer on the ml for the higher-level points raised w.r.t to policy design/deployment. If you want to start a more generic thread about that and full-rbf up to you.

sipa commented at 1:31 am on September 9, 2021: member

Is this needed for anything, actually? I saw somewhere a suggestion (can’t remember where) to use a new tx version number if/when new consensus semantics for nSequence are desired. That’s already nonstandard, so anything can be carved out from it. Is there any problem with that (which, I guess, if an approach like this PR is not taken, will inevitably how new nSequence semantics would need to be introduced anyway).

I’m just somewhat hesitant to change relay policy (possibly making it less predictable during a transition period) without actual planned consensus change that relies on it.

JeremyRubin commented at 2:25 am on September 9, 2021: contributor

This PR covers both nSeqeunce and CSV which are distinct. It would mean that whatever new script semantics applied to the unused portions of the CSV argument would have to then also require that transaction version 3 is used. However, there’s not really a sound way that we can specify which transaction version for CSV semantics we are requiring at output creation time (e.g., v2 or v3) so we have to be proactive about fixing that case in my opinion. Concretely, suppose we said if bit 24 is set require tx version == 3, that would be a new consensus rule applying to things that could have been previously happening on tx version == 2, so if we’re ever to make use of the unused bits in CSV for any consensus reason we should do this.

With respect to nSeqeunce itself, I think I agree that the semantics could be done separately for tx version 3 vs version 2. However, if we are going through the trouble of fixing CSV upgradability, it seems reasonable to me to fix both.

JeremyRubin commented at 2:29 am on September 9, 2021: contributor

btw I believe what you saw was the mailing list message from harding which I rebut.

JeremyRubin renamed this:
~~Discourage CSV as NOP when locktime disable is set~~
Discourage CSV as NOP when locktime disable is set & discourage unknown nSequence
on Sep 9, 2021

fanquake removed the label Consensus on Sep 18, 2021

fanquake added the label TX fees and policy on Sep 18, 2021

fanquake added the label Validation on Sep 18, 2021

fanquake added the label Needs Conceptual Review on Sep 18, 2021

in src/policy/policy.cpp:117 in e8eab74719

121-        ((~(CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | CTxIn::SEQUENCE_LOCKTIME_MASK) &
122-                txin.nSequence) != 0)
123-        );
124-        if (seq_is_reserved) {
125+        bool seq_is_reserved_for_upgrade;
126+        switch (static_cast<CTxIn::SEQUENCE_ROOT_TYPE>(txin.nSequence >> CTxIn::SEQUENCE_ROOT_TYPE_SHIFT)) {

darosior commented at 8:52 am on September 22, 2021:

This shift is backward? It should be

0txin.nSequence << CTxIn::SEQUENCE_ROOT_TYPE_SHIFT

If you want to match that the upper 8 bits are 0x80 as specified by BOLT3

JeremyRubin commented at 4:47 pm on September 22, 2021:

Hmm?

0hex(0x80000000 >> 24) == '0x80'

darosior commented at 5:02 pm on September 22, 2021:

Sorry, brainfart

darosior commented at 9:07 am on September 22, 2021: member

~~I think the shift in e8eab747192bd330e67bff1222bb851bc515b134 is wrong.~~ (EDIT: i should do coffee then PR comments, not the other way around) Also it may be clearer to check the presence of a Lightning commitment by just masking by 0xff000000?

0if ((txin.nSequence & UNCHECKED_METADATA_MASK) == 0x80) {
1}
2// Else it must be either 0x0000..00, 0xffff..ff, 0xffff..fe, or 0xffff..fd

Finally note that i mentioned Lightning by mail but there may be other applications relying on these bits that have been free for use for years.. (i don’t have another one on the top of my head)

EDIT: just to be clear i think the goal is sensible, Concept ACK.

in src/script/interpreter.cpp:613 in 1bd2b15d82 outdated

605@@ -606,10 +606,29 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
606                     // To provide for future soft-fork extensibility, if the
607                     // operand has the disabled lock-time flag set,
608                     // CHECKSEQUENCEVERIFY behaves as a NOP.
609-                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0)
610+                    // This also covers CTxIn::SEQUENCE_FINAL since all bits are
611+                    // set in that case.
612+                    if ((nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0) {
613+                        // CHECKSEQUENCEVERIFY behaving as NOP, set error appropriately
614+                        if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS)

glozow commented at 1:01 pm on September 24, 2021:

In 1bd2b15d82ebc80b3c30ab247287717a893cb37a

The approach of using DISCOURAGE_UPGRADABLE_NOPS within an already-upgraded NOP also feels very weird.

in src/test/data/tx_valid.json:272 in e8eab74719

273 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "2147483648 CHECKSEQUENCEVERIFY"]],
274-"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "NONE"],
275+"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "DISCOURAGE_UPGRADABLE_NOPS"],
276 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKSEQUENCEVERIFY"]],
277-"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "NONE"],
278+"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "DISCOURAGE_UPGRADABLE_NOPS"],

glozow commented at 1:10 pm on September 24, 2021:

e5b2a824147c342bfbf2c8b1696afc686c2bfda7

This isn’t “fixing” but loosening this unit test. This shows a misapplication of SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS if you need to remove it in order to make these tests pass. A better approach would be to add a SCRIPT_VERIFY_DISCOURAGE_DISABLE_LOCKTIME_FLAG for this purpose.

glozow commented at 1:14 pm on September 24, 2021: member

I’m mostly repeating what other people said, but implementing this change for nSequence means that Bitcoin Core won’t relay transactions with this bit set. Wallet and application devs may have written software under the assumption that non-consensus usage of nSequence (specifically the disable locktime flag) won’t affect transaction propagation. This assumption has been true for the past few years, but would now suddenly be broken. Thus, it seems unsafe for users if we change this tx relay policy out from underneath them without putting more effort in coordinating with other projects in the ecosystem.

I also don’t think the right solution would be to carve out exemptions for each application using bit masks. It makes the interface unnecessarily complex and harder for everyone to work with.

wrt the hypothetical upgrade to relative timelock semantics, I don’t feel convinced that the other approaches brought up by other contributors (gating on a higher nVersion, repurposing new opcode, using annex, etc.) aren’t adequate options. There seems little value in making this change for OP_CSV arguments. I also have a comment about the approach below.

I agree that it would be unsafe to try to deploy a consensus change to upgrade the relative timelocks gated on the disable locktime flag now. I appreciate that you’ve brought attention to this, but it seems like a more appropriate solution is to document it and make sure we don’t do the unsafe thing, instead of doing an additional unsafe thing just in case we want to do the other unsafe thing in the near future.

DrahtBot commented at 10:21 am on January 27, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24136 (Extract CTxIn::MAX_SEQUENCE_NONFINAL constant, rework BIP 65/68/112 docs by MarcoFalke)

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.

in src/primitives/transaction.h:105 in e8eab74719

100+    /* Sequence number that is rbf-opt-out (BIP 125); relative-timelock-opt-out
101+     * (BIP 68); and absolute-timelock-opt-in. */
102+    static const uint32_t SEQUENCE_VALUE_RESERVED_NO_RBF_YES_CLTV = 0xfffffffe;
103+    /* Sequence number that is rbf-opt-in (BIP 125); and relative-timelock-opt-out
104+     * (BIP 68); and absolute-timelock-opt-in. */
105+    static const uint32_t SEQUENCE_VALUE_RESERVED_YES_RBF_NO_CSV = 0xfffffffd;

maflcko commented at 4:38 pm on January 27, 2022:

How is this different from MAX_BIP125_RBF_SEQUENCE?

JeremyRubin commented at 2:54 am on January 28, 2022:

I think the constant might be the same, but the name is more clear in this BIP that it’s not a MAX, it’s a specific value meant to be matched literally, which implies YES_RBF NO_CSV…

I think it’s OK to duplicate the constant if it has a more clear meaning contextually.

in src/primitives/transaction.h:102 in e8eab74719

 97+    };
 98+
 99+    /* Reserved Values: */
100+    /* Sequence number that is rbf-opt-out (BIP 125); relative-timelock-opt-out
101+     * (BIP 68); and absolute-timelock-opt-in. */
102+    static const uint32_t SEQUENCE_VALUE_RESERVED_NO_RBF_YES_CLTV = 0xfffffffe;

maflcko commented at 4:39 pm on January 27, 2022:

I am adding this as MAX_SEQUENCE_NONFINAL in #24136 (conflicting pull)

JeremyRubin commented at 2:55 am on January 28, 2022:

see above note about “MAX_*”.

I am OK with having a MAX value meant for <=, and a specific value SEQUENCE_VALUE_RESERVED_NO_RBF_YES_CLTV meant for exact checking.

maflcko commented at 5:16 pm on January 27, 2022: member

I couldn’t find any data in this thread that shows how often each type/bit that isn’t special cased in the logic here is used. Without this data, it is not known how much funds this will “confiscate”, if any.

JeremyRubin commented at 2:57 am on January 28, 2022: contributor

yeah I’ll have to do a reindex or something with some instrumenting to detect it… I can do that…

Note that this is really two separate changes, one to CSV and one to nSequence, can make reports for both.

DrahtBot added the label Needs rebase on Jan 31, 2022

DrahtBot commented at 10:17 am on January 31, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

maflcko commented at 12:47 pm on May 12, 2022: member

Closing for now. This has a NACK, is still missing data on how many funds will be confiscated, and has been needing rebase for a few months.

Let us know when it should be reopened.

maflcko closed this on May 12, 2022

bitcoin locked this on Nov 10, 2023

Discourage CSV as NOP when locktime disable is set & discourage unknown nSequence #22871

Conflicts