Merge #1564: mempool: disable full-RBF, require BIP125 opt-in signaling

0e0d019 mempool: disable full-RBF, require BIP125 opt-in signaling (Byron Hambly)

Pull request description:

  Bitcoin Core defaults to accepting any fee-bumping replacement regardless of signaling (full-RBF). Revert this for Elements: only replace mempool transactions that explicitly opt in via BIP125 nSequence signaling (or TRUC), and report fullrbf=false in getmempoolinfo.

  Update feature_rbf.py accordingly, and add a regression test for CVE-2021-31876 confirming that inherited signaling from an unconfirmed parent does not make a non-signaling child replaceable.

ACKs for top commit:
  tomt1664:
    Tested locally ACK 0e0d019

Tree-SHA512: 58847919c6a702fe3d5ea4065e07912684cce6f084ca2507d9ef24c6fe8fdb290bb08108d9e025f26a5674e731bf71a9bdbd262aaf78e4f991bcdf06b61228f7

Author: tomt1664

src/rpc/mempool.cpp

@@ -695,7 +695,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
     ret.pushKV("minrelaytxfee", ValueFromAmount(pool.m_opts.min_relay_feerate.GetFeePerK()));
     ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));
     ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
-    ret.pushKV("fullrbf", true);
+    ret.pushKV("fullrbf", false);
     return ret;
 }
 
@@ -717,7 +717,7 @@ static RPCHelpMan getmempoolinfo()
                 {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
                 {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"},
                 {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
-                {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection (DEPRECATED)"},
+                {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection (DEPRECATED). Always false: this mempool requires BIP125 opt-in signaling for replacement, except for TRUC transactions."},
             }},
         RPCExamples{
             HelpExampleCli("getmempoolinfo", "")

src/validation.cpp

@@ -986,7 +986,24 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
                 // Transaction conflicts with a mempool tx, but we're not allowing replacements in this context.
                 return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed");
             }
-            ws.m_conflicts.insert(ptxConflicting->GetHash());
+            if (!ws.m_conflicts.count(ptxConflicting->GetHash()))
+            {
+                // Transactions that don't explicitly signal replaceability are
+                // *not* replaceable with the current logic, even if one of their
+                // unconfirmed ancestors signals replaceability. This diverges
+                // from BIP125's inherited signaling description (see CVE-2021-31876).
+                // Applications relying on first-seen mempool behavior should
+                // check all unconfirmed ancestors; otherwise an opt-in ancestor
+                // might be replaced, causing removal of this descendant.
+                //
+                // All TRUC transactions are considered replaceable.
+                const bool allow_rbf{SignalsOptInRBF(*ptxConflicting) || ptxConflicting->version == TRUC_VERSION};
+                if (!allow_rbf) {
+                    return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
+                }
+
+                ws.m_conflicts.insert(ptxConflicting->GetHash());
+            }
         }
     }
 

test/functional/feature_rbf.py

@@ -9,6 +9,7 @@
 # from test_framework.script import CScript
 from test_framework.messages import (
     MAX_BIP125_RBF_SEQUENCE,
+    SEQUENCE_FINAL,
     COIN,
     # CTransaction,
     # CTxIn,
@@ -82,6 +83,9 @@ def run_test(self):
         self.log.info("Running test full replace by fee...")
         self.test_fullrbf()
 
+        self.log.info("Running test no inherited signaling (CVE-2021-31876)...")
+        self.test_no_inherited_signaling()
+
         self.log.info("Passed")
 
     def make_utxo(self, node, amount, *, confirmed=True, scriptPubKey=None):
@@ -602,11 +606,13 @@ def test_replacement_relay_fee(self):
 
     def test_fullrbf(self):
         # BIP125 signaling is not respected
+        # ELEMENTS: disable mempoolfullrbf and keep BIP125 signaling
 
         confirmed_utxo = self.make_utxo(self.nodes[0], int(2 * COIN))
-        assert self.nodes[0].getmempoolinfo()["fullrbf"]
+        assert not self.nodes[0].getmempoolinfo()["fullrbf"] # ELEMENTS
 
         # Create an explicitly opt-out BIP125 transaction, which will be ignored
+        # ELEMENTS: respect BIP125 signal
         optout_tx = self.wallet.send_self_transfer(
             from_node=self.nodes[0],
             utxo_to_spend=confirmed_utxo,
@@ -620,12 +626,67 @@ def test_fullrbf(self):
                 fee_rate=Decimal('0.02'),
         )
 
-        # Send the replacement transaction, conflicting with the optout_tx.
-        self.nodes[0].sendrawtransaction(conflicting_tx['hex'], 0)
+        # ELEMENTS: The replacement is rejected because optout_tx did not signal replaceability.
+        assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, conflicting_tx['hex'], 0)
+
+        # ELEMENTS: Optout_tx is still in the mempool; the replacement was rejected
+        assert optout_tx['txid'] in self.nodes[0].getrawmempool()
+        assert conflicting_tx['txid'] not in self.nodes[0].getrawmempool()
+
+    # ELEMENTS
+    def test_no_inherited_signaling(self):
+        """Make sure we're not susceptible to CVE-2021-31876.
+
+        BIP125 says a transaction is replaceable if it directly signals via
+        nSequence, *or* if it spends an output of an unconfirmed parent that
+        itself signals replaceability ("inherited signaling").
+        Elements only enforces the direct-signaling half of that rule
+        when deciding whether to *accept* a replacement -- see the comment
+        next to the SignalsOptInRBF() call in MemPoolAccept::PreChecks().
+        This is the discrepancy reported as CVE-2021-31876: a transaction
+        can be reported as `bip125-replaceable` (because it inherits the
+        signal from an unconfirmed ancestor) while an actual attempt to
+        replace it is still rejected.
+
+        This test confirms that this divergence is consistent: a
+        non-signaling child of a signaling, unconfirmed parent must remain
+        non-replaceable.
+        """
+        confirmed_utxo = self.make_utxo(self.nodes[0], int(2 * COIN))
+
+        # Parent explicitly signals replaceability...
+        parent_utxo = self.wallet.send_self_transfer(
+            from_node=self.nodes[0],
+            utxo_to_spend=confirmed_utxo,
+            sequence=MAX_BIP125_RBF_SEQUENCE,
+            fee=Decimal("0.01"),
+        )["new_utxo"]
+
+        # ...but the child spending it does not.
+        child_tx = self.wallet.send_self_transfer(
+            from_node=self.nodes[0],
+            utxo_to_spend=parent_utxo,
+            sequence=SEQUENCE_FINAL,
+            fee=Decimal("0.01"),
+        )
+
+        # `bip125-replaceable` reports True: it is inherited from the
+        # unconfirmed, signaling parent (the BIP125-compliant half of the logic,
+        # implemented by IsRBFOptIn() for RPC reporting purposes).
+        assert_equal(True, self.nodes[0].getmempoolentry(child_tx['txid'])['bip125-replaceable'])
+
+        # However, an actual replacement of the child is rejected: PreChecks()
+        # only consults the child's own sequence numbers, not its ancestors',
+        # so the inherited signal is *not* honoured for acceptance purposes.
+        conflicting_tx = self.wallet.create_self_transfer(
+            utxo_to_spend=parent_utxo,
+            fee=Decimal("0.05"),
+        )
+        assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, conflicting_tx['hex'], 0)
 
-        # Optout_tx is not anymore in the mempool.
-        assert optout_tx['txid'] not in self.nodes[0].getrawmempool()
-        assert conflicting_tx['txid'] in self.nodes[0].getrawmempool()
+        # The non-signaling child is still in the mempool; the replacement never got in.
+        assert child_tx['txid'] in self.nodes[0].getrawmempool()
+        assert conflicting_tx['txid'] not in self.nodes[0].getrawmempool()
 
 if __name__ == '__main__':
     ReplaceByFeeTest(__file__).main()