The PR branch HEAD was e8eab74719 at the time of this review club meeting.
Notes
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.
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.
Can you summarize the changes being proposed in this PR?
What is the difference between policy and consensus rules?
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).
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?
What do you think of the
comment and
response about
upgrading the relative timelock semantics by increasing the nVersion number?
Do you think this change (discouragement of setting the most significant bit) should be applied
to nSequence numbers, CSV values, both, or neither? Why?
Why is this
commit,
which removes the SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS script verification flag from static
OP_CSV tests, needed?
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?
<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
<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
<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)?
<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
<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)
<glozow> shoryak: dunxen: correcto! policy is only applied to unconfirmed transactions we're evaluating for our mempool. and policy is strictly stricter than consensus.
<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.
<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.?
<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
<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
<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
<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?
<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?
<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?
<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
<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?
<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?
<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
<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
<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
<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
<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)
<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?
<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)
<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