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

Expanded the semantics to reject in transaction standardness as well.

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.

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?

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

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

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.

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

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.

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.

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.

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.

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.

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.

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.

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

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.

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.

🐙 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".}

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.