From 0a3ecf2d790fad654831b79a3e6c1dac5d0b5a3e Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Wed, 3 Jul 2019 15:18:00 -0700 Subject: [PATCH 01/21] mempool: Allow double spend of RBF transaction "Quick and dirty" isDoubleSpend() check evaluates to false if the conflicting tx has opted in to RBF. More verbose checks are still required for BIP125. General RBF tests added. --- lib/mempool/mempool.js | 4 +- test/mempool-test.js | 186 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 1 deletion(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index 090ff470b..2ef368e52 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -1659,7 +1659,9 @@ class Mempool extends EventEmitter { isDoubleSpend(tx) { for (const {prevout} of tx.inputs) { const {hash, index} = prevout; - if (this.isSpent(hash, index)) + const conflict = this.getSpent(hash, index); + + if (conflict && !conflict.tx.isRBF()) return true; } diff --git a/test/mempool-test.js b/test/mempool-test.js index be0dd998e..160e493fe 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1093,4 +1093,190 @@ describe('Mempool', function() { throw err; }); }); + + describe('Replace-by-fee', function () { + const blocks = new BlockStore({ + memory: true + }); + + const chain = new Chain({ + memory: true, + blocks + }); + + const mempool = new Mempool({ + chain, + memory: true + }); + + before(async () => { + await blocks.open(); + await mempool.open(); + await chain.open(); + }); + + after(async () => { + await chain.close(); + await mempool.close(); + await blocks.close(); + }); + + beforeEach(async () => { + await mempool.reset(); + assert.strictEqual(mempool.map.size, 0); + }); + + // Number of coins available in + // chaincoins (100k satoshi per coin). + const N = 100; + const chaincoins = new MemWallet(); + const wallet = new MemWallet(); + + it('should create coins in chain', async () => { + const mtx = new MTX(); + mtx.addInput(new Input()); + + for (let i = 0; i < N; i++) { + const addr = chaincoins.createReceive().getAddress(); + mtx.addOutput(addr, 100000); + } + + const cb = mtx.toTX(); + const block = await getMockBlock(chain, [cb], false); + const entry = await chain.add(block, VERIFY_NONE); + + await mempool._addBlock(entry, block.txs); + + // Add 100 blocks so we don't get + // premature spend of coinbase. + for (let i = 0; i < 100; i++) { + const block = await getMockBlock(chain); + const entry = await chain.add(block, VERIFY_NONE); + + await mempool._addBlock(entry, block.txs); + } + + chaincoins.addTX(cb); + }); + + it('should not accept RBF tx', async() => { + mempool.options.replaceByFee = false; + + const mtx = new MTX(); + const coin = chaincoins.getCoins()[0]; + mtx.addCoin(coin); + mtx.inputs[0].sequence = 0xfffffffd; + + const addr = wallet.createReceive().getAddress(); + mtx.addOutput(addr, 90000); + + chaincoins.sign(mtx); + + assert(mtx.verify()); + const tx = mtx.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx); + }, { + type: 'VerifyError', + reason: 'replace-by-fee' + }); + + assert(!mempool.hasCoin(tx.hash(), 0)); + assert.strictEqual(mempool.map.size, 0); + }); + + it('should accept RBF tx with RBF option enabled', async() => { + mempool.options.replaceByFee = true; + + const mtx = new MTX(); + const coin = chaincoins.getCoins()[0]; + mtx.addCoin(coin); + mtx.inputs[0].sequence = 0xfffffffd; + + const addr = wallet.createReceive().getAddress(); + mtx.addOutput(addr, coin.value - 1000); + + chaincoins.sign(mtx); + + assert(mtx.verify()); + const tx = mtx.toTX(); + + await mempool.addTX(tx); + + assert(mempool.hasCoin(tx.hash(), 0)); + assert.strictEqual(mempool.map.size, 1); + }); + + it('should reject double spend without RBF from mempool', async() => { + mempool.options.replaceByFee = true; + + const coin = chaincoins.getCoins()[0]; + + const mtx1 = new MTX(); + const mtx2 = new MTX(); + mtx1.addCoin(coin); + mtx2.addCoin(coin); + + const addr1 = wallet.createReceive().getAddress(); + mtx1.addOutput(addr1, coin.value - 1000); + + const addr2 = wallet.createReceive().getAddress(); + mtx2.addOutput(addr2, coin.value - 1000); + + chaincoins.sign(mtx1); + chaincoins.sign(mtx2); + + assert(mtx1.verify()); + assert(mtx2.verify()); + const tx1 = mtx1.toTX(); + const tx2 = mtx2.toTX(); + + assert(!tx1.isRBF()); + + await mempool.addTX(tx1); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'bad-txns-inputs-spent' + }); + + assert(mempool.hasCoin(tx1.hash(), 0)); + assert.strictEqual(mempool.map.size, 1); + }); + + it('should accept double spend with RBF into mempool', async() => { + mempool.options.replaceByFee = true; + + const coin = chaincoins.getCoins()[0]; + + const mtx1 = new MTX(); + const mtx2 = new MTX(); + mtx1.addCoin(coin); + mtx2.addCoin(coin); + + mtx1.inputs[0].sequence = 0xfffffffd; + + const addr1 = wallet.createReceive().getAddress(); + mtx1.addOutput(addr1, coin.value - 1000); + + const addr2 = wallet.createReceive().getAddress(); + mtx2.addOutput(addr2, coin.value - 1000); + + chaincoins.sign(mtx1); + chaincoins.sign(mtx2); + + assert(mtx1.verify()); + assert(mtx2.verify()); + const tx1 = mtx1.toTX(); + const tx2 = mtx2.toTX(); + + assert(tx1.isRBF()); + + await mempool.addTX(tx1); + await mempool.addTX(tx2); + }); + }); }); From bb8b4b0a5201726074d366a5e3f777a7163a877e Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 5 Jul 2019 10:37:42 -0700 Subject: [PATCH 02/21] mempool: Reject RBF replacements with insufficient fees Implements and tests BIP125 rules specifically pertaining to fee requirements for replacement transactions. Valid replacements must pay a higher fee rate than the original TX, and also must pay for the bandwidth of all potentially evicted transactions in addition to its own bandwidth. --- lib/mempool/mempool.js | 60 ++++++++++++++++++++++++++++++++++++++++++ test/mempool-test.js | 58 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index 2ef368e52..fdea7cada 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -953,6 +953,46 @@ class Mempool extends EventEmitter { 0); } + // If we reject RBF transactions altogether we can skip these checks, + // because incoming conflicts are already rejected as double spends. + if (this.options.replaceByFee) { + const conflicts = this.getConflicts(tx); + let conflictingFees = 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; + + 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); + } + } + // Contextual sanity checks. const [fee, reason, score] = tx.checkInputs(view, height); @@ -1668,6 +1708,26 @@ class Mempool extends EventEmitter { return false; } + /** + * Get an array of all transactions currently in the mempool that + * spend one or more of the same outputs as an incoming transaction. + * @param {TX} tx + * @returns {Promise} - Returns (@link MempoolEntry[]}. + */ + + getConflicts(tx) { + const conflicts = new Set(); + + for (const { prevout: { hash, index } } of tx.inputs) { + const conflict = this.getSpent(hash, index); + + if (conflict) { + conflicts.add(conflict); + } + } + return Array.from(conflicts); + } + /** * Get coin viewpoint (lock). * Note: this does not return diff --git a/test/mempool-test.js b/test/mempool-test.js index 160e493fe..4bfd1a923 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1247,7 +1247,7 @@ describe('Mempool', function() { assert.strictEqual(mempool.map.size, 1); }); - it('should accept double spend with RBF into mempool', async() => { + it('should reject replacement with lower fee rate', async() => { mempool.options.replaceByFee = true; const coin = chaincoins.getCoins()[0]; @@ -1260,10 +1260,10 @@ describe('Mempool', function() { mtx1.inputs[0].sequence = 0xfffffffd; const addr1 = wallet.createReceive().getAddress(); - mtx1.addOutput(addr1, coin.value - 1000); + mtx1.addOutput(addr1, coin.value - 1000); // 1000 satoshi fee const addr2 = wallet.createReceive().getAddress(); - mtx2.addOutput(addr2, coin.value - 1000); + mtx2.addOutput(addr2, coin.value - 900); // 900 satoshi fee chaincoins.sign(mtx1); chaincoins.sign(mtx2); @@ -1276,7 +1276,57 @@ describe('Mempool', function() { assert(tx1.isRBF()); await mempool.addTX(tx1); - await mempool.addTX(tx2); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'insufficient fee' + }); + }); + + it('should reject replacement that doesnt pay all child fees', async() => { + mempool.options.replaceByFee = true; + + const addr1 = chaincoins.createReceive().getAddress(); + const addr2 = wallet.createReceive().getAddress(); + const originalCoin = chaincoins.getCoins()[0]; + let coin = originalCoin; + + // Generate chain of 10 transactions, each paying 1000 sat fee + for (let i = 0; i < 10; i++) { + const mtx = new MTX(); + mtx.addCoin(coin); + mtx.inputs[0].sequence = 0xfffffffd; + mtx.addOutput(addr1, coin.value - 1000); + chaincoins.sign(mtx); + assert(mtx.verify()); + const tx = mtx.toTX(); + await mempool.addTX(tx); + + coin = Coin.fromTX(tx, 0, -1); + } + + // Pay for all child fees + let fee = 10 * 1000; + + // Pay for its own bandwidth (estimating tx2 size as 200 bytes) + fee += mempool.options.minRelay * 0.2; + + // Attempt to submit a replacement for the initial parent TX + const mtx2 = new MTX(); + mtx2.addCoin(originalCoin); + mtx2.addOutput(addr2, originalCoin.value - fee + 100); + chaincoins.sign(mtx2); + assert(mtx2.verify()); + const tx2 = mtx2.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'insufficient fee' + }); }); }); }); From d62f1b3951d14f7709eddb63bc6055977f486425 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 5 Jul 2019 14:02:14 -0700 Subject: [PATCH 03/21] mempool: Reject RBF replacement edge cases Implements two extra rules defined in BIP125: - The replacement transaction may only include an unconfirmed input if that input was included in one of the original transactions. - The number of original transactions to be replaced and their descendant transactions which will be evicted from the mempool must not exceed a total of 100 transactions. --- lib/mempool/mempool.js | 30 +++++++++++- test/mempool-test.js | 101 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index fdea7cada..f023018bb 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -955,14 +955,27 @@ class Mempool extends EventEmitter { // If we reject RBF transactions altogether we can skip these checks, // because incoming conflicts are already rejected as double spends. - if (this.options.replaceByFee) { - const conflicts = this.getConflicts(tx); + let conflicts = []; + if (this.options.replaceByFee) + 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, @@ -991,6 +1004,19 @@ class Mempool extends EventEmitter { '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); + } + } } // Contextual sanity checks. diff --git a/test/mempool-test.js b/test/mempool-test.js index 4bfd1a923..047c031e0 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1328,5 +1328,106 @@ describe('Mempool', function() { reason: 'insufficient fee' }); }); + + it('should reject replacement including new unconfirmed UTXO', async() => { + mempool.options.replaceByFee = true; + + const coin1 = chaincoins.getCoins()[0]; + const coin2 = chaincoins.getCoins()[1]; + + const mtx1 = new MTX(); + const mtx2 = new MTX(); + mtx1.addCoin(coin1); + mtx2.addCoin(coin2); + + mtx1.inputs[0].sequence = 0xfffffffd; + + const addr1 = chaincoins.createReceive().getAddress(); + mtx1.addOutput(addr1, coin1.value - 1000); + + const addr2 = chaincoins.createReceive().getAddress(); + mtx2.addOutput(addr2, coin2.value - 1000); + + chaincoins.sign(mtx1); + chaincoins.sign(mtx2); + + assert(mtx1.verify()); + assert(mtx2.verify()); + const tx1 = mtx1.toTX(); + const tx2 = mtx2.toTX(); + + assert(tx1.isRBF()); + + await mempool.addTX(tx1); + await mempool.addTX(tx2); + + // Attempt to replace tx1 and include the unconfirmed output of tx2 + const mtx3 = new MTX(); + mtx3.addCoin(coin1); + const coin3 = Coin.fromTX(tx2, 0, -1); + mtx3.addCoin(coin3); + const addr3 = wallet.createReceive().getAddress(); + // Remember to bump the fee! + mtx3.addOutput(addr3, coin1.value + coin3.value - 2000); + chaincoins.sign(mtx3); + assert(mtx3.verify()); + const tx3 = mtx3.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx3); + }, { + type: 'VerifyError', + reason: 'replacement-adds-unconfirmed' + }); + }); + + it('should reject replacement evicting too many descendants', async() => { + mempool.options.replaceByFee = true; + + const addr1 = chaincoins.createReceive().getAddress(); + const coin0 = chaincoins.getCoins()[0]; + const coin1 = chaincoins.getCoins()[1]; + + // Generate big TX with 100 outputs + const mtx1 = new MTX(); + mtx1.addCoin(coin0); + mtx1.addCoin(coin1); + mtx1.inputs[0].sequence = 0xfffffffd; + const outputValue = (coin0.value / 100) + (coin1.value / 100) - 100; + for (let i = 0; i < 100; i++) + mtx1.addOutput(addr1, outputValue); + + chaincoins.sign(mtx1); + assert(mtx1.verify()); + const tx1 = mtx1.toTX(); + await mempool.addTX(tx1); + + // Spend each of those outputs individually + for (let i = 0; i < 100; i++) { + const mtx = new MTX(); + const coin = Coin.fromTX(tx1, i, -1); + mtx.addCoin(coin); + mtx.addOutput(addr1, coin.value - 1000); + chaincoins.sign(mtx); + assert(mtx.verify()); + const tx = mtx.toTX(); + await mempool.addTX(tx); + } + + // Attempt to evict the whole batch by replacing the first TX (tx1) + const mtx2 = new MTX(); + mtx2.addCoin(coin0); + mtx2.addOutput(addr1, coin0.value - 1000); + chaincoins.sign(mtx2); + assert(mtx2.verify()); + const tx2 = mtx2.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'too many potential replacements' + }); + }); }); }); From 70ef2867261510fe33b6503cae951d102e6ea6c1 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 5 Jul 2019 16:47:32 -0700 Subject: [PATCH 04/21] mempool: replace by fee All the BIP125 rules have been implemented to reject invalid replacement TXs. This is where the actual replacement happens, adding the replacement TX and evicting all the conflicts and their descendants. --- lib/mempool/mempool.js | 20 +++++++++++++-- test/mempool-test.js | 56 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index f023018bb..f127e4410 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -844,7 +844,20 @@ class Mempool extends EventEmitter { const entry = MempoolEntry.fromTX(tx, view, height); // Contextual verification. - await this.verify(entry, view); + const conflicts = await this.verify(entry, view); + + // Remove conflicting TXs and their descendants + if (conflicts.length) { + assert(this.options.replaceByFee); + + for (const conflict of conflicts) { + this.logger.debug( + 'Replacing tx %h with %h', + conflict.tx.hash(), + tx.hash()); + this.evictEntry(conflict); + } + } // Add and index the entry. await this.addEntry(entry, view); @@ -862,10 +875,11 @@ class Mempool extends EventEmitter { /** * Verify a transaction with mempool standards. + * Returns an array of conflicting TXs * @method * @param {MempoolEntry} entry * @param {CoinView} view - * @returns {Promise} + * @returns {Promise} - Returns {@link MempoolEntry[]} */ async verify(entry, view) { @@ -1061,6 +1075,8 @@ class Mempool extends EventEmitter { assert(await this.verifyResult(tx, view, flags), 'BUG: Verify failed for mandatory but not standard.'); } + + return conflicts; } /** diff --git a/test/mempool-test.js b/test/mempool-test.js index 047c031e0..6903fa1e6 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1283,6 +1283,20 @@ describe('Mempool', function() { type: 'VerifyError', reason: 'insufficient fee' }); + + // Try again with higher fee + const mtx3 = new MTX(); + mtx3.addCoin(coin); + mtx3.addOutput(addr2, coin.value - 1200); // 1200 satoshi fee + chaincoins.sign(mtx3); + assert(mtx3.verify()); + const tx3 = mtx3.toTX(); + + await mempool.addTX(tx3); + + // tx1 has been replaced by tx3 + assert(!mempool.has(tx1.hash())); + assert(mempool.has(tx3.hash())); }); it('should reject replacement that doesnt pay all child fees', async() => { @@ -1294,6 +1308,7 @@ describe('Mempool', function() { let coin = originalCoin; // Generate chain of 10 transactions, each paying 1000 sat fee + const childHashes = []; for (let i = 0; i < 10; i++) { const mtx = new MTX(); mtx.addCoin(coin); @@ -1304,6 +1319,8 @@ describe('Mempool', function() { const tx = mtx.toTX(); await mempool.addTX(tx); + childHashes.push(tx.hash()); + coin = Coin.fromTX(tx, 0, -1); } @@ -1327,6 +1344,21 @@ describe('Mempool', function() { type: 'VerifyError', reason: 'insufficient fee' }); + + // Try again with higher fee + const mtx3 = new MTX(); + mtx3.addCoin(originalCoin); + mtx3.addOutput(addr2, originalCoin.value - fee); + chaincoins.sign(mtx3); + assert(mtx3.verify()); + const tx3 = mtx3.toTX(); + + await mempool.addTX(tx3); + + // All child TXs have been replaced by tx3 + for (const hash of childHashes) + assert(!mempool.has(hash)); + assert(mempool.has(tx3.hash())); }); it('should reject replacement including new unconfirmed UTXO', async() => { @@ -1403,6 +1435,8 @@ describe('Mempool', function() { await mempool.addTX(tx1); // Spend each of those outputs individually + let tx; + const hashes = []; for (let i = 0; i < 100; i++) { const mtx = new MTX(); const coin = Coin.fromTX(tx1, i, -1); @@ -1410,14 +1444,19 @@ describe('Mempool', function() { mtx.addOutput(addr1, coin.value - 1000); chaincoins.sign(mtx); assert(mtx.verify()); - const tx = mtx.toTX(); + tx = mtx.toTX(); + + hashes.push(tx.hash()); + await mempool.addTX(tx); } // Attempt to evict the whole batch by replacing the first TX (tx1) const mtx2 = new MTX(); mtx2.addCoin(coin0); - mtx2.addOutput(addr1, coin0.value - 1000); + mtx2.addCoin(coin1); + // Send with massive fee to pay for 100 evicted TXs + mtx2.addOutput(addr1, 5000); chaincoins.sign(mtx2); assert(mtx2.verify()); const tx2 = mtx2.toTX(); @@ -1428,6 +1467,19 @@ describe('Mempool', function() { type: 'VerifyError', reason: 'too many potential replacements' }); + + // Manually remove one of the descendants in advance + const entry = mempool.getEntry(tx.hash()); + mempool.evictEntry(entry); + + // Send back the same TX + await mempool.addTX(tx2); + + // Entire mess has been replaced by tx2 + assert(mempool.has(tx2.hash())); + assert(!mempool.has(tx1.hash())); + for (const hash of hashes) + assert(!mempool.has(hash)); }); }); }); From f3cd6597ed99372471b2a3f6239e2b74076d952f Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Mon, 14 Oct 2019 17:57:05 -0400 Subject: [PATCH 05/21] mempool: clarify eviction of RBF conflicts --- lib/mempool/mempool.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index f127e4410..e954ffdd2 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -846,10 +846,10 @@ class Mempool extends EventEmitter { // Contextual verification. const conflicts = await this.verify(entry, view); - // Remove conflicting TXs and their descendants - if (conflicts.length) { - assert(this.options.replaceByFee); - + // RBF only: Remove conflicting TXs and their descendants + if (!this.options.replaceByFee) { + assert(conflicts.length === 0); + } else { for (const conflict of conflicts) { this.logger.debug( 'Replacing tx %h with %h', From bf43d9a7d431400938abf20e33b207497387b565 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 11 Aug 2023 15:28:51 -0400 Subject: [PATCH 06/21] mempool: improve readability and documentation of RBF logic --- lib/mempool/mempool.js | 145 +++++++++++++++++++++++++---------------- lib/protocol/policy.js | 7 ++ test/mempool-test.js | 4 +- 3 files changed, 97 insertions(+), 59 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index e954ffdd2..d707a4d65 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -974,63 +974,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. @@ -1079,6 +1023,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 index 39fce7484..85c8859d4 100644 --- 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 index 6903fa1e6..b9ac207b3 100644 --- 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 From caabf10d537325db9f26eabc461f37fa147a47a8 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sat, 19 Aug 2023 15:20:38 -0400 Subject: [PATCH 07/21] mempool: allow replacement of tx spending unconfirmed utxo --- lib/mempool/mempool.js | 35 +++++++++++++++--- test/mempool-test.js | 83 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index d707a4d65..3cb9f7723 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -1848,7 +1848,7 @@ class Mempool extends EventEmitter { } /** - * Get coin viewpoint (no lock). + * Get coin viewpoint as it pertains to mempool (no lock). * @method * @param {TX} tx * @returns {Promise} - Returns {@link CoinView}. @@ -1859,14 +1859,37 @@ class Mempool extends EventEmitter { for (const {prevout} of tx.inputs) { const {hash, index} = prevout; - const tx = this.getTX(hash); - if (tx) { - if (this.hasCoin(hash, index)) - view.addIndex(tx, index, -1); - continue; + // First check mempool for the TX + // that created the coin we need for the view + const parentTX = this.getTX(hash); + + if (parentTX) { + // Does parent TX even have the output index? + if (index >= parentTX.outputs.length) + continue; + + // Check to see if this output is already spent + // by another unconfirmed TX in the mempool + const spender = this.getSpent(hash, index); + + if (!spender) { + // Parent TX output is unspent, add it to the view + view.addIndex(parentTX, index, -1); + continue; + } + + // If the spender TX signals opt-in RBF, then we + // can still consider the parent TX output as spendable. + // Note that at this point we are not fully checking all the + // replaceability rules and the tx initally passed to this + // function may not be actually allowed to replace the spender. + if (spender.tx.isRBF()) + view.addIndex(parentTX, index, -1); } + // Parent TX is not in mempool. + // Check the chain (UTXO set) for the coin. const coin = await this.chain.readCoin(prevout); if (coin) diff --git a/test/mempool-test.js b/test/mempool-test.js index b9ac207b3..9158fa684 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1481,5 +1481,88 @@ describe('Mempool', function() { for (const hash of hashes) assert(!mempool.has(hash)); }); + + it('should accept replacement spending an unconfirmed output', async () => { + mempool.options.replaceByFee = true; + + const addr1 = chaincoins.createReceive().getAddress(); + const coin0 = chaincoins.getCoins()[0]; + + // Generate parent TX + const mtx0 = new MTX(); + mtx0.addCoin(coin0); + mtx0.addOutput(addr1, coin0.value - 200); + chaincoins.sign(mtx0); + assert(mtx0.verify()); + const tx0 = mtx0.toTX(); + await mempool.addTX(tx0); + + // Spend unconfirmed output to replaceable child + const mtx1 = new MTX(); + const coin1 = Coin.fromTX(tx0, 0, -1); + mtx1.addCoin(coin1); + mtx1.inputs[0].sequence = 0xfffffffd; + mtx1.addOutput(addr1, coin1.value - 200); + chaincoins.sign(mtx1); + assert(mtx1.verify()); + const tx1 = mtx1.toTX(); + await mempool.addTX(tx1); + + // Send replacement + const mtx2 = new MTX(); + mtx2.addCoin(coin1); + mtx2.addOutput(addr1, coin1.value - 400); + chaincoins.sign(mtx2); + assert(mtx2.verify()); + const tx2 = mtx2.toTX(); + await mempool.addTX(tx2); + + // Unconfirmed parent and replacement in mempool together + assert(mempool.has(tx0.hash())); + assert(!mempool.has(tx1.hash())); + assert(mempool.has(tx2.hash())); + }); + + it('should not accept replacement for non-rbf spender of unconfirmed utxo', async () => { + mempool.options.replaceByFee = true; + + const addr1 = chaincoins.createReceive().getAddress(); + const coin0 = chaincoins.getCoins()[0]; + + // Generate parent TX + const mtx0 = new MTX(); + mtx0.addCoin(coin0); + mtx0.addOutput(addr1, coin0.value - 200); + chaincoins.sign(mtx0); + assert(mtx0.verify()); + const tx0 = mtx0.toTX(); + await mempool.addTX(tx0); + + // Spend unconfirmed output to non-replaceable child + const mtx1 = new MTX(); + const coin1 = Coin.fromTX(tx0, 0, -1); + mtx1.addCoin(coin1); + mtx1.inputs[0].sequence = 0xffffffff; // not replaceable + mtx1.addOutput(addr1, coin1.value - 200); + chaincoins.sign(mtx1); + assert(mtx1.verify()); + const tx1 = mtx1.toTX(); + await mempool.addTX(tx1); + + // Send attempted replacement + const mtx2 = new MTX(); + mtx2.addCoin(coin1); + mtx2.addOutput(addr1, coin1.value - 400); + chaincoins.sign(mtx2); + assert(mtx2.verify()); + const tx2 = mtx2.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'bad-txns-inputs-spent' + }); + }); }); }); From 5e0cd1948af31fbc5f67ae12526dac4d2bc4cd89 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sat, 19 Aug 2023 16:09:14 -0400 Subject: [PATCH 08/21] mempool: restore original isDoubleSpend() implementation We only need to call the "quick and dirty test" if the user has set mempool RBF option to false. Do the Rule #1 check in verifyRBF() instead where it belongs. This leaves one loose end in maybeOrphan() that must be caught so we don't treat double-spends like orphans. --- lib/mempool/mempool.js | 52 +++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index 3cb9f7723..4c935af3b 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -817,15 +817,19 @@ class Mempool extends EventEmitter { 0); } - // Quick and dirty test to verify we're - // not double-spending an output in the - // mempool. - if (this.isDoubleSpend(tx)) { - this.emit('conflict', tx); - throw new VerifyError(tx, - 'duplicate', - 'bad-txns-inputs-spent', - 0); + if (!this.options.replaceByFee) { + // Quick and dirty test to verify we're + // not double-spending an output in the + // mempool. + // If we are verifying RBF, we will do a + // slow and clean test later... + if (this.isDoubleSpend(tx)) { + this.emit('conflict', tx); + throw new VerifyError(tx, + 'duplicate', + 'bad-txns-inputs-spent', + 0); + } } // Get coin viewpoint as it @@ -1045,6 +1049,15 @@ class Mempool extends EventEmitter { let totalEvictions = 0; for (const conflict of conflicts) { + // Rule #1 - as implemented in Bitcoin Core (not BIP 125) + // We do not replace TXs that do not signal opt-in RBF + if (!conflict.tx.isRBF()) { + throw new VerifyError(tx, + 'duplicate', + 'bad-txns-inputs-spent', + 0); + } + conflictingFees += conflict.descFee; totalEvictions += this.countDescendants(conflict) + 1; @@ -1538,9 +1551,22 @@ class Mempool extends EventEmitter { const missing = []; for (const {prevout} of tx.inputs) { + // We assume the view came from mempool.getCoinView() + // which will only exclude spent coins if the spender + // is not replaceable if (view.hasEntry(prevout)) continue; + // If this prevout is missing from the view because + // it has a spender in the mempool it's not an orphan, + // it's a double-spend. + if (this.isSpent(prevout.hash, prevout.index)) { + throw new VerifyError(tx, + 'duplicate', + 'bad-txns-inputs-spent', + 0); + } + if (this.hasReject(prevout.hash)) { this.logger.debug( 'Not storing orphan %h (rejected parents).', @@ -1774,7 +1800,7 @@ class Mempool extends EventEmitter { const {hash, index} = prevout; const conflict = this.getSpent(hash, index); - if (conflict && !conflict.tx.isRBF()) + if (conflict) return true; } @@ -1884,8 +1910,12 @@ class Mempool extends EventEmitter { // Note that at this point we are not fully checking all the // replaceability rules and the tx initally passed to this // function may not be actually allowed to replace the spender. - if (spender.tx.isRBF()) + if (spender.tx.isRBF()) { + // If any TX in the mempool signals opt-in RBF + // we already know this option is set. + assert(this.options.replaceByFee); view.addIndex(parentTX, index, -1); + } } // Parent TX is not in mempool. From 6f04c0dd6e68feed2308a8433b3ae9b8f02081ec Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sat, 19 Aug 2023 17:51:36 -0400 Subject: [PATCH 09/21] mempool: fix RBF Rule #2 The current implementation improperly assumed that any unconfirmed inputs spent by the replacement TX were also spent by its direct conflict. It therefore did not account for the case where the replacement was spending an unconfirmed CHILD of its direct replacement. This would cause it to replace its own inputs which is an invalid mempool state and led to unexpected errors. --- lib/mempool/mempool.js | 59 ++++++++++++---------- test/mempool-test.js | 110 ++++++++++++++++++++++++++++++++++------- 2 files changed, 125 insertions(+), 44 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index 4c935af3b..6d1b08200 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -978,7 +978,7 @@ class Mempool extends EventEmitter { conflicts = this.getConflicts(tx); if (conflicts.length) { - this.verifyRBF(entry, conflicts); + this.verifyRBF(entry, view, conflicts); } // Contextual sanity checks. @@ -1038,11 +1038,12 @@ class Mempool extends EventEmitter { * mempool-replacements.md * @method * @param {MempoolEntry} entry + * @param {CoinView} view * @param {MempoolEntry[]} conflicts * @returns {Boolean} */ - verifyRBF(entry, conflicts) { + verifyRBF(entry, view, conflicts) { const tx = entry.tx; let conflictingFees = 0; @@ -1058,6 +1059,36 @@ class Mempool extends EventEmitter { 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. + // This also prevents the case of replacing our own inputs. + PREVOUTS: for (const {prevout} of tx.inputs) { + // Confirmed inputs are not relevant to Rule #2 + if (view.getHeight(prevout) > 0) { + continue; + } + + // Any unconfirmed inputs spent by the replacement + // must also be spent by the conflict. + for (const {prevout: conflictPrevout} of conflict.tx.inputs) { + if (conflictPrevout.hash.equals(prevout.hash) + && conflictPrevout.input === prevout.input) { + // Once we find a match we don't need to check any more + // of the conflict's inputs. Continue by testing the + // next input in the potential replacement tx. + continue PREVOUTS; + } + } + + // Rule #2 violation + throw new VerifyError(tx, + 'nonstandard', + 'replacement-adds-unconfirmed', + 0); + } + conflictingFees += conflict.descFee; totalEvictions += this.countDescendants(conflict) + 1; @@ -1097,30 +1128,6 @@ class Mempool extends EventEmitter { '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); - } } /** diff --git a/test/mempool-test.js b/test/mempool-test.js index 9158fa684..df38070a0 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1362,38 +1362,42 @@ describe('Mempool', function() { }); it('should reject replacement including new unconfirmed UTXO', async() => { + // {confirmed coin 1} {confirmed coin 2} + // | | | + // | tx 1 tx 2 {output} + // | | + // | +--------------------------------+ + // | | + // tx 3 is invalid! + mempool.options.replaceByFee = true; const coin1 = chaincoins.getCoins()[0]; const coin2 = chaincoins.getCoins()[1]; + // tx 1 spends a confirmed coin const mtx1 = new MTX(); - const mtx2 = new MTX(); mtx1.addCoin(coin1); - mtx2.addCoin(coin2); - mtx1.inputs[0].sequence = 0xfffffffd; - const addr1 = chaincoins.createReceive().getAddress(); mtx1.addOutput(addr1, coin1.value - 1000); + chaincoins.sign(mtx1); + assert(mtx1.verify()); + const tx1 = mtx1.toTX(); + assert(tx1.isRBF()); + await mempool.addTX(tx1); + // tx 2 spends a different confirmed coin + const mtx2 = new MTX(); + mtx2.addCoin(coin2); const addr2 = chaincoins.createReceive().getAddress(); mtx2.addOutput(addr2, coin2.value - 1000); - - chaincoins.sign(mtx1); chaincoins.sign(mtx2); - - assert(mtx1.verify()); assert(mtx2.verify()); - const tx1 = mtx1.toTX(); const tx2 = mtx2.toTX(); - - assert(tx1.isRBF()); - - await mempool.addTX(tx1); await mempool.addTX(tx2); - // Attempt to replace tx1 and include the unconfirmed output of tx2 + // Attempt to replace tx 1 and include the unconfirmed output of tx 2 const mtx3 = new MTX(); mtx3.addCoin(coin1); const coin3 = Coin.fromTX(tx2, 0, -1); @@ -1483,12 +1487,20 @@ describe('Mempool', function() { }); it('should accept replacement spending an unconfirmed output', async () => { + // {confirmed coin 1} + // | + // tx 0 {output} + // | | + // tx 1 | + // | + // tx 2 + mempool.options.replaceByFee = true; const addr1 = chaincoins.createReceive().getAddress(); const coin0 = chaincoins.getCoins()[0]; - // Generate parent TX + // Generate parent tx 0 const mtx0 = new MTX(); mtx0.addCoin(coin0); mtx0.addOutput(addr1, coin0.value - 200); @@ -1497,7 +1509,7 @@ describe('Mempool', function() { const tx0 = mtx0.toTX(); await mempool.addTX(tx0); - // Spend unconfirmed output to replaceable child + // Spend unconfirmed output to replaceable child tx 1 const mtx1 = new MTX(); const coin1 = Coin.fromTX(tx0, 0, -1); mtx1.addCoin(coin1); @@ -1508,7 +1520,7 @@ describe('Mempool', function() { const tx1 = mtx1.toTX(); await mempool.addTX(tx1); - // Send replacement + // Send replacement tx 2 const mtx2 = new MTX(); mtx2.addCoin(coin1); mtx2.addOutput(addr1, coin1.value - 400); @@ -1517,7 +1529,7 @@ describe('Mempool', function() { const tx2 = mtx2.toTX(); await mempool.addTX(tx2); - // Unconfirmed parent and replacement in mempool together + // Unconfirmed parent tx 0 and replacement tx 2 are in mempool together assert(mempool.has(tx0.hash())); assert(!mempool.has(tx1.hash())); assert(mempool.has(tx2.hash())); @@ -1564,5 +1576,67 @@ describe('Mempool', function() { reason: 'bad-txns-inputs-spent' }); }); + + it('should not accept replacement that evicts its own inputs', async () => { + // {confirmed coin 1} + // | + // tx 0 {output} + // | | + // | tx 1 {output} + // | | + // | +-------+ + // | | + // tx 2 is invalid! + + mempool.options.replaceByFee = true; + + const addr1 = chaincoins.createReceive().getAddress(); + const coin0 = chaincoins.getCoins()[0]; + + // Generate tx 0 which spends a confirmed coin + const mtx0 = new MTX(); + mtx0.addCoin(coin0); + mtx0.addOutput(addr1, coin0.value - 200); + chaincoins.sign(mtx0); + assert(mtx0.verify()); + const tx0 = mtx0.toTX(); + await mempool.addTX(tx0); + + // Generate tx 1 which spends an output of tx 0 + const mtx1 = new MTX(); + const coin1 = Coin.fromTX(tx0, 0, -1); + mtx1.addCoin(coin1); + mtx1.inputs[0].sequence = 0xfffffffd; + mtx1.addOutput(addr1, coin1.value - 200); + chaincoins.sign(mtx1); + assert(mtx1.verify()); + const tx1 = mtx1.toTX(); + await mempool.addTX(tx1); + + // Send tx 2 which attempts to: + // - replace tx 1 by spending an output of tx 0 + // - ALSO spend an output of tx 1 + // This is obviously invalid because if tx 1 is replaced, + // its output no longer exists so it can not be spent by tx 2. + const mtx2 = new MTX(); + mtx2.addCoin(coin1); + const coin2 = Coin.fromTX(tx1, 0, -1); + mtx2.addCoin(coin2); + mtx2.addOutput(addr1, coin2.value + coin1.value - 1000); + chaincoins.sign(mtx2); + assert(mtx2.verify()); + const tx2 = mtx2.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'replacement-adds-unconfirmed' + }); + + assert(mempool.has(tx0.hash())); + assert(mempool.has(tx1.hash())); + assert(!mempool.has(tx2.hash())); + }); }); }); From 6ae1752e5a408e6e35b67841c4fd9b972d9ad613 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Mon, 21 Aug 2023 10:45:15 -0400 Subject: [PATCH 10/21] mempool: check input index when comparing replacement to conflict --- lib/mempool/mempool.js | 2 +- test/mempool-test.js | 59 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index 6d1b08200..f70ea3bc5 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -1074,7 +1074,7 @@ class Mempool extends EventEmitter { // must also be spent by the conflict. for (const {prevout: conflictPrevout} of conflict.tx.inputs) { if (conflictPrevout.hash.equals(prevout.hash) - && conflictPrevout.input === prevout.input) { + && conflictPrevout.index === prevout.index) { // Once we find a match we don't need to check any more // of the conflict's inputs. Continue by testing the // next input in the potential replacement tx. diff --git a/test/mempool-test.js b/test/mempool-test.js index df38070a0..0f83adfff 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -1638,5 +1638,64 @@ describe('Mempool', function() { assert(mempool.has(tx1.hash())); assert(!mempool.has(tx2.hash())); }); + + it('should not accept replacement that does not evict its own inputs', async () => { + // ...because it spends an unconfirmed coin the conflict did not spend. + + // {confirmed coin 1} + // | + // tx 0 {output 0} {output 1} + // | | | + // tx 1 +-------+ | + // | | + // tx 2 is invalid! + + mempool.options.replaceByFee = true; + + const addr1 = chaincoins.createReceive().getAddress(); + const coin0 = chaincoins.getCoins()[0]; + + // Generate tx 0 which spends a confirmed coin and creates two outputs + const mtx0 = new MTX(); + mtx0.addCoin(coin0); + mtx0.addOutput(addr1, parseInt(coin0.value / 2) - 200); + mtx0.addOutput(addr1, parseInt(coin0.value / 2) - 200); + chaincoins.sign(mtx0); + assert(mtx0.verify()); + const tx0 = mtx0.toTX(); + await mempool.addTX(tx0); + + // Generate tx 1 which spends output 0 of tx 0 + const mtx1 = new MTX(); + const coin1 = Coin.fromTX(tx0, 0, -1); + mtx1.addCoin(coin1); + mtx1.inputs[0].sequence = 0xfffffffd; + mtx1.addOutput(addr1, coin1.value - 200); + chaincoins.sign(mtx1); + assert(mtx1.verify()); + const tx1 = mtx1.toTX(); + await mempool.addTX(tx1); + + // Send tx 2 which spends outputs 0 & 1 of tx 0, replacing tx 1 + const mtx2 = new MTX(); + mtx2.addCoin(coin1); + const coin2 = Coin.fromTX(tx0, 1, -1); + mtx2.addCoin(coin2); + mtx2.addOutput(addr1, coin2.value + coin1.value - 1000); + chaincoins.sign(mtx2); + assert(mtx2.verify()); + const tx2 = mtx2.toTX(); + + await assert.rejects(async () => { + await mempool.addTX(tx2); + }, { + type: 'VerifyError', + reason: 'replacement-adds-unconfirmed' + }); + + assert(mempool.has(tx0.hash())); + assert(mempool.has(tx1.hash())); + assert(!mempool.has(tx2.hash())); + }); }); }); From 24408b9c107aa872f9521875d8171e6aefe81408 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 11 Aug 2023 10:29:38 -0400 Subject: [PATCH 11/21] mempool: allow replacement of tx spending unconfirmed utxo --- lib/mempool/mempool.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index f70ea3bc5..34a363fcb 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -513,8 +513,17 @@ class Mempool extends EventEmitter { if (!entry) return false; - if (this.isSpent(hash, index)) - return false; + const spender = this.getSpent(hash, index); + // If the TX that spent this coin signals + // replaceability, then even though it is "spent" + // we still "have" it. + if (spender) { + if (spender.tx.isRBF()) { + return true; + } else { + return false; + } + } if (index >= entry.tx.outputs.length) return false; From f316989da59ad8f0b8cdad912a15260b90851390 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 1 Aug 2023 16:12:49 -0400 Subject: [PATCH 12/21] mempool: allow RBF by default --- lib/mempool/mempool.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index 34a363fcb..ba9ea72e9 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -2147,7 +2147,7 @@ class MempoolOptions { this.rejectAbsurdFees = true; this.prematureWitness = false; this.paranoidChecks = false; - this.replaceByFee = false; + this.replaceByFee = true; this.maxSize = policy.MEMPOOL_MAX_SIZE; this.maxOrphans = policy.MEMPOOL_MAX_ORPHANS; From f83e39da95d448f1a9f797d3f7f401b685010a98 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 1 Aug 2023 16:22:49 -0400 Subject: [PATCH 13/21] wallet: make all txs replaceable by default, optionally opt out --- bin/bwallet-cli | 2 ++ lib/wallet/http.js | 2 ++ lib/wallet/wallet.js | 5 +++++ 3 files changed, 9 insertions(+) diff --git a/bin/bwallet-cli b/bin/bwallet-cli index 3b2b0bba9..cc853e2ef 100755 --- a/bin/bwallet-cli +++ b/bin/bwallet-cli @@ -298,6 +298,7 @@ class CLI { passphrase: this.config.str('passphrase'), outputs: outputs, smart: this.config.bool('smart'), + replaceable: this.config.bool('replaceable'), rate: this.config.ufixed('rate', 8), subtractFee: this.config.bool('subtract-fee') }; @@ -327,6 +328,7 @@ class CLI { passphrase: this.config.str('passphrase'), outputs: [output], smart: this.config.bool('smart'), + replaceable: this.config.bool('replaceable'), rate: this.config.ufixed('rate', 8), subtractFee: this.config.bool('subtract-fee'), sign: this.config.bool('sign') diff --git a/lib/wallet/http.js b/lib/wallet/http.js index b3ec7d89a..ce194aa8f 100644 --- a/lib/wallet/http.js +++ b/lib/wallet/http.js @@ -447,6 +447,7 @@ class HTTP extends Server { smart: valid.bool('smart'), account: valid.str('account'), sort: valid.bool('sort'), + replaceable: valid.bool('replaceable'), subtractFee: valid.bool('subtractFee'), subtractIndex: valid.i32('subtractIndex'), depth: valid.u32(['confirmations', 'depth']), @@ -495,6 +496,7 @@ class HTTP extends Server { smart: valid.bool('smart'), account: valid.str('account'), sort: valid.bool('sort'), + replaceable: valid.bool('replaceable'), subtractFee: valid.bool('subtractFee'), subtractIndex: valid.i32('subtractIndex'), depth: valid.u32(['confirmations', 'depth']), diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 3703deaf9..0171f2a75 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1172,6 +1172,7 @@ class Wallet extends EventEmitter { * @param {Object} options - See {@link Wallet#fund options}. * @param {Object[]} options.outputs - See {@link MTX#addOutput}. * @param {Boolean} options.sort - Sort inputs and outputs (BIP69). + * @param {Boolean} options.replaceable - Signal BIP 125 replaceability * @param {Boolean} options.template - Build scripts for inputs. * @param {Number} options.locktime - TX locktime * @param {Boolean?} force - Bypass the lock. @@ -1215,6 +1216,10 @@ class Wallet extends EventEmitter { if (options.locktime != null) mtx.setLocktime(options.locktime); + // Opt in BIP 125 replaceability + if (options.replaceable !== false) + mtx.inputs[0].sequence = 0xfffffffd; + // Consensus sanity checks. assert(mtx.isSane(), 'TX failed sanity check.'); assert(mtx.verifyInputs(this.wdb.state.height + 1), From f99a7c23825b708211cb9f8507b0a70a65c6ba7a Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 1 Aug 2023 16:23:16 -0400 Subject: [PATCH 14/21] test: add utility forEvent() --- test/util/common.js | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/util/common.js b/test/util/common.js index b794abbab..8d9b575c0 100644 --- a/test/util/common.js +++ b/test/util/common.js @@ -123,6 +123,50 @@ common.forValue = async function(obj, key, val, timeout = 30000) { }); }; +common.forEvent = async function forEvent(obj, name, count = 1, timeout = 5000) { + assert(typeof obj === 'object'); + assert(typeof name === 'string'); + assert(typeof count === 'number'); + assert(typeof timeout === 'number'); + + let countdown = count; + const events = []; + + return new Promise((resolve, reject) => { + let timeoutHandler, listener; + + const cleanup = function cleanup() { + clearTimeout(timeoutHandler); + obj.removeListener(name, listener); + }; + + listener = function listener(...args) { + events.push({ + event: name, + values: [...args] + }); + + countdown--; + if (countdown === 0) { + cleanup(); + resolve(events); + return; + } + }; + + timeoutHandler = setTimeout(() => { + cleanup(); + const msg = `Timeout waiting for event ${name} ` + + `(received ${count - countdown}/${count})`; + + reject(new Error(msg)); + return; + }, timeout); + + obj.on(name, listener); + }); +}; + function parseUndo(data) { const br = bio.read(data); const items = []; From b8afe61cdd6357526e3f6dd8bed9b08832fd1758 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Thu, 3 Aug 2023 14:32:32 -0400 Subject: [PATCH 15/21] wallet: implement and test bumpTXFee() --- lib/wallet/wallet.js | 62 +++++++++++------ test/wallet-rbf-test.js | 150 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+), 21 deletions(-) create mode 100644 test/wallet-rbf-test.js diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 0171f2a75..0e7a4d680 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1298,13 +1298,15 @@ class Wallet extends EventEmitter { /** * Intentionally double-spend outputs by * increasing fee for an existing transaction. + * Complies with BIP-125 replace-by-fee * @param {Hash} hash * @param {Rate} rate * @param {(String|Buffer)?} passphrase + * @param {Boolean?} sign - sign with wallet * @returns {Promise} - Returns {@link TX}. */ - async increaseFee(hash, rate, passphrase) { + async bumpTXFee(hash, rate, passphrase, sign) { assert((rate >>> 0) === rate, 'Rate must be a number.'); const wtx = await this.getTX(hash); @@ -1317,8 +1319,16 @@ class Wallet extends EventEmitter { const tx = wtx.tx; - if (tx.isCoinbase()) - throw new Error('Transaction is a coinbase.'); + // Require explicit opt-in RBF signalling (for now) BIP 125 rule #1 + if (!tx.isRBF()) + throw new Error('Transaction does not signal opt-in replace-by-fee.'); + + // No child spends + // Covers BIP 125 rule #5 and makes rule #3 fee easier to compute + for (let index = 0; index < tx.outputs.length; index++) { + if (await this.txdb.isSpent(wtx.hash, index)) + throw new Error('Transaction has descendants in the wallet.'); + } const view = await this.getSpentView(tx); @@ -1326,23 +1336,21 @@ class Wallet extends EventEmitter { throw new Error('Not all coins available.'); const oldFee = tx.getFee(view); + // Compute fee for TX with signatures at given rate + const currentFee = tx.getMinFee(null, rate); - let fee = tx.getMinFee(null, rate); - - if (fee > CoinSelector.MAX_FEE) - fee = CoinSelector.MAX_FEE; - - if (oldFee >= fee) - throw new Error('Fee is not increasing.'); - + // Start new TX as copy of old TX + // Covers BIP 125 rule #2 const mtx = MTX.fromTX(tx); mtx.view = view; + // Remove all existing signatures for (const input of mtx.inputs) { input.script.clear(); input.witness.clear(); } + // Find the original TX change output let change; for (let i = 0; i < mtx.outputs.length; i++) { const output = mtx.outputs[i]; @@ -1364,36 +1372,48 @@ class Wallet extends EventEmitter { } if (!change) - throw new Error('No change output.'); + throw new Error('Transaction has no change output.'); + // Start by reducing absolute fee to 0 change.value += oldFee; - if (mtx.getFee() !== 0) throw new Error('Arithmetic error for change.'); - change.value -= fee; + // Pay for replacement fees: BIP 125 rule #3 + change.value -= oldFee; + + // Pay for our own current fee: BIP 125 rule #4 + change.value -= currentFee; if (change.value < 0) - throw new Error('Fee is too high.'); + throw new Error('Change output insufficient for fee.'); + // If change output is below dust, give it all to fee if (change.isDust()) { mtx.outputs.splice(mtx.changeIndex, 1); mtx.changeIndex = -1; } + if (!sign) + return mtx.toTX(); + await this.sign(mtx, passphrase); if (!mtx.isSigned()) - throw new Error('TX could not be fully signed.'); + throw new Error('Replacement TX could not be fully signed.'); const ntx = mtx.toTX(); this.logger.debug( - 'Increasing fee for wallet tx (%s): %h', - this.id, ntx.hash()); - - await this.wdb.addTX(ntx); - await this.wdb.send(ntx); + 'Bumping fee for wallet tx (%s): replacing %h with %h', + this.id, + wtx.hash, + ntx.hash()); + + if (sign) { + await this.wdb.addTX(ntx); + await this.wdb.send(ntx); + } return ntx; } diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js new file mode 100644 index 000000000..aa04ec026 --- /dev/null +++ b/test/wallet-rbf-test.js @@ -0,0 +1,150 @@ +/* eslint-env mocha */ +/* eslint prefer-arrow-callback: "off" */ + +'use strict'; + +const assert = require('bsert'); +const {forEvent} = require('./util/common'); +const FullNode = require('../lib/node/fullnode'); +const MTX = require('../lib/primitives/mtx'); + +const node = new FullNode({ + network: 'regtest', + plugins: [require('../lib/wallet/plugin')] +}); + +let alice = null; +let bob = null; +let aliceReceive = null; +let bobReceive = null; +const {wdb} = node.require('walletdb'); + +describe('Wallet RBF', function () { + before(async () => { + await node.open(); + }); + + after(async () => { + await node.close(); + }); + + it('should create and fund wallet', async () => { + alice = await wdb.create({id: 'alice'}); + bob = await wdb.create({id: 'bob'}); + + aliceReceive = (await alice.receiveAddress()).toString('regtest'); + bobReceive = (await bob.receiveAddress()).toString('regtest'); + + await node.rpc.generateToAddress([110, aliceReceive]); + + const aliceBal = await alice.getBalance(); + assert.strictEqual(aliceBal.confirmed, 110 * 50e8); + + const bobBal = await bob.getBalance(); + assert.strictEqual(bobBal.confirmed, 0); + }); + + it('should not replace missing tx', async () => { + const dummyHash = Buffer.alloc(32, 0x10); + assert.rejects(async () => { + await alice.bumpTXFee(dummyHash, 1000 /* satoshis per kvB */, null, true); + }, { + message: 'Transaction not found.' + }); + }); + + it('should not replace confirmed tx', async () => { + const txs = await alice.getHistory(); + const cb = txs[0]; + assert.rejects(async () => { + await alice.bumpTXFee(cb.hash, 1000 /* satoshis per kvB */, null, true); + }, { + message: 'Transaction is confirmed.' + }); + }); + + it('should not replace a non-replaceable tx', async () => { + const tx = await alice.send({ + outputs: [{ + address: aliceReceive, + value: 1e8 + }], + replaceable: false + }); + + assert(!tx.isRBF()); + + await forEvent(node.mempool, 'tx'); + assert(node.mempool.hasEntry(tx.hash())); + + assert.rejects(async () => { + await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, null, true); + }, { + message: 'Transaction does not signal opt-in replace-by-fee.' + }); + }); + + it('should not replace a wallet tx with child spends', async () => { + const tx1 = await alice.send({ + outputs: [{ + address: aliceReceive, + value: 1e8 + }] + }); + + assert(tx1.isRBF()); + + await forEvent(node.mempool, 'tx'); + assert(node.mempool.hasEntry(tx1.hash())); + + const mtx2 = new MTX(); + mtx2.addTX(tx1, 0); + mtx2.addOutput(aliceReceive, 1e8 - 1000); + mtx2.inputs[0].sequence = 0xfffffffd; + await alice.sign(mtx2); + const tx2 = mtx2.toTX(); + await wdb.addTX(tx2); + await wdb.send(tx2); + + assert(tx2.isRBF()); + + await forEvent(node.mempool, 'tx'); + assert(node.mempool.hasEntry(tx2.hash())); + + assert.rejects(async () => { + await alice.bumpTXFee(tx1.hash(), 1000 /* satoshis per kvB */, null, true); + }, { + message: 'Transaction has descendants in the wallet.' + }); + }); + + it('should replace a replaceable tx', async () => { + const tx = await alice.send({ + outputs: [{ + address: bobReceive, + value: 1e8 + }], + replaceable: true + }); + + assert(tx.isRBF()); + + await forEvent(node.mempool, 'tx'); + assert(node.mempool.has(tx.hash())); + + const rtx = await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, null, true); + + await forEvent(node.mempool, 'tx'); + assert(!node.mempool.hasEntry(tx.hash())); + assert(node.mempool.hasEntry(rtx.hash())); + }); + + it('should only have paid Bob once', async () => { + let bobBal = await bob.getBalance(); + assert.strictEqual(bobBal.unconfirmed, 1e8); + + await node.rpc.generateToAddress([1, aliceReceive]); + bobBal = await bob.getBalance(); + assert.strictEqual(bobBal.confirmed, 1e8); + }); +}); From e83563a6e3945d5d1c9d14843fbc373f108b5d06 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 11 Aug 2023 16:23:05 -0400 Subject: [PATCH 16/21] cli/http: add bwallet-cli bump --- bin/bwallet-cli | 15 +++++++++++++++ lib/client/wallet.js | 28 ++++++++++++++++++++++++++++ lib/wallet/http.js | 17 +++++++++++++++++ lib/wallet/wallet.js | 4 ++-- test/wallet-rbf-test.js | 10 +++++----- 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/bin/bwallet-cli b/bin/bwallet-cli index cc853e2ef..409975f77 100755 --- a/bin/bwallet-cli +++ b/bin/bwallet-cli @@ -364,6 +364,17 @@ class CLI { this.log('Abandoned tx: ' + hash); } + async bumpTX() { + const hash = this.config.str([0, 'hash']); + const rate = this.config.uint([1, 'rate']); + const sign = this.config.bool([2, 'sign']); + const passphrase = this.config.str([3, 'passphrase']); + + const tx = await this.wallet.bumpTX(hash, rate, sign, passphrase); + + this.log(tx); + } + async getDetails() { const hash = this.config.str(0); const details = await this.wallet.getTX(hash); @@ -605,6 +616,9 @@ class CLI { case 'rescan': await this.rescan(); break; + case 'bump': + await this.bumpTX(); + break; default: this.log('Unrecognized command.'); this.log('Commands:'); @@ -617,6 +631,7 @@ class CLI { this.log(' $ balance: Get wallet balance.'); this.log(' $ block [height]: View wallet block.'); this.log(' $ blocks: List wallet blocks.'); + this.log(' $ bump [hash]: Bump TX fee with replacement.'); this.log(' $ change: Derive new change address.'); this.log(' $ coins: View wallet coins.'); this.log(' $ dump [address]: Get wallet key WIF by address.'); diff --git a/lib/client/wallet.js b/lib/client/wallet.js index 812386e26..020de66e8 100644 --- a/lib/client/wallet.js +++ b/lib/client/wallet.js @@ -356,6 +356,21 @@ class WalletClient extends Client { return this.del(`/wallet/${id}/tx/${hash}`); } + /** + * @param {Number} id + * @param {Hash} hash + * @param {Number?} rate + * @param {Bool?} sign + * @param {String?} passphrase + * @returns {Promise} + */ + + bumpTX(id, hash, rate, sign, passphrase) { + return this.post( + `/wallet/${id}/bump/${hash}`, + {hash, rate, sign, passphrase}); + } + /** * Create a transaction, fill. * @param {Number} id @@ -832,6 +847,19 @@ class Wallet extends EventEmitter { return this.client.abandon(this.id, hash); } + /** + * Send an RBF replacement to bump fee + * @param {Hash} hash + * @param {Number?} rate + * @param {Bool?} sign + * @param {String?} passphrase + * @returns {Promise} + */ + + bumpTX(hash, rate, sign, passphrase) { + return this.client.bumpTX(this.id, hash); + } + /** * Create a transaction, fill. * @param {Object} options diff --git a/lib/wallet/http.js b/lib/wallet/http.js index ce194aa8f..6a3749a22 100644 --- a/lib/wallet/http.js +++ b/lib/wallet/http.js @@ -548,6 +548,23 @@ class HTTP extends Server { res.json(200, tx.getJSON(this.network)); }); + // Create replacement fee-bump TX + this.post('/wallet/:id/bump/:hash', async (req, res) => { + const valid = Validator.fromRequest(req); + const hash = valid.brhash('hash'); + enforce(hash, 'txid is required.'); + let rate = valid.u64('rate'); + if (!rate) + rate = this.network.minRelay; + const sign = valid.bool('sign', true); + const passphrase = valid.str('passphrase'); + + // Bump fee by reducing change output value. + const tx = await req.wallet.bumpTXFee(hash, rate, sign, passphrase); + + res.json(200, tx.getJSON(this.network)); + }); + // Zap Wallet TXs this.post('/wallet/:id/zap', async (req, res) => { const valid = Validator.fromRequest(req); diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 0e7a4d680..66ed6bb90 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1301,12 +1301,12 @@ class Wallet extends EventEmitter { * Complies with BIP-125 replace-by-fee * @param {Hash} hash * @param {Rate} rate - * @param {(String|Buffer)?} passphrase * @param {Boolean?} sign - sign with wallet + * @param {(String|Buffer)?} passphrase * @returns {Promise} - Returns {@link TX}. */ - async bumpTXFee(hash, rate, passphrase, sign) { + async bumpTXFee(hash, rate, sign, passphrase) { assert((rate >>> 0) === rate, 'Rate must be a number.'); const wtx = await this.getTX(hash); diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js index aa04ec026..16ba071c1 100644 --- a/test/wallet-rbf-test.js +++ b/test/wallet-rbf-test.js @@ -47,7 +47,7 @@ describe('Wallet RBF', function () { it('should not replace missing tx', async () => { const dummyHash = Buffer.alloc(32, 0x10); assert.rejects(async () => { - await alice.bumpTXFee(dummyHash, 1000 /* satoshis per kvB */, null, true); + await alice.bumpTXFee(dummyHash, 1000 /* satoshis per kvB */, true, null); }, { message: 'Transaction not found.' }); @@ -57,7 +57,7 @@ describe('Wallet RBF', function () { const txs = await alice.getHistory(); const cb = txs[0]; assert.rejects(async () => { - await alice.bumpTXFee(cb.hash, 1000 /* satoshis per kvB */, null, true); + await alice.bumpTXFee(cb.hash, 1000 /* satoshis per kvB */, true, null); }, { message: 'Transaction is confirmed.' }); @@ -78,7 +78,7 @@ describe('Wallet RBF', function () { assert(node.mempool.hasEntry(tx.hash())); assert.rejects(async () => { - await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, null, true); + await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, true, null); }, { message: 'Transaction does not signal opt-in replace-by-fee.' }); @@ -112,7 +112,7 @@ describe('Wallet RBF', function () { assert(node.mempool.hasEntry(tx2.hash())); assert.rejects(async () => { - await alice.bumpTXFee(tx1.hash(), 1000 /* satoshis per kvB */, null, true); + await alice.bumpTXFee(tx1.hash(), 1000 /* satoshis per kvB */, true, null); }, { message: 'Transaction has descendants in the wallet.' }); @@ -132,7 +132,7 @@ describe('Wallet RBF', function () { await forEvent(node.mempool, 'tx'); assert(node.mempool.has(tx.hash())); - const rtx = await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, null, true); + const rtx = await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, true, null); await forEvent(node.mempool, 'tx'); assert(!node.mempool.hasEntry(tx.hash())); From 9a1135ad1291b03da42931656b303494804b115c Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Thu, 21 Sep 2023 10:47:50 -0400 Subject: [PATCH 17/21] wallet: do not replace TX with more than one change address --- lib/wallet/wallet.js | 8 ++++++-- test/wallet-rbf-test.js | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 66ed6bb90..5f54962c5 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1351,7 +1351,7 @@ class Wallet extends EventEmitter { } // Find the original TX change output - let change; + let change = null; for (let i = 0; i < mtx.outputs.length; i++) { const output = mtx.outputs[i]; const addr = output.getAddress(); @@ -1365,9 +1365,13 @@ class Wallet extends EventEmitter { continue; if (path.branch === 1) { + // Edge case if (for example) a customer requests a withdrawl + // to one of our own change addresses. + // TODO: Allow bumpTXFee() caller to specify an output to reduce. + if (change) + throw new Error('Found more than one change address.'); change = output; mtx.changeIndex = i; - break; } } diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js index 16ba071c1..14780083e 100644 --- a/test/wallet-rbf-test.js +++ b/test/wallet-rbf-test.js @@ -147,4 +147,23 @@ describe('Wallet RBF', function () { bobBal = await bob.getBalance(); assert.strictEqual(bobBal.confirmed, 1e8); }); + + it('should not send replacement if original has more than one change address', async () => { + const changeAddr = (await alice.createChange()).getAddress('string'); + const tx = await alice.send({ + outputs: [{ + address: changeAddr, + value: 1e8 + }], + replaceable: true + }); + await forEvent(node.mempool, 'tx'); + + assert.rejects(async () => { + await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, true, null); + }, { + message: 'Found more than one change address.' + }); + await node.rpc.generateToAddress([1, aliceReceive]); + }); }); From d5d302ccc1bdab61d6e06a2c993781487563e351 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Thu, 21 Sep 2023 11:39:15 -0400 Subject: [PATCH 18/21] wallet: do not accept too-low RBF replacement fee rate --- lib/wallet/wallet.js | 1 + test/wallet-rbf-test.js | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 5f54962c5..955f14f6a 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1308,6 +1308,7 @@ class Wallet extends EventEmitter { async bumpTXFee(hash, rate, sign, passphrase) { assert((rate >>> 0) === rate, 'Rate must be a number.'); + assert(rate >= this.network.minRelay, 'Provided fee rate is too low.'); const wtx = await this.getTX(hash); diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js index 14780083e..8d96d47f0 100644 --- a/test/wallet-rbf-test.js +++ b/test/wallet-rbf-test.js @@ -166,4 +166,24 @@ describe('Wallet RBF', function () { }); await node.rpc.generateToAddress([1, aliceReceive]); }); + + it('should not send replacement with too-low fee rate', async () => { + const tx = await alice.send({ + outputs: [{ + address: bobReceive, + value: 1e8 + }], + replaceable: true, + rate: 100000 + }); + await forEvent(node.mempool, 'tx'); + + assert.rejects(async () => { + // Try a fee rate below minRelay (1000) + await alice.bumpTXFee(tx.hash(), 999 /* satoshis per kvB */, true, null); + }, { + message: 'Provided fee rate is too low.' + }); + await node.rpc.generateToAddress([1, aliceReceive]); + }); }); From 4d974807de28d208ee49e57df4943a8fda750f21 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Thu, 21 Sep 2023 16:08:19 -0400 Subject: [PATCH 19/21] wallet rbf: add new in/out if necessary --- lib/primitives/mtx.js | 16 +++++++++- lib/wallet/coinselector.js | 8 ++++- lib/wallet/wallet.js | 62 ++++++++++++++++++++++++-------------- test/wallet-rbf-test.js | 31 +++++++++++++++++++ 4 files changed, 92 insertions(+), 25 deletions(-) diff --git a/lib/primitives/mtx.js b/lib/primitives/mtx.js index 3fc58da0c..68716fe7f 100644 --- a/lib/primitives/mtx.js +++ b/lib/primitives/mtx.js @@ -1227,11 +1227,25 @@ class MTX extends TX { async fund(coins, options) { assert(options, 'Options are required.'); assert(options.changeAddress, 'Change address is required.'); - assert(this.inputs.length === 0, 'TX is already funded.'); + if (!options.bumpFee) + assert(this.inputs.length === 0, 'TX is already funded.'); // Select necessary coins. const select = await this.selectCoins(coins, options); + // Bump Fee mode: + // We want the coin selector to ignore the values of + // all existing inputs and outputs, but still consider their size. + // Inside the coin selector this is done by making a copy of the TX + // and then setting the total output value to 0 + // (input value is already ignored). + // The coin selector will add coins to cover the fee on the entire TX + // including paying for any inputs it adds. + // Now that coin selection is done, we add back in the fee paid by + // the original existing inputs and outputs so we can set the change value. + if (options.bumpFee) + select.fee += this.getFee(); + // Add coins to transaction. for (const coin of select.chosen) this.addCoin(coin); diff --git a/lib/wallet/coinselector.js b/lib/wallet/coinselector.js index ee57bff92..bd50af894 100644 --- a/lib/wallet/coinselector.js +++ b/lib/wallet/coinselector.js @@ -71,6 +71,7 @@ class CoinSelector { this.changeAddress = null; this.inputs = new BufferMap(); this.useSelectEstimate = false; + this.bumpFee = false; // Needed for size estimation. this.getAccount = null; @@ -158,6 +159,11 @@ class CoinSelector { this.useSelectEstimate = options.useSelectEstimate; } + if (options.bumpFee != null) { + assert(typeof options.bumpFee === 'boolean'); + this.bumpFee = options.bumpFee; + } + if (options.changeAddress) { const addr = options.changeAddress; if (typeof addr === 'string') { @@ -209,7 +215,7 @@ class CoinSelector { async init(coins) { this.coins = coins.slice(); - this.outputValue = this.tx.getOutputValue(); + this.outputValue = this.bumpFee ? 0 : this.tx.getOutputValue(); this.chosen = []; this.change = 0; this.fee = CoinSelector.MIN_FEE; diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 955f14f6a..729a5d6c9 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1143,7 +1143,8 @@ class Wallet extends EventEmitter { rate: rate, maxFee: options.maxFee, useSelectEstimate: options.useSelectEstimate, - getAccount: this.getAccountByAddress.bind(this) + getAccount: this.getAccountByAddress.bind(this), + bumpFee: options.bumpFee }); assert(mtx.getFee() <= CoinSelector.MAX_FEE, 'TX exceeds MAX_FEE.'); @@ -1376,27 +1377,42 @@ class Wallet extends EventEmitter { } } - if (!change) - throw new Error('Transaction has no change output.'); - - // Start by reducing absolute fee to 0 - change.value += oldFee; - if (mtx.getFee() !== 0) - throw new Error('Arithmetic error for change.'); - - // Pay for replacement fees: BIP 125 rule #3 - change.value -= oldFee; - - // Pay for our own current fee: BIP 125 rule #4 - change.value -= currentFee; - - if (change.value < 0) - throw new Error('Change output insufficient for fee.'); - - // If change output is below dust, give it all to fee - if (change.isDust()) { - mtx.outputs.splice(mtx.changeIndex, 1); - mtx.changeIndex = -1; + if (change) { + // Start by reducing absolute fee to 0 + change.value += oldFee; + if (mtx.getFee() !== 0) + throw new Error('Arithmetic error for change.'); + + // Pay for replacement fees: BIP 125 rule #3 + change.value -= oldFee; + + // Pay for our own current fee: BIP 125 rule #4 + change.value -= currentFee; + + // We need to add more inputs. + // This will obviously also fail the dust test next + // and conveniently remove the change output for us. + if (change.value < 0) + throw new Error('Change value insufficient to bump fee.'); + + // If change output is below dust, + // give it all to fee (remove change output) + if (change.isDust()) { + mtx.outputs.splice(mtx.changeIndex, 1); + mtx.changeIndex = -1; + } + } else { + // We need to add more inputs (and maybe a change output) to increase the + // fee. Since the original inputs and outputs already paid for + // their own fee (rule #3) all we have to do is pay for this + // new TX's fee (rule #4). + await this.fund( + mtx, + { + bumpFee: true, // set bump fee mode to ignore existing output values + rate, // rate in s/kvB for the rule 4 fee + depth: 1 // replacements can not add new unconfirmed coins + }); } if (!sign) @@ -1420,7 +1436,7 @@ class Wallet extends EventEmitter { await this.wdb.send(ntx); } - return ntx; + return mtx; } /** diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js index 8d96d47f0..188d19b04 100644 --- a/test/wallet-rbf-test.js +++ b/test/wallet-rbf-test.js @@ -186,4 +186,35 @@ describe('Wallet RBF', function () { }); await node.rpc.generateToAddress([1, aliceReceive]); }); + + it('should bump a tx with no change by adding new in/out pair', async () => { + const coins = await alice.getCoins(); + let coin; + for (coin of coins) { + if (!coin.coinbase) + break; + } + const mtx = new MTX(); + mtx.addCoin(coin); + mtx.addOutput(bobReceive, coin.value - 200); + mtx.inputs[0].sequence = 0xfffffffd; + await alice.sign(mtx); + const tx = mtx.toTX(); + assert.strictEqual(tx.inputs.length, 1); + assert.strictEqual(tx.outputs.length, 1); + await alice.wdb.addTX(tx); + await alice.wdb.send(tx); + await forEvent(node.mempool, 'tx'); + + const rtx = await alice.bumpTXFee(tx.hash(), 2000 /* satoshis per kvB */, true, null); + assert.strictEqual(rtx.inputs.length, 2); + assert.strictEqual(rtx.outputs.length, 2); + assert(rtx.getRate() >= 2000 && rtx.getRate() < 3000); + + await forEvent(node.mempool, 'tx'); + assert(!node.mempool.hasEntry(tx.hash())); + assert(node.mempool.hasEntry(rtx.hash())); + + await node.rpc.generateToAddress([1, aliceReceive]); + }); }); From 5f472ad92318f8d3609f4ed40d5078303604ad73 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 22 Sep 2023 11:52:37 -0400 Subject: [PATCH 20/21] wallet rbf: do not violate rule 6 --- lib/wallet/wallet.js | 26 ++++++++++++++++++++++++-- test/wallet-rbf-test.js | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 729a5d6c9..49ecba5d9 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1309,7 +1309,7 @@ class Wallet extends EventEmitter { async bumpTXFee(hash, rate, sign, passphrase) { assert((rate >>> 0) === rate, 'Rate must be a number.'); - assert(rate >= this.network.minRelay, 'Provided fee rate is too low.'); + assert(rate >= this.network.minRelay, 'Fee rate is below minimum.'); const wtx = await this.getTX(hash); @@ -1415,14 +1415,36 @@ class Wallet extends EventEmitter { }); } - if (!sign) + const oldRate = tx.getRate(view); + + if (!sign) { + // Estimate final fee after signing for rule 6 check + const estSize = + await mtx.estimateSize(this.getAccountByAddress.bind(this)); + const fee = mtx.getFee(); + const estRate = policy.getRate(estSize, fee); + + if (estRate <= oldRate) { + throw new Error(`Provided fee rate of ${rate} s/kvB results in ` + + `insufficient estimated total fee rate (${estRate}) ` + + `to replace original (${oldRate})`); + } + return mtx.toTX(); + } await this.sign(mtx, passphrase); if (!mtx.isSigned()) throw new Error('Replacement TX could not be fully signed.'); + const newRate = mtx.getRate(); + if (newRate <= oldRate) { + throw new Error(`Provided fee rate of ${rate} s/kvB results in ` + + `insufficient total fee rate (${newRate}) ` + + `to replace original (${oldRate})`); + } + const ntx = mtx.toTX(); this.logger.debug( diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js index 188d19b04..76bcb88b5 100644 --- a/test/wallet-rbf-test.js +++ b/test/wallet-rbf-test.js @@ -182,7 +182,7 @@ describe('Wallet RBF', function () { // Try a fee rate below minRelay (1000) await alice.bumpTXFee(tx.hash(), 999 /* satoshis per kvB */, true, null); }, { - message: 'Provided fee rate is too low.' + message: 'Fee rate is below minimum.' }); await node.rpc.generateToAddress([1, aliceReceive]); }); @@ -217,4 +217,40 @@ describe('Wallet RBF', function () { await node.rpc.generateToAddress([1, aliceReceive]); }); + + it('should not violate rule 6 signed or unsigned', async () => { + const coins = await alice.getCoins(); + let coin; + for (coin of coins) { + if (!coin.coinbase) + break; + } + const mtx = new MTX(); + mtx.addCoin(coin); + mtx.addOutput(bobReceive, coin.value - 200); + mtx.inputs[0].sequence = 0xfffffffd; + await alice.sign(mtx); + const tx = mtx.toTX(); + assert.strictEqual(tx.inputs.length, 1); + assert.strictEqual(tx.outputs.length, 1); + await alice.wdb.addTX(tx); + await alice.wdb.send(tx); + await forEvent(node.mempool, 'tx'); + + // Do not sign, estimate fee rate + await assert.rejects(async () => { + await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, false, null); + }, { + message: /^Provided fee rate of 1000 s\/kvB results in insufficient estimated total fee rate/ + }); + + // Do sign, then check fee rate + await assert.rejects(async () => { + await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, true, null); + }, { + message: /^Provided fee rate of 1000 s\/kvB results in insufficient total fee rate/ + }); + + await node.rpc.generateToAddress([1, aliceReceive]); + }); }); From 14e57439dc1dd927380e756da03dc6cad5205a5b Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 22 Sep 2023 18:57:57 -0400 Subject: [PATCH 21/21] wallet rbf: add new in/out when change output is too low for RBF fee --- lib/wallet/wallet.js | 44 +++++++++++++++++------------ test/wallet-rbf-test.js | 61 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 17 deletions(-) diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 49ecba5d9..21ed2b655 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1337,10 +1337,6 @@ class Wallet extends EventEmitter { if (!tx.hasCoins(view)) throw new Error('Not all coins available.'); - const oldFee = tx.getFee(view); - // Compute fee for TX with signatures at given rate - const currentFee = tx.getMinFee(null, rate); - // Start new TX as copy of old TX // Covers BIP 125 rule #2 const mtx = MTX.fromTX(tx); @@ -1377,31 +1373,36 @@ class Wallet extends EventEmitter { } } + let changeAdjust = 0; if (change) { - // Start by reducing absolute fee to 0 - change.value += oldFee; - if (mtx.getFee() !== 0) - throw new Error('Arithmetic error for change.'); + const originalFee = mtx.getFee(); + const originalChangeValue = change.value; - // Pay for replacement fees: BIP 125 rule #3 - change.value -= oldFee; + // Compute fee for TX with signatures at given rate + const currentFee = tx.getMinFee(null, rate); // Pay for our own current fee: BIP 125 rule #4 change.value -= currentFee; - // We need to add more inputs. - // This will obviously also fail the dust test next - // and conveniently remove the change output for us. - if (change.value < 0) - throw new Error('Change value insufficient to bump fee.'); - // If change output is below dust, // give it all to fee (remove change output) if (change.isDust()) { mtx.outputs.splice(mtx.changeIndex, 1); mtx.changeIndex = -1; + + if (change.value < 0) { + // The existing change output wasn't enough to pay the RBF fee. + // Remove it completely and add more inputs and maybe a new change. + change = null; + // We will subtract the rule 3 (original tx) fee after re-funding. + changeAdjust = originalFee - originalChangeValue; + } } - } else { + } + + // Note this is not just an "else" because the prior code block + // might have removed an existing change. + if (!change) { // We need to add more inputs (and maybe a change output) to increase the // fee. Since the original inputs and outputs already paid for // their own fee (rule #3) all we have to do is pay for this @@ -1413,6 +1414,15 @@ class Wallet extends EventEmitter { rate, // rate in s/kvB for the rule 4 fee depth: 1 // replacements can not add new unconfirmed coins }); + + if (changeAdjust) { + // This edge case can be avoided with a more complex coin selector + assert( + mtx.changeIndex !== -1, + 'RBF re-funding of changeless tx resulted in changeless solution'); + // Add back in the value from the original change output we removed + mtx.outputs[mtx.changeIndex].value -= changeAdjust; + } } const oldRate = tx.getRate(view); diff --git a/test/wallet-rbf-test.js b/test/wallet-rbf-test.js index 76bcb88b5..087b6e3d5 100644 --- a/test/wallet-rbf-test.js +++ b/test/wallet-rbf-test.js @@ -253,4 +253,65 @@ describe('Wallet RBF', function () { await node.rpc.generateToAddress([1, aliceReceive]); }); + + it('should remove change and pay to fees if below dust', async () => { + const coins = await alice.getCoins(); + let coin; + for (coin of coins) { + if (!coin.coinbase) + break; + } + const mtx = new MTX(); + const changeAddr = (await alice.createChange()).getAddress('string'); + mtx.addCoin(coin); + mtx.addOutput(bobReceive, coin.value - 400 - 141); // Bob gets most of it + + mtx.addOutput(changeAddr, 400); // small change output + assert(!mtx.outputs[1].isDust()); // not dust yet but will be after RBF + mtx.inputs[0].sequence = 0xfffffffd; + await alice.sign(mtx); + const tx = mtx.toTX(); + await alice.wdb.addTX(tx); + await alice.wdb.send(tx); + await forEvent(node.mempool, 'tx'); + + const rtx = await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, true, null); + + assert.strictEqual(rtx.outputs.length, 1); // change output was removed + + await forEvent(node.mempool, 'tx'); + assert(!node.mempool.hasEntry(tx.hash())); + assert(node.mempool.hasEntry(rtx.hash())); + + await node.rpc.generateToAddress([1, aliceReceive]); + }); + + it('should add inputs if change output is insufficient for RBF', async () => { + const coins = await alice.getCoins(); + let coin; + for (coin of coins) { + if (!coin.coinbase) + break; + } + const mtx = new MTX(); + const changeAddr = (await alice.createChange()).getAddress('string'); + mtx.addCoin(coin); + mtx.addOutput(bobReceive, coin.value - 100 - 141); // Bob gets most of it + mtx.addOutput(changeAddr, 100); // change too small to pay for fee bump + + mtx.inputs[0].sequence = 0xfffffffd; + await alice.sign(mtx); + const tx = mtx.toTX(); + await alice.wdb.addTX(tx); + await alice.wdb.send(tx); + await forEvent(node.mempool, 'tx'); + + const rtx = await alice.bumpTXFee(tx.hash(), 1000 /* satoshis per kvB */, true, null); + + await forEvent(node.mempool, 'tx'); + assert(!node.mempool.hasEntry(tx.hash())); + assert(node.mempool.hasEntry(rtx.hash())); + + await node.rpc.generateToAddress([1, aliceReceive]); + }); });