policy: getmempoolentry return "correct" bip125-replaceable status

mjdietzx · mjdietzx · commit 831ecb0cc0d2 · 2021-12-01T12:09:08.000-06:00
Now RBF signaling adheres to BIP125 RBF (Rule 5) which states that the number
of original transactions to be replaced and their descendant transactions
must not exceed `MAX_BIP125_REPLACEMENT_CANDIDATES`. Prior to this commit,
the bip125-replaceable status of a given transaction indicated whether the
transaction or _any_ of its unconfirmed ancestors opted-in to RBF, with no
consideration of this limitation.

Note: the bip125-replaceable status that `getmempoolentry` returns is still
inconsistent with what the mempool will treat as replaceable, as the mempool
does not (yet) allow inherited signaling.
diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
@@ -21,20 +21,21 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
         return RBFTransactionState::UNKNOWN;
     }
 
-    // First check the transaction itself.
-    if (SignalsOptInRBF(tx)) {
+    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
+    if (entry.GetCountWithDescendants() > MAX_BIP125_REPLACEMENT_CANDIDATES) {
+        return RBFTransactionState::FINAL;
+    } else if (SignalsOptInRBF(tx)) {
         return RBFTransactionState::REPLACEABLE_BIP125;
     }
 
     // If all the inputs have nSequence >= maxint-1, it still might be
     // signaled for RBF if any unconfirmed parents have signaled.
     uint64_t noLimit = std::numeric_limits<uint64_t>::max();
     std::string dummy;
-    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
     pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
 
     for (CTxMemPool::txiter it : ancestors) {
-        if (SignalsOptInRBF(it->GetTx())) {
+        if (SignalsOptInRBF(it->GetTx()) && it->GetCountWithDescendants() <= MAX_BIP125_REPLACEMENT_CANDIDATES) {
             return RBFTransactionState::REPLACEABLE_BIP125;
         }
     }
diff --git a/src/policy/rbf.h b/src/policy/rbf.h
@@ -30,7 +30,10 @@ enum class RBFTransactionState {
  * Determine whether an unconfirmed transaction is signaling opt-in to RBF
  * according to BIP 125
  * This involves checking sequence numbers of the transaction, as well
- * as the sequence numbers of all in-mempool ancestors.
+ * as the sequence numbers of all in-mempool ancestors for inherited signaling.
+ * This also takes the number of original transactions to be replaced and their
+ * descendant transactions into consideration as according to BIP125 RBF (Rule #5)
+ * this must not exceed `MAX_BIP125_REPLACEMENT_CANDIDATES`.
  *
  * @param tx   The unconfirmed transaction
  * @param pool The mempool, which may contain the tx
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
@@ -740,24 +740,24 @@ def test_reorged_inherited_signaling_and_descendant_limit(self):
 
         # Create a chain the size of `MAX_REPLACEMENT_LIMIT` spending `joined_tx` - the BIP125 Rule #5 imposed limit
         tx = None
-        for _ in range(MAX_REPLACEMENT_LIMIT - 1):
+        for i in range(MAX_REPLACEMENT_LIMIT - 1):
             tx = self.wallet.send_self_transfer(
                 from_node=self.nodes[0],
                 utxo_to_spend=joined_utxo if tx is None else self.wallet.get_utxo(txid=tx['txid']),  # a straight line of descendants
                 sequence=0xffffffff,
                 fee_rate=Decimal('0.0001'),
             )
-            assert_equal(True, self.nodes[0].getmempoolentry(tx['txid'])['bip125-replaceable'])  # inherited
+            assert_equal(i < MAX_REPLACEMENT_LIMIT - 2, self.nodes[0].getmempoolentry(tx['txid'])['bip125-replaceable'])  # inherited
         # Now we have a chain of: `optin_parent_tx`, `joined_tx`, and 99 txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT`
 
         # Attempting to replace the opt-in parent transaction will now result in more than `MAX_REPLACEMENT_LIMIT`
-        # conflicting txns being evicted from the mempool. However, it (and all of its descendants) are still signaling replaceability.
-        # We would've expected that once `MAX_REPLACEMENT_LIMIT` is exceeded, the opt-in parent txn stops signaling
+        # conflicting txns being evicted from the mempool. Therefore, it (and all of its descendants) no longer signal replaceability.
+        # Since `MAX_REPLACEMENT_LIMIT` is exceeded, the opt-in parent txn stops signaling
         # replaceability, along with _all_ of its descendants.
         entry = self.nodes[0].getmempoolentry(optin_parent_tx["txid"])
         assert_greater_than(entry['descendantcount'], MAX_REPLACEMENT_LIMIT)
-        assert_equal(True, entry['bip125-replaceable'])
-        assert_equal(True, self.nodes[0].getmempoolentry(tx["txid"])['bip125-replaceable'])
+        assert_equal(False, entry['bip125-replaceable'])
+        assert_equal(False, self.nodes[0].getmempoolentry(tx["txid"])['bip125-replaceable'])
 
         # Case in point, we can't actually replace `optin_parent_tx` once it has `MAX_REPLACEMENT_LIMIT` descendants
         self.wallet.create_self_transfer(

Original file line number	Diff line number	Diff line change
`@@ -21,20 +21,21 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)`
`21`	`21`	`return RBFTransactionState::UNKNOWN;`
`22`	`22`	`}`
`23`	`23`
`24`		`- // First check the transaction itself.`
`25`		`- if (SignalsOptInRBF(tx)) {`
	`24`	`+ CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());`
	`25`	`+ if (entry.GetCountWithDescendants() > MAX_BIP125_REPLACEMENT_CANDIDATES) {`
	`26`	`+ return RBFTransactionState::FINAL;`
	`27`	`+ } else if (SignalsOptInRBF(tx)) {`
`26`	`28`	`return RBFTransactionState::REPLACEABLE_BIP125;`
`27`	`29`	`}`
`28`	`30`
`29`	`31`	`// If all the inputs have nSequence >= maxint-1, it still might be`
`30`	`32`	`// signaled for RBF if any unconfirmed parents have signaled.`
`31`	`33`	`uint64_t noLimit = std::numeric_limits<uint64_t>::max();`
`32`	`34`	`std::string dummy;`
`33`		`- CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());`
`34`	`35`	`pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false);`
`35`	`36`
`36`	`37`	`for (CTxMemPool::txiter it : ancestors) {`
`37`		`- if (SignalsOptInRBF(it->GetTx())) {`
	`38`	`+ if (SignalsOptInRBF(it->GetTx()) && it->GetCountWithDescendants() <= MAX_BIP125_REPLACEMENT_CANDIDATES) {`
`38`	`39`	`return RBFTransactionState::REPLACEABLE_BIP125;`
`39`	`40`	`}`
`40`	`41`	`}`