mempool: improve readability and documentation of RBF logic

pinheadmz · Aug 11, 2023 · 9079625 · 9079625
1 parent 48c6eff
commit 9079625
Show file tree

Hide file tree

Showing 3 changed files with 97 additions and 59 deletions.
diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js
@@ -983,63 +983,7 @@ class Mempool extends EventEmitter {
       conflicts = this.getConflicts(tx);
 
     if (conflicts.length) {
-      let conflictingFees = 0;
-      let totalEvictions = 0;
-
-      // Replacement TXs must pay a higher fee rate than each conflicting TX.
-      // We compare using deltaFee to account for prioritiseTransaction()
-      for (const conflict of conflicts) {
-        conflictingFees += conflict.descFee;
-        totalEvictions += this.countDescendants(conflict) + 1;
-
-        // Replacement TXs must not evict/replace more than 100 descendants
-        if (totalEvictions > 100) {
-          throw new VerifyError(tx,
-          'nonstandard',
-          'too many potential replacements',
-          0);
-        }
-
-        if (entry.getDeltaRate() <= conflict.getDeltaRate()) {
-          throw new VerifyError(tx,
-          'insufficientfee',
-          'insufficient fee',
-          0);
-        }
-      }
-
-      // Replacement TX must also pay for the total fees of all descendant
-      // transactions that will be evicted if an ancestor is replaced.
-      // Thus the replacement "pays for the bandwidth" of all the conflicts.
-      if (entry.deltaFee <= conflictingFees) {
-        throw new VerifyError(tx,
-        'insufficientfee',
-        'insufficient fee',
-        0);
-      }
-
-      // Once the conflicts are all paid for, the replacement TX fee
-      // still needs to cover it's own bandwidth.
-      const feeRemaining = entry.deltaFee - conflictingFees;
-      if (feeRemaining < minFee) {
-        throw new VerifyError(tx,
-          'insufficientfee',
-          'insufficient fee',
-          0);
-      }
-
-      // Replacement TXs can not consume any unconfirmed outputs that were not
-      // already included in the original transactions. They can only add
-      // confirmed UTXO, saving the trouble of checking new mempool ancestors.
-      for (const {prevout} of tx.inputs) {
-        if (!this.isSpent(prevout.hash, prevout.index) &&
-            this.map.has(prevout.hash)) {
-          throw new VerifyError(tx,
-            'nonstandard',
-            'replacement-adds-unconfirmed',
-            0);
-        }
-      }
+      this.verifyRBF(entry, conflicts);
     }
 
     // Contextual sanity checks.
@@ -1088,6 +1032,93 @@ class Mempool extends EventEmitter {
     return conflicts;
   }
 
+  /**
+   * Verify BIP 125 replaceability
+   * https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki
+   * Also see documented discrepancy between specification and implementation
+   * regarding Rule #1:
+   * https://www.cve.org/CVERecord?id=CVE-2021-31876
+   * Bitcoin Core policy may also change regularly, see:
+   * https://github.com/bitcoin/bitcoin/blob/master/doc/policy/
+   *   mempool-replacements.md
+   * @method
+   * @param {MempoolEntry} entry
+   * @param {MempoolEntry[]} conflicts
+   * @returns {Boolean}
+   */
+
+  verifyRBF(entry, conflicts) {
+    const tx = entry.tx;
+
+    let conflictingFees = 0;
+    let totalEvictions = 0;
+
+    for (const conflict of conflicts) {
+      conflictingFees += conflict.descFee;
+      totalEvictions += this.countDescendants(conflict) + 1;
+
+      // Rule #5
+      // Replacement TXs must not evict/replace more than 100 descendants
+      if (totalEvictions > policy.MEMPOOL_MAX_REPLACEMENTS) {
+        throw new VerifyError(tx,
+        'nonstandard',
+        'too many potential replacements',
+        0);
+      }
+
+      // Rule #6 - currently implemented by Bitcoin Core but not in BIP 125
+      // The replacement transaction must have a higher feerate than its
+      // direct conflicts.
+      // We compare using deltaFee to account for prioritiseTransaction()
+      if (entry.getDeltaRate() <= conflict.getDeltaRate()) {
+        throw new VerifyError(tx,
+        'insufficientfee',
+        'insufficient fee: must not reduce total mempool fee rate',
+        0);
+      }
+    }
+
+    // Rule #3 and #4
+    // Replacement TX must pay for the total fees of all descendant
+    // transactions that will be evicted if an ancestor is replaced.
+    // Thus the replacement "pays for the bandwidth" of all the conflicts.
+    // Once the conflicts are all paid for, the replacement TX fee
+    // still needs to cover it's own bandwidth.
+    // We use our policy min fee, Bitcoin Core has a separate incremental fee
+    const minFee = policy.getMinFee(entry.size, this.options.minRelay);
+    const feeRemaining = entry.deltaFee - conflictingFees;
+    if (feeRemaining < minFee) {
+      throw new VerifyError(tx,
+        'insufficientfee',
+        'insufficient fee: must pay for fees including conflicts',
+        0);
+    }
+
+    // Rule #2
+    // Replacement TXs can not consume any unconfirmed outputs that were not
+    // already included in the original transactions. They can only add
+    // confirmed UTXO, saving the trouble of checking new mempool ancestors.
+    for (const {prevout} of tx.inputs) {
+      // We don't have the transaction in the mempool and it is not an
+      // orphan, it means we are spending from the chain.
+      // Rule #2 does not apply.
+      if (!this.map.has(prevout.hash))
+        continue;
+
+      // If the prevout is in the mempool and it was already being spent
+      // by other transactions (which we are in conflict with) - we
+      // are not violating rule #2.
+      if (this.isSpent(prevout.hash, prevout.index))
+        continue;
+
+      // TX is spending new unconfirmed coin, which violates rule #2.
+      throw new VerifyError(tx,
+        'nonstandard',
+        'replacement-adds-unconfirmed',
+        0);
+    }
+  }
+
   /**
    * Verify inputs, return a boolean
    * instead of an error based on success.

diff --git a/lib/protocol/policy.js b/lib/protocol/policy.js
@@ -172,6 +172,13 @@ exports.MEMPOOL_EXPIRY_TIME = 72 * 60 * 60;
 
 exports.MEMPOOL_MAX_ORPHANS = 100;
 
+/**
+ * BIP125 Rule #5
+ * Maximum number of transactions that can be replaced.
+ */
+
+exports.MEMPOOL_MAX_REPLACEMENTS = 100;
+
 /**
  * Minimum block size to create. Block will be
  * filled with free transactions until block

diff --git a/test/mempool-test.js b/test/mempool-test.js
@@ -1281,7 +1281,7 @@ describe('Mempool', function() {
         await mempool.addTX(tx2);
       }, {
         type: 'VerifyError',
-        reason: 'insufficient fee'
+        reason: 'insufficient fee: must not reduce total mempool fee rate'
       });
 
       // Try again with higher fee
@@ -1342,7 +1342,7 @@ describe('Mempool', function() {
         await mempool.addTX(tx2);
       }, {
         type: 'VerifyError',
-        reason: 'insufficient fee'
+        reason: 'insufficient fee: must pay for fees including conflicts'
       });
 
       // Try again with higher fee