Discourage CSV as NOP when locktime disable is set & discourage unknown nSequence (validation, tx fees and policy)


Host: glozow  -  PR author: JeremyRubin

The PR branch HEAD was e8eab74719 at the time of this review club meeting.


  • Bitcoin has various “allow any” consensus rules that can be repurposed in future soft forks, such as upgradable NOPs and upgradable witness versions. In the future, if we repurpose a NOP opcode (e.g. NOP4 as OP_CTV) or define new semantics under a witness version (e.g. witness version 1 for taproot), nodes that don’t upgrade immediately will still be able to stay in consensus when the new rules activate.

  • Bitcoin Core’s policy discourages usage of upgradable NOPs, witness versions, taproot leaf versions, etc. This prevents nodes from accepting to-be-invalid transactions into their mempool prior to activation and miners from building consensus-invalid blocks if they don’t upgrade.

  • BIP68 and BIP112 introduced consensus-enforced semantics on the nSequence field of a transaction input and the OP_CHECKSEQUENCEVERIFY opcode to enable relative lock-time spending constraints.

    • It specifies the most significant bit of the nSequence field as the disable locktime flag such that, if that bit is set, nodes do not apply consensus meaning to the sequence number.

    • The disable locktime flag was documented as leaving room for “future extensibility,” i.e. we might create new relative locktime rules where the disable locktime flag is set.

    • However, as there was at least one application in the Bitcoin ecosystem using the nSequence field - including the 31st bit - use of this bit was intentionally not discouraged in policy.

  • PR #22871 proposes a policy change to discourage use of this bit in nSequence numbers and as arguments to OP_CSV in the interpreter.

    • The PR author has also written a blog post about this unexpected behavior.

    • The author of the BIP112 implementation explained why this policy was not added originally and noted that this change makes sense now only if there are no more applications using the disable locktime flag for non-consensus purposes in their nSequence numbers, as it would prohibit their transactions from propagating.

  • The PR author posted to the mailing list soliciting feedback on this proposal.

    • Dave Harding noted on the mailing list that upgrades to relative timelock semantics would also be gated on a different nVersion number.

    • Pieter Wuille commented that, since nVersion numbers greater than 2 are discouraged in policy, this method of upgrading relative timelocks is preserved without discouraging use of the locktime disable flag.

    • Antoine Poinsot noted on the mailing list that the Lightning Network protocol uses nSequence numbers to encode commitment transaction numbers, including the disable locktime flag.


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. Can you summarize the changes being proposed in this PR?

  3. What is the difference between policy and consensus rules?

  4. Why do we discourage the usage of upgradable NOPs in policy? How is this implemented? (Hint: grep for SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS in src/script/interpreter.cpp).

  5. What are some reasons to discourage use of the locktime disable flag in policy?

  6. What are some reasons not to discourage use of the locktime disable flag in policy?

  7. What do you think of the comment and response about upgrading the relative timelock semantics by increasing the nVersion number?

  8. Do you think this change (discouragement of setting the most significant bit) should be applied to nSequence numbers, CSV values, both, or neither? Why?

  9. Why is this commit, which removes the SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS script verification flag from static OP_CSV tests, needed?

  10. What do you think of the approach of reusing the SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS script verification flag to discourage use of the locktime disable flag?

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <jnewbery> hi!
  317:00 <ziggie> hi
  417:00 <dunxen> hi!
  517:00 <glozow> hello friends! welcome to PR review club!
  617:00 <willcl_ark> hellooo
  717:00 <dunxen> yay, policy!
  817:00 <glozow> we’re looking at https://github.com/bitcoin/bitcoin/pull/22871 today
  917:00 <lightlike> hi
 1017:00 <shoryak> hii
 1117:01 <glozow> did anyone get a chance to review the PR? y/n
 1217:01 <jeremyrubin> gm
 1317:01 <svav> hi
 1417:01 <ziggie> n
 1517:01 <jeremyrubin> y
 1617:01 <lightlike> y
 1717:01 <dunxen> not in depth
 1817:01 <willcl_ark> y
 1917:02 <glozow> o! is it anyone’s first time btw? :)
 2017:02 <ziggie> yes mine
 2117:02 <shoryak> yes mine first time
 2217:02 <jnewbery> welcome ziggie! welcome shoryak!
 2317:02 <glozow> OOO welcome ziggie and shoryak!!
 2417:03 <jeremyrubin> 👋 shoryak
 2517:03 <glozow> https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/ i might also link this blog post which was written by the PR author
 2617:03 <svav> To the new guys, how did you find out about PR club?
 2717:03 <jnewbery> feel free to ask questions any time. There are some tips on attending your first review club meeting here: https://bitcoincore.reviews/your-first-meeting :)
 2817:03 <jeremyrubin> note the blog post is out of date to the PR w.r.t. the solution
 2917:03 <schmidty> hi
 3017:04 <glozow> and i also have some notes here: https://bitcoincore.reviews/22871
 3117:04 <glozow> okie dokie: Can anyone who reviewed the PR summarize the changes being proposed?
 3217:04 <Azorcode> Hello everyone
 3317:05 <willcl_ark> We want to modify mempool acceptance so that un-upgraded nodes from the future might not accidentally create consensus-invalid blocks using transactions that entered their mempool
 3417:06 <glozow> willcl_ark: yes! to be more specific though, what consensus rule(s) would these un-upgraded nodes be missing?
 3517:07 <svav> NOP - SOmething Op Code what does the N stand for?
 3617:07 <willcl_ark> If we tightened the rules (with a soft fork), then they would still blindly accept future-invalid transactions as valid
 3717:07 <dunxen> consensus rules around the unused bits of nSequence?
 3817:08 <willcl_ark> NOP = No Operation to me :)
 3917:08 <sipa> https://en.wikipedia.org/wiki/NOP_(code)
 4017:08 <lightlike> they could use OP_CSV in combination with the disablelocktime flag freely, thinking it does nothing, while the upgraded nodes enforce some new consensus rules
 4117:08 <glozow> lightlike: exactly!
 4217:09 <jeremyrubin> yep that's one part; the PR also does another thing too
 4317:09 <Sachin> Could it also be that the nSequence triggers other rules as well?
 4417:09 <jeremyrubin> yep!
 4517:10 <glozow> yes, we should highlight that this is for nSequence numbers and for arguments to OP_CSV
 4617:10 <ziggie> so are we going to declare some transactions as non-standard with this PR ?
 4717:11 <glozow> ziggie: correct, it means transactions that set the locktime disable flag are now non-standard
 4817:11 <glozow> (with the changes in this PR, i mean)
 4917:11 <ziggie> glozow thanks
 5017:11 <glozow> quick conceptual question: What is the difference between policy and consensus rules?
 5117:12 <willcl_ark> So it's the case that currently people can pass (or store) arbitrary data for CSV, as long as they've disable it, and it just gets OP_NOP-ed (and they use the data for their own purpose)?
 5217:12 <shoryak> is policy for mempool acceptance and consensus for block acceptance ?
 5317:13 <jeremyrubin> it's actually a bit worse than that willcl_ark
 5417:13 <Sachin> willcl_ark I believe that is what this PR implements. It should've been the case before
 5517:13 <jeremyrubin> it's that any data that is not defined to be interpreted in the CSV argument or nSequence (which are, tho similar, very different fields) are uninterpreted
 5617:13 <dunxen> policy is around what we accept into our mempool and propagate through the network, some policy invalid txs might still be consensus valid (unless some future soft fork makes that not the case)
 5717:14 <willcl_ark> ah
 5817:14 <ziggie> willcl_ark so a clever/aother way of doing a OP_RETRUN?
 5917:14 <jeremyrubin> doesn't really matter if disabled/enabled, but disabled is a wide set of ones with no rules
 6017:14 <glozow> shoryak: dunxen: correcto! policy is only applied to unconfirmed transactions we're evaluating for our mempool. and policy is strictly stricter than consensus.
 6117:15 <willcl_ark> It's a nice model though, to have a stricter mempool which you "know" you can freely select only valid transactions from when constructing a block.
 6217:15 <jeremyrubin> ziggie yeah, the Lightning Network currently does this using the nSequence (not CSV arg) field
 6317:15 <glozow> next question: Why do we discourage the usage of upgradable NOPs in policy? How is this implemented?
 6417:15 <ziggie> policy = declare a tx as standard, consensus= declare tx as valid ?
 6517:16 <jeremyrubin> ziggie https://github.com/lightningnetwork/lightning-rfc/blob/master/03-transactions.md#commitment-transaction
 6617:17 <Sachin> To protect users from potentially creating transactions that are later restricted by a softfork, making them harder (or impossible) to spend
 6717:17 <lightlike> is the term "policy" only used in the context of tx acceptance? Or could it be used for anything that is our own business and doesn't violate consensus, e.g. p2p stuff etc.?
 6817:18 <michaelfolkson> hi
 6917:18 <willcl_ark> +1 Sachin
 7017:18 <jeremyrubin> Sachin are there other users who would have issues other than spenders?
 7117:19 <glozow> lightlike: do you mean like, if we have policies that aren't about transaction validation?
 7217:20 <willcl_ark> I also associate "policy" with my local node settings, which might be configurable to some degree whilst still remaining consensus-valid. Although, as most nodes run the same (default) policy rules, if you want your tx to be propagated then you want your policy to match others' policies too
 7317:20 <glozow> we have limits on the number of ancestor/descendants transactions can have in our mempool, which is policy but not necessarily related to the tx itself
 7417:20 <lightlike> i don't know, we have many local rules that are similar, i was just asking if they would be called policy, or whether that term is usually reserved for mempool acceptance issues
 7517:21 <glozow> we sometimes configure our node with transaction rules that aren't really policy-related, like our wallet maximum fee
 7617:21 <Sachin> jeremyrubin I guess receivers could unknowingly receive to a script that they dont fully understand?
 7717:21 <glozow> i also wouldn't consider something like "i will only keep 100 orphan transactions" policy
 7817:21 <jeremyrubin> Sachin well not quite, if you make your own addresses
 7917:22 <jeremyrubin> think about how the issue would effect miners
 8017:22 <lightlike> jeremyrubin: also miners that have not upgraded might mine blocks that are seen as invalid by upgraded nodes and therefore not accepted
 8117:22 <jeremyrubin> lightlike yep! you got it
 8217:22 <Sachin> Ah, thank you
 8317:23 <glozow> good answers :)
 8417:24 <glozow> ok let's now go over some pros and cons of this PR, for the sake of deciding whether we want to "concpet ack" it
 8517:24 <willcl_ark> Would it be fair to say that a lot of policy rules are local anti-DOS measures?
 8617:24 <michaelfolkson> Customary Suhas StackExchange link share: https://bitcoin.stackexchange.com/questions/100317/what-is-the-difference-between-policy-and-consensus-when-it-comes-to-a-bitcoin-c
 8717:24 <glozow> What are some reasons to discourage use of the locktime disable flag in policy? What are some reasons NOT to discourage use of the locktime disable flag in policy?
 8817:25 <glozow> willcl_ark: yes, a lot of them are
 8917:25 <Sachin> willcl_ark many, but some are also for protecting users and making soft forks safer
 9017:25 <Sachin> and miners
 9117:25 <willcl_ark> (except the ones we are discussing today!)
 9217:25 <michaelfolkson> From Core's perspective policy is defined here: https://github.com/bitcoin/bitcoin/tree/master/src/policy
 9317:25 <jeremyrubin> glozow i think it makes sense to talk about the nSequence and CSV commits seprately in terms of pro/con
 9417:25 <willcl_ark> right
 9517:25 <sipa> michaelfolkson: not solely
 9617:25 <michaelfolkson> But as a user I guess you could broaden the definition of policy
 9717:25 <michaelfolkson> sipa: Oh cool
 9817:26 <glozow> i recently also came across this explanation of policy: https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a#policy (the rest of the document is an interest read too)
 9917:27 <michaelfolkson> Looks good :)
10017:27 <glozow> jeremyrubin: fair enough, i'll rephrase. We have 4 questions to talk about: what are the pros/cons of discouraging the use of the locktime disable flag in nSequence/CSV args?
10117:28 <jeremyrubin> 🙏
10217:29 <dunxen> i saw that lightning uses the disable bit and some unused bits for encoding the commitment number, so that would prevent commitment transactions being broadcast?
10317:29 <lightlike> a con would be some people out there already using it for something. this seems to be the case for the nSequence/lightning?
10417:30 <glozow> dunxen: indeed. discouraging something in policy means that Bitcoin Core nodes will not relay them, so we have to be careful not to exclude entire applications' transactions if they rely on being able to use nSequence as they please
10517:31 <Sachin> Would this pr truly prevent commitment txs from being broadcast? or only commitment Txs which use CSV
10617:32 <svav> What is Bitcoin locktime please?
10717:33 <ziggie> svav makes a transaction not valid until a specific time in the future is reached
10817:33 <willcl_ark> I think of sanket1729's questions posted before the meeting by jeremy at this point? Whilst the interpreter changes seem logical, do we really envision further overloading these OP codes with more rules in the future?
10917:33 <svav> Thanks ziggie!
11017:34 <glozow> svav: "locktime" generally refers to spending conditions that are like "this UTXO cannot be spent until X time in the future"
11117:34 <jeremyrubin> svav confusingly there's a concept of a 'lock time' and a nLockTime, which is a specific kind of 'lock time'
11217:34 <jeremyrubin> always good to clarify which one is being discussed
11317:34 <glozow> CSV is for relative locktime, so the time constraint is based on the time/#blocks between the time the UTXO is confirmed and when it's spent
11417:34 <ziggie> so we have to be very conscious in the feature, introducing something like BIP 68, allowing something to be disabled?
11517:34 <ziggie> *future
11617:35 <lightlike> I also didn't understand the effect on the lightnig network: would this PR basically shut down the LN in its current form, could there be some exceptions for the specific LN use case?
11717:35 <ziggie> because people will use it for their own purposes, and nobody knows I they will
11817:35 <michaelfolkson> Sachin: Commitment transactions only go onchain if close isn't cooperative. Otherwise a simple 2-of-2
11917:35 <jeremyrubin> ziggie i would go as far as to say if we can't make this policy change now, we can never use these bits ever again in the future
12017:35 <jeremyrubin> (for a consensus purpose)
12117:36 <jeremyrubin> the PR has no impact on the lightning network, does anyone know why
12217:36 <michaelfolkson> (I think... doubting myself now). So only affects uncooperative closes and justice transactions
12317:36 <glozow> jeremyrubin: i disagree with that. we can still use these bits in the future, gated on a different nVersion number, which doesn't have the "not discouraged in policy" problem
12417:36 <jeremyrubin> glozow i don't think that's true actually
12517:37 <glozow> michaelfolkson: to me, it seems _more_ dangerous for Bitcoin Core tx relay policy to only impact non-cooperative Lightning Network closes
12617:38 <glozow> or just in general, tx relay being inconsistent like that
12717:38 <jeremyrubin> the pr has no effect on lightning network closes?
12817:38 <michaelfolkson> glozow: Well yeah those are the ones which are most important. Cooperative closes aren't emergency or time pressured
12917:38 <glozow> this might be a good time to bring in sanket's suggested question: Does it make sense to change something for an unplanned upgrade?
13017:39 <sipa> 13:35:44 < jeremyrubin> ziggie i would go as far as to say if we can't make this policy change now, we can never use these bits ever again in the future
13117:39 <sipa> i don't understand this
13217:40 <sipa> why can't the policy change be made when a use for the bits is being rolled out?
13317:40 <jeremyrubin> So there are a couple reasons
13417:40 <michaelfolkson> glozow: Upgradeability is a nice to have if there are no downsides (even if you can't imagine what the upgrade will be). But there does appear to be downsides with this
13517:41 <jeremyrubin> A) When you make an upgrade in the future, you want un-upgraded mining nodes to not accept invalid txns to the mempool
13617:41 <michaelfolkson> I personally can't imagine what the future upgrade would be here (though interested in ideas)
13717:41 <sipa> jeremyrubin: sure, it'd need to time - but consensus changes take time anyway
13817:41 <jeremyrubin> so in order for that to be the case, you want the tightening of rules to occur far in the past so that there's plenty of upgrade time
13917:42 <jeremyrubin> The longer you wait, the more likely it is that more metadata use cases proliferate too, so it's better to make expectations clearer
14017:42 <glozow> indeed, this is a very tight restriction on our tx relay policy. i don't think the downsides are even very clear right now; efforts should be made to understand whether applications are relying on an assumption that they can use the 31st bit of nSequence
14117:42 <jeremyrubin> W.r.t. TX Version, it's not a sound upgrade path for the reasons i laid out here https://github.com/bitcoin/bitcoin/pull/22871#issuecomment-915709410
14217:42 <willcl_ark> But that could boil down to "because we didn't do this 5 years ago we can't do that now", right?
14317:42 <dunxen> i'm struggling to see what was changed so this PR does not affect unilateral closes on lightning
14417:43 <jeremyrubin> Essentially, you can't really control for tx version at the script level (IMO) to fix this.
14517:43 <jeremyrubin> dunxen https://github.com/bitcoin/bitcoin/blob/e8eab747192bd330e67bff1222bb851bc515b134/src/policy/policy.cpp
14617:44 <jeremyrubin> see case CTxIn::SEQUENCE_ROOT_TYPE::UNCHECKED_METADATA
14717:45 <jeremyrubin> that exempts the LN's bolt defined use case of 0x80-------- nSequences as being just metadata with no meaning for consensus
14817:45 <sipa> jeremyrubin: hmm, and couldn't a different opcode be used instead?
14917:45 <glozow> it also seems like an abstraction violation to force our policy code to enumerate the types of LN sequences...
15017:45 <sipa> (a new OP_CSV2, say)
15117:45 <michaelfolkson> willcl_ark: I think if there was a proposed upgrade in future that people wanted the fact that this change wasn't done 5 years ago wouldn't impact it. Of course it would take longer than if we did the change now (but we don't know what the upgrade would be now if it ever arrives)
15217:46 <jeremyrubin> glozow well we already have policy exemptions for LN in the mempool, this is similar. any app can use the seq metadata field
15317:46 <glozow> are there other applications that use nSequence? have we observed 0 usage of the disable locktime flag for a few years?
15417:46 <jeremyrubin> sipa a new CSV opcode would work; but i also think the impacts on the CSV arg are lesser than the nSequence field
15517:47 <jeremyrubin> I think the nSequence field semantics as it pertains to v1 CSV have to be fixed given that v1 CSV is tx.nVersion >= 2
15617:47 <sipa> jeremyrubin: right, but for the nSequence field, new semantics can be introduced with tx version
15717:47 <sipa> ?
15817:47 <jeremyrubin> nope I do not think so
15917:47 <jeremyrubin> since it would allow stealing funds prematurely from a csv were the semantics to change
16017:47 <glozow> jeremyrubin: could you explain why, for the nSequence field specifically, it wouldn't be sufficient to gate on a new nVersion number?
16117:48 <jeremyrubin> Yes
16217:48 <jeremyrubin> Imagine that *today* i create an output which is IF <2 years> CSV <backuip> Checksig ELSE <normal> Checksig ENDIF
16317:48 <glozow> ok since we're running low on time, next question for the review clubbies is: Do you think this change (discouragement of setting the most significant bit) should be applied to nSequence numbers, CSV values, both, or neither? Why?
16417:48 <lightlike> has someone done analysis of the blockchain if (and with which values) nSequence is currently used?
16517:49 <jeremyrubin> and then you switch to tx.nVersion 3
16617:49 <jeremyrubin> my output is spendable in tx.nVersion 3
16717:49 <jeremyrubin> and if nVersion 3 undefines the CSV semantics as they were prior
16817:49 <jeremyrubin> then you'd trigger backup prematurely
16917:49 <glozow> lightlike: good question. I would also feel much more comfortable reasoning about this PR if there was such an analysis done
17017:49 <jeremyrubin> so whatever new semantic for CSV exists in nversion 3 has to be compatible with old scripts
17117:50 <jeremyrubin> you could do something like nVersion 3 must only be segwit v2 (not v1, v0) and that might work? But that hurts fungibility
17217:51 <Sachin> jeremyrubin Sorry to backtrack but I don't understand why this doesnt affect LN commitment txs
17317:51 <jeremyrubin> Sachin LN commitment txns specifically use 0x80 prefixed sequence numbers
17417:51 <glozow> why wouldn't a new semantic for CSV scripts be compatible with old scripts?
17517:51 <jeremyrubin> the PR applies no rules when the top bits are exactly 0x80
17617:51 <Sachin> ah, thank you
17717:52 <jeremyrubin> because if an old output could be spent in tx.nVersion 3 with different semantics, it would disrupt the timing of that old output
17817:52 <michaelfolkson> Meh not convinced on the fungibility hurt, every SegWit version introduces new rules that supposedly hurt fungibility (of course they do when they are first introduced but that's the cost of a new SegWit version)
17917:53 <jeremyrubin> michaelfolkson it's different, you're talking about fungibility from privacy v.s. cospendable fungibility
18017:53 <jeremyrubin> one is much worse than the other
18117:53 <michaelfolkson> Hmm ok
18217:53 <jeremyrubin> glozow so the backup clause could either be made available prematurely or too late (or never) under tx.nVersion 3
18317:53 <glozow> cospendability? you mean spending a "old CSV" and "new CSV" in the same tx?
18417:54 <jeremyrubin> nope, it would be any pre segwit v2 (not v1, v0) output in the same tx as new (maybe could limit to leaf versions in v1)
18517:54 <jeremyrubin> too late is fine, never is fine (just use tx nversion 2), but too early breaks the spec
18617:56 <jeremyrubin> it maybe makes it more intuitive to think about it like "could we define signatures as always being valid in tx.nVersion 3?"
18717:56 <jeremyrubin> we cannot do that, because then any miner could mine a block stealing all the coins\
18817:57 <jeremyrubin> we need to preserve some semantics across tx version for output types.
18917:57 <jeremyrubin> and nSequence and CSV arg are a part of that, or else it may premit theft
19017:58 <glozow> afaik the current CSV semantics allow any nversion >=2
19117:58 <jeremyrubin> yep; that's part of why this problem exists
19217:58 <dunxen> glozow: i think i'd need to look at this more closely before I have an answer for your last question haha
19317:59 <jeremyrubin> i think harding thought it was just nVersion == 2
19418:00 <jeremyrubin> but because it's >= 2, the rules need to stay largely the same on all future versions unless we do the tx.nVersion blocks inputs that are not segwit v2 or newer or something
19518:00 <glozow> haha no problem. i hope people have some food for thought around how to reason about this PR conceptually
19618:00 <lightlike> so you think it was a mistake not to just use nVersion=2?
19718:01 <glozow> given that BIP68 uses nVersion >= 2, i'd say there isn't really a problem with having an "old CSV" input inside a transaction with version 3
19818:01 <jeremyrubin> well there isn't if you don't change nSequence semantics :)
19918:01 <jeremyrubin> but that's the crux of this, which is preserving upgradability
20018:02 <glozow> looks like we're out of time - the final 2 questions are around the approach of the PR, since it requires a lot of unit tests to be loosened
20118:02 <glozow> they are left as exercise to the reader i guess :P
20218:02 <glozow> #endmeeting