public inbox for bitcoindev@googlegroups.com
 help / color / mirror / Atom feed

* [bitcoin-dev] Note on Sequence Lock Upgrades Defect
@ 2021-09-04  3:32 Jeremy
  2021-09-05  3:19 ` Jeremy
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Jeremy @ 2021-09-04  3:32 UTC (permalink / raw)
  To: Bitcoin development mailing list

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

• * Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
    2021-09-04  3:32 [bitcoin-dev] Note on Sequence Lock Upgrades Defect Jeremy
  @ 2021-09-05  3:19 ` Jeremy
    2021-09-06  2:35 ` David A. Harding
    2021-09-09  0:02 ` Antoine Riard
    2 siblings, 0 replies; 7+ messages in thread
  From: Jeremy @ 2021-09-05  3:19 UTC (permalink / raw)
    To: Jeremy; +Cc: Bitcoin development mailing list
  
  [-- 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 --]
  
  ^ permalink raw reply	[flat|nested] 7+ messages in thread

• * Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
    2021-09-04  3:32 [bitcoin-dev] Note on Sequence Lock Upgrades Defect Jeremy
    2021-09-05  3:19 ` Jeremy
  @ 2021-09-06  2:35 ` David A. Harding
    2021-09-06  3:17   ` Jeremy
    2021-09-09  0:02 ` Antoine Riard
    2 siblings, 1 reply; 7+ messages in thread
  From: David A. Harding @ 2021-09-06  2:35 UTC (permalink / raw)
    To: Jeremy, Bitcoin Protocol Discussion
  
  [-- 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 --]
  
  ^ permalink raw reply	[flat|nested] 7+ messages in thread

• • * Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
      2021-09-06  2:35 ` David A. Harding
    @ 2021-09-06  3:17   ` Jeremy
      2021-09-06  6:16     ` darosior
      0 siblings, 1 reply; 7+ messages in thread
    From: Jeremy @ 2021-09-06  3:17 UTC (permalink / raw)
      To: David A. Harding; +Cc: Bitcoin Protocol Discussion
    
    [-- 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 --]
    
    ^ permalink raw reply	[flat|nested] 7+ messages in thread

  • • * Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
        2021-09-06  3:17   ` Jeremy
      @ 2021-09-06  6:16     ` darosior
        0 siblings, 0 replies; 7+ messages in thread
      From: darosior @ 2021-09-06  6:16 UTC (permalink / raw)
        To: Jeremy, Bitcoin Protocol Discussion
      
      [-- 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 --]
      
      ^ permalink raw reply	[flat|nested] 7+ messages in thread

• * Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
    2021-09-04  3:32 [bitcoin-dev] Note on Sequence Lock Upgrades Defect Jeremy
    2021-09-05  3:19 ` Jeremy
    2021-09-06  2:35 ` David A. Harding
  @ 2021-09-09  0:02 ` Antoine Riard
    2021-09-09  1:04   ` Jeremy
    2 siblings, 1 reply; 7+ messages in thread
  From: Antoine Riard @ 2021-09-09  0:02 UTC (permalink / raw)
    To: Jeremy, Bitcoin Protocol Discussion
  
  [-- 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 --]
  
  ^ permalink raw reply	[flat|nested] 7+ messages in thread

• • * Re: [bitcoin-dev] Note on Sequence Lock Upgrades Defect
      2021-09-09  0:02 ` Antoine Riard
    @ 2021-09-09  1:04   ` Jeremy
      0 siblings, 0 replies; 7+ messages in thread
    From: Jeremy @ 2021-09-09  1:04 UTC (permalink / raw)
      To: Antoine Riard; +Cc: Bitcoin Protocol Discussion
    
    [-- 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 --]
    
    ^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-09  1:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-04  3:32 [bitcoin-dev] Note on Sequence Lock Upgrades Defect Jeremy
2021-09-05  3:19 ` Jeremy
2021-09-06  2:35 ` David A. Harding
2021-09-06  3:17   ` Jeremy
2021-09-06  6:16     ` darosior
2021-09-09  0:02 ` Antoine Riard
2021-09-09  1:04   ` Jeremy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox