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

From: Jeremy <jlrubin@mit•edu>
To: Antoine Riard <antoine.riard@gmail•com>
Cc: Bitcoin Protocol Discussion <bitcoin-dev@lists•linuxfoundation.org>
Subject: Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
Date: Wed, 8 Sep 2021 18:04:53 -0700	[thread overview]
Message-ID: <CAD5xwhj3N7aK-Of1+-DykH_XRbKBs32E4=uu81XmJhTx5yYZ7A@mail.gmail.com> (raw)
In-Reply-To: <CALZpt+Fk=_3=Hb_u4OptAwHKaG6R=+6igDeLQESQ_u_QbBQePg@mail.gmail.com>

[-- 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 --]