wallet rbf: add new in/out when change output is too low for RBF fee

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);

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]);
+  });
 });