[bitcoin-dev] Note on Sequence Lock Upgrades Defect

[bitcoin-dev] Note on Sequence Lock Upgrades Defect

[Jeremy]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

Hi Bitcoin Devs,

I recently noticed a flaw in the Sequence lock implementation with respect
to upgradability. It might be the case that this is protected against by
some transaction level policy (didn't see any in policy.cpp, but if not,
I've put up a blogpost explaining the defect and patching it
https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/

I've proposed patching it here https://github.com/bitcoin/bitcoin/pull/22871,
it is proper to widely survey the community before patching to ensure no
one is depending on the current semantics in any live application lest this
tightening of standardness rules engender a confiscatory effect.

Best,

Jeremy

--
@JeremyRubin <https://twitter.com/JeremyRubin>
<https://twitter.com/JeremyRubin>

[-- Attachment #2: Type: text/html, Size: 2101 bytes --]

Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect

[Jeremy]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

In working on resolving this issue, one issue that has come up is what
sequence values get used by wallet implementations?

E.g., in Bitcoin Core a script test says

BIP125_SEQUENCE_NUMBER = 0xfffffffd  # Sequence number that is rbf-opt-in
(BIP 125) and csv-opt-out (BIP 68)

Are any other numbers currently expected by any wallet software to be
broadcastable with the DISABLE flag set? Does anyone use *this* number? Is
there any advantage of this number v.s. just 0? Do people commonly use
0xfffffffd? 0xfffffffe is special, but it seems the former has the
alternative of either 0 valued sequence lock (1<<22 or 0).

Are there any other sequence numbers that are not defined in a BIP that
might be used somewhere?

Cheers,

Jeremy
--
@JeremyRubin <https://twitter.com/JeremyRubin>
<https://twitter.com/JeremyRubin>


On Fri, Sep 3, 2021 at 8:32 PM Jeremy <jlrubin@mit.edu> wrote:

> Hi Bitcoin Devs,
>
> I recently noticed a flaw in the Sequence lock implementation with respect
> to upgradability. It might be the case that this is protected against by
> some transaction level policy (didn't see any in policy.cpp, but if not,
> I've put up a blogpost explaining the defect and patching it
> https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/
>
> I've proposed patching it here
> https://github.com/bitcoin/bitcoin/pull/22871, it is proper to widely
> survey the community before patching to ensure no one is depending on the
> current semantics in any live application lest this tightening of
> standardness rules engender a confiscatory effect.
>
> Best,
>
> Jeremy
>
> --
> @JeremyRubin <https://twitter.com/JeremyRubin>
> <https://twitter.com/JeremyRubin>
>

[-- Attachment #2: Type: text/html, Size: 4920 bytes --]

Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect

[David A. Harding]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

On Fri, Sep 03, 2021 at 08:32:19PM -0700, Jeremy via bitcoin-dev wrote:
> Hi Bitcoin Devs,
> 
> I recently noticed a flaw in the Sequence lock implementation with respect
> to upgradability. It might be the case that this is protected against by
> some transaction level policy (didn't see any in policy.cpp, but if not,
> I've put up a blogpost explaining the defect and patching it
> https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/

Isn't this why BIP68 requires using tx.version=2?  Wouldn't we just
deploy any new nSequence rules with tx.version>2?

-Dave

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect

[Jeremy]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

BIP 68 says >= 2:
*This specification defines the meaning of sequence numbers for
transactions with an nVersion greater than or equal to 2 for which the rest
of this specification relies on.*
BIP-112 says not < 2
// Fail if the transaction's version number is not set high
// enough to trigger BIP 68 rules.
if (static_cast<uint32_t>(txTo->nVersion) < 2) return false;

A further proof that this needs fix: the flawed upgradable semantic exists
in script as well as in the transaction nSeqeunce. we can't really control
the transaction version an output will be spent with in the future, so it
would be weird/bad to change the semantic in transaction version 3.

--
@JeremyRubin <https://twitter.com/JeremyRubin>
<https://twitter.com/JeremyRubin>


On Sun, Sep 5, 2021 at 7:36 PM David A. Harding <dave@dtrt.org> wrote:

> On Fri, Sep 03, 2021 at 08:32:19PM -0700, Jeremy via bitcoin-dev wrote:
> > Hi Bitcoin Devs,
> >
> > I recently noticed a flaw in the Sequence lock implementation with
> respect
> > to upgradability. It might be the case that this is protected against by
> > some transaction level policy (didn't see any in policy.cpp, but if not,
> > I've put up a blogpost explaining the defect and patching it
> > https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/
>
> Isn't this why BIP68 requires using tx.version=2?  Wouldn't we just
> deploy any new nSequence rules with tx.version>2?
>
> -Dave
>

[-- Attachment #2: Type: text/html, Size: 3756 bytes --]

Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect

[darosior]

[-- Attachment #1: Type: text/plain, Size: 2681 bytes --]

Hi Jeremy,

I think it would be nice to have and suggested something similar (enforce minimality) in the context of
Miniscript a few months ago [0].

However your code:

const bool seq_is_reserved = (txin.nSequence < CTxIn::SEQUENCE_FINAL-2) && (
// when sequence is set to disabled, it is reserved for future use
((txin.nSequence & CTxIn::SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0) ||
// when sequence has bits set outside of the type flag and locktime mask,
// it is reserved for future use.
((~(CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | CTxIn::SEQUENCE_LOCKTIME_MASK) &
txin.nSequence) != 0)
);

Would effectively prevent Lightning Network commitment transactions from relaying. The protocol uses
a hack encoding the commitment transaction numbering in the part of nSequence (and nLockTime)
without consensus meaning. This both sets the LOCKTIME_DISABLE_FLAG and uses bits outside of
the mask.

[0] https://github.com/rust-bitcoin/rust-miniscript/pull/246#issue-671512626
[1] https://github.com/lightningnetwork/lightning-rfc/blob/master/03-transactions.md#commitment-transaction
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Le lundi 6 septembre 2021 à 5:17 AM, Jeremy via bitcoin-dev <bitcoin-dev@lists.linuxfoundation.org> a écrit :

> BIP 68 says >= 2:This specification defines the meaning of sequence numbers for transactions with an nVersion greater than or equal to 2 for which the rest of this specification relies on.
> BIP-112 says not < 2
> // Fail if the transaction's version number is not set high
> // enough to trigger BIP 68 rules.
> if (static_cast<uint32_t>(txTo->nVersion) < 2) return false;
>
> A further proof that this needs fix: the flawed upgradable semantic exists in script as well as in the transaction nSeqeunce. we can't really control the transaction version an output will be spent with in the future, so it would be weird/bad to change the semantic in transaction version 3.
>
> --
> [@JeremyRubin](https://twitter.com/JeremyRubin)https://twitter.com/JeremyRubin
>
> On Sun, Sep 5, 2021 at 7:36 PM David A. Harding <dave@dtrt.org> wrote:
>
>> On Fri, Sep 03, 2021 at 08:32:19PM -0700, Jeremy via bitcoin-dev wrote:
>>> Hi Bitcoin Devs,
>>>
>>> I recently noticed a flaw in the Sequence lock implementation with respect
>>> to upgradability. It might be the case that this is protected against by
>>> some transaction level policy (didn't see any in policy.cpp, but if not,
>>> I've put up a blogpost explaining the defect and patching it
>>> https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/
>>
>> Isn't this why BIP68 requires using tx.version=2? Wouldn't we just
>> deploy any new nSequence rules with tx.version>2?
>>
>> -Dave

[-- Attachment #2: Type: text/html, Size: 5847 bytes --]

Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect

[Antoine Riard]

[-- Attachment #1: Type: text/plain, Size: 4756 bytes --]

Hi Jeremy,

Answering here from #22871 discussions.

I agree on the general principle to not blur mempool policies signaling in
committed transaction data. Beyond preserving upgradeability, another good
argument is to let L2 nodes update the mempool policies signaling their
pre-signed transactions non-interactively. If one of the transaction fields
is assigned mempool semantics, in case of tightening policy changes, you
will need to re-sign or bear the risks of having non-propagating
transactions which opens the door for exploitation by a malicious
counterparty. I think this point is kinda relevant if we have future
cross-layer coordinated safety fixes to deal with a la CVE-2021-31876.

Even further, a set of L2 counterparties would like to pick up divergent
tx-relay/mempool policies, having the signaling fields as part of the
signature force them to come to consensus.

I think we can take the opportunity of p2p packages to introduce a new
field to signal policy. Of course, a malicious tx-relay peer could modify
its content to jam your transaction's propagation but in that case it is
easier to just drop it.

One issue with taking back the `nSequence` field for consensus-semantic
sounds is depriving the application-layer from a discrete, zero-cost
payload (e.g the LN obfuscated commitment number watermark). This might be
controversial as we'll increase the price of such applications if they're
still willingly to relay application specific data through the p2p network
(e.g force them to use a costly OP_RETURN output or payer/payee
interactions to setup a pay-to-contract)

W.r.t flag day activation to smooth policy deployment, I think that's
something we might rely on in the future though we could distinguish few
types of policy deployments :
1) loosening changes (e.g full-rbf/dust threshold removal), a transaction
which was relaying under
the former policy should relay under the new one
2) tightening changes (e.g #22871), a transaction which was relaying under
the former policy
might not relay under the new one
3) new feature introduced (e.g packages), a transaction is offered a new
mode of relay

I think 1) doesn't need that level of ecosystem coordination as
applications/second-layers should always benefit from such changes. Maybe
with the exception of full-rbf, where we have historical 0-conf softwares,
with (broken) security assumptions made on the opt-out RBF mechanism. Same
with 3), better to have new features deployed gradually, a flag day
activation day in this case won't mean that all higher stacks will jump to
use package-relay ?

Where a flag day might make sense would be for 2) ? It would create a
higher level of commitment by the base layer software instead of a pure
communication on the ML/GH, which might not be concretized in the announced
release due to slow review process/feature freeze/rebase conflicts...
Reversing the process and asking for Bitcoin applications/higher layers to
update first might get us in the trap of never doing the change, as someone
might have a small use-case in the corner relying on a given policy
behavior.

That said, w.r.t to the proposed policy change in #22871, I think it's
better to deploy full-rbf first, then give a time buffer to higher
applications to free up the `nSequence` field and finally start to
discourage the usage. Otherwise, by introducing new discouragement waivers,
e.g not rejecting the usage of the top 8 bits, I think we're moving away
from the policy design principle we're trying to establish (separation of
mempool policies signaling from consensus data)

Le ven. 3 sept. 2021 à 23:32, Jeremy via bitcoin-dev <
bitcoin-dev@lists.linuxfoundation.org> a écrit :

> Hi Bitcoin Devs,
>
> I recently noticed a flaw in the Sequence lock implementation with respect
> to upgradability. It might be the case that this is protected against by
> some transaction level policy (didn't see any in policy.cpp, but if not,
> I've put up a blogpost explaining the defect and patching it
> https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/
>
> I've proposed patching it here
> https://github.com/bitcoin/bitcoin/pull/22871, it is proper to widely
> survey the community before patching to ensure no one is depending on the
> current semantics in any live application lest this tightening of
> standardness rules engender a confiscatory effect.
>
> Best,
>
> Jeremy
>
> --
> @JeremyRubin <https://twitter.com/JeremyRubin>
> <https://twitter.com/JeremyRubin>
> _______________________________________________
> bitcoin-dev mailing list
> bitcoin-dev@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
>

[-- Attachment #2: Type: text/html, Size: 6577 bytes --]

Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect

[Jeremy]

[-- Attachment #1: Type: text/plain, Size: 7077 bytes --]

See the current patchset proposed:
https://github.com/bitcoin/bitcoin/pull/22871/commits

Two things are happening that are separate:

1) Fixing the semantics of arg in <arg> OP_CHECKSEQUENCEVERIFY
2) Fixing the semantics on nSequence in each tx input

There is no sense in conditioning part 1 on RBF or anything else, since
it's only loosely related to 2. I think it should be a class-2 rollout as
you describe above since it's a rule tightening.

For part 2, I think the way the patches handle it currently (which is
defining 1 byte type prefix followed by 3 bytes application data) is
sufficient for immediate deployment.

I agree with you that a class-2 rollout might be appropriate for it, but
that can be followed by removing the SEQUENCE_ROOT_TYPE::SPECIAL field
later as a class-1 rollout. However, so long as it's not being used for any
particular constants, there is no need to deallocate
SEQUENCE_ROOT_TYPE::SPECIAL tag as long as no new use case must overlap
it's range.

With respect to the SEQUENCE_ROOT_TYPE::UNCHECKED_METADATA, it is in fact
*not* mempool data, but is a special type of metadata which is required for
the counterparty to efficiently respond to a unilateral channel closure (see
bolt-3 This obscures the number of commitments made on the channel in the
case of unilateral close, yet still provides a useful index for both nodes
(who know the payment_basepoints) to quickly find a revoked commitment
transaction.)

I understand wanting to remove full-rbf, but I think that fixing the
upgradability of sequences is much less controversial among the
userbase and worth doing expediently. That part 1 is doable now -- albeit
as a class 2 -- means that it would not be unreasonable to bundle parts 1
and 2 so that we don't double burden the community with an upgrade effort.
Further, RBF can be disabled on a purely ad-hoc node-by-node policy layer,
whereas this restriction requires more community coordination/awareness.
--
@JeremyRubin <https://twitter.com/JeremyRubin>
<https://twitter.com/JeremyRubin>


On Wed, Sep 8, 2021 at 5:03 PM Antoine Riard <antoine.riard@gmail.com>
wrote:

> Hi Jeremy,
>
> Answering here from #22871 discussions.
>
> I agree on the general principle to not blur mempool policies signaling in
> committed transaction data. Beyond preserving upgradeability, another good
> argument is to let L2 nodes update the mempool policies signaling their
> pre-signed transactions non-interactively. If one of the transaction fields
> is assigned mempool semantics, in case of tightening policy changes, you
> will need to re-sign or bear the risks of having non-propagating
> transactions which opens the door for exploitation by a malicious
> counterparty. I think this point is kinda relevant if we have future
> cross-layer coordinated safety fixes to deal with a la CVE-2021-31876.
>
> Even further, a set of L2 counterparties would like to pick up divergent
> tx-relay/mempool policies, having the signaling fields as part of the
> signature force them to come to consensus.
>
> I think we can take the opportunity of p2p packages to introduce a new
> field to signal policy. Of course, a malicious tx-relay peer could modify
> its content to jam your transaction's propagation but in that case it is
> easier to just drop it.
>
> One issue with taking back the `nSequence` field for consensus-semantic
> sounds is depriving the application-layer from a discrete, zero-cost
> payload (e.g the LN obfuscated commitment number watermark). This might be
> controversial as we'll increase the price of such applications if they're
> still willingly to relay application specific data through the p2p network
> (e.g force them to use a costly OP_RETURN output or payer/payee
> interactions to setup a pay-to-contract)
>
> W.r.t flag day activation to smooth policy deployment, I think that's
> something we might rely on in the future though we could distinguish few
> types of policy deployments :
> 1) loosening changes (e.g full-rbf/dust threshold removal), a transaction
> which was relaying under
> the former policy should relay under the new one
> 2) tightening changes (e.g #22871), a transaction which was relaying under
> the former policy
> might not relay under the new one
> 3) new feature introduced (e.g packages), a transaction is offered a new
> mode of relay
>
> I think 1) doesn't need that level of ecosystem coordination as
> applications/second-layers should always benefit from such changes. Maybe
> with the exception of full-rbf, where we have historical 0-conf softwares,
> with (broken) security assumptions made on the opt-out RBF mechanism. Same
> with 3), better to have new features deployed gradually, a flag day
> activation day in this case won't mean that all higher stacks will jump to
> use package-relay ?
>
> Where a flag day might make sense would be for 2) ? It would create a
> higher level of commitment by the base layer software instead of a pure
> communication on the ML/GH, which might not be concretized in the announced
> release due to slow review process/feature freeze/rebase conflicts...
> Reversing the process and asking for Bitcoin applications/higher layers to
> update first might get us in the trap of never doing the change, as someone
> might have a small use-case in the corner relying on a given policy
> behavior.
>
> That said, w.r.t to the proposed policy change in #22871, I think it's
> better to deploy full-rbf first, then give a time buffer to higher
> applications to free up the `nSequence` field and finally start to
> discourage the usage. Otherwise, by introducing new discouragement waivers,
> e.g not rejecting the usage of the top 8 bits, I think we're moving away
> from the policy design principle we're trying to establish (separation of
> mempool policies signaling from consensus data)
>
> Le ven. 3 sept. 2021 à 23:32, Jeremy via bitcoin-dev <
> bitcoin-dev@lists.linuxfoundation.org> a écrit :
>
>> Hi Bitcoin Devs,
>>
>> I recently noticed a flaw in the Sequence lock implementation with
>> respect to upgradability. It might be the case that this is protected
>> against by some transaction level policy (didn't see any in policy.cpp, but
>> if not, I've put up a blogpost explaining the defect and patching it
>> https://rubin.io/bitcoin/2021/09/03/upgradable-nops-flaw/
>>
>> I've proposed patching it here
>> https://github.com/bitcoin/bitcoin/pull/22871, it is proper to widely
>> survey the community before patching to ensure no one is depending on the
>> current semantics in any live application lest this tightening of
>> standardness rules engender a confiscatory effect.
>>
>> Best,
>>
>> Jeremy
>>
>> --
>> @JeremyRubin <https://twitter.com/JeremyRubin>
>> <https://twitter.com/JeremyRubin>
>> _______________________________________________
>> bitcoin-dev mailing list
>> bitcoin-dev@lists.linuxfoundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
>>
>

[-- Attachment #2: Type: text/html, Size: 11094 bytes --]