Wallet: add BIP125 RBF bump fee #1170

pinheadmz · 2023-08-11T20:25:10Z

based on #811

CLI API:

bwallet-cli bump <txid> <incremental fee rate> <sign> <passphrase>

HTTP API:

/wallet/:id/bump/:hash POST --data {hash, rate, sign, passphrase}

default incremental fee rate is network minRelay in s/vkB
default sign is true
default passphrase is null

Returns new TX JSON object. If sign is true, also signs and broadcasts

TODO:

add tests for http endpoint and cover all options
add ability to add additional (confirmed) inputs if necessary

"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.

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.

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.

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.

codecov · 2023-08-11T20:45:32Z

Codecov Report

Patch coverage: 83.45% and project coverage change: +0.43% 🎉

Comparison is base (0c18028) 70.41% compared to head (14e5743) 70.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1170      +/-   ##
==========================================
+ Coverage   70.41%   70.85%   +0.43%     
==========================================
  Files         174      174              
  Lines       27515    27609      +94     
==========================================
+ Hits        19376    19563     +187     
+ Misses       8139     8046      -93

Files Changed	Coverage Δ
lib/client/wallet.js	`54.19% <0.00%> (-0.85%)`	⬇️
lib/wallet/http.js	`52.75% <9.09%> (-0.80%)`	⬇️
lib/mempool/mempool.js	`69.18% <87.87%> (+3.64%)`	⬆️
lib/wallet/wallet.js	`76.50% <95.45%> (+7.30%)`	⬆️
lib/primitives/mtx.js	`77.29% <100.00%> (+0.12%)`	⬆️
lib/protocol/policy.js	`88.88% <100.00%> (+0.20%)`	⬆️
lib/wallet/coinselector.js	`86.75% <100.00%> (+0.96%)`	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

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.

bolaum · 2023-10-05T19:32:08Z

This is awesome!

andriibezkorovainyi · 2023-10-27T12:46:26Z

Hello, thank you a lot @pinheadmz for your work and attention. I've been working on a RBF feature of my app using this branch. Yes, it works great. It would be totally complete with support of subtractFee option set to true, but for my purposes, it's completely OK without it. And I also want to admire your help to solve the issues, again, thank you.

andriibezkorovainyi · 2023-11-17T01:23:44Z

Bug report

Basically what I'm doing is inputs consolidation:

I take all of my wallet coins
Filter them for confirmation and !isDust()
Sort and take n lowest value inputs
Adding output with almost all of input values sum to my primary account receive address(This is how it works for me)
Fund my mtx with these inputs
Sign and broadcast

When I'm trying to perform an rbf on such tx, increasing fee significantly(passing 2000 rate to the rbf method and more), the error on node occures and rbf doesn't work

Code of tx constructing:

   coins = coins
     .map((c) => {
       const coin = new CoinClass({
         ...c,
         hash: Buffer.from(c.hash as string, "hex").reverse(),
       });

       if (coin.height === -1 || coin.isDust(rate)) return null;

       coin.script = ScriptClass.fromRaw(Buffer.from(c.script as string, "hex"));
       return coin;
     })
     .filter((c) => c !== null)
     .sort((a, b) => b.value - a.value)
     .slice(-inputsCount);

   const mtx = new MTX();

   let totalValue: any = 0;
   for (const coin of coins) totalValue += coin.value;
   totalValue = Math.round(totalValue * 0.9); // Rest for change

   // Adding main output
   const output = new OutputClass({
     address: primaryAccount.receiveAddress,
     value: totalValue,
   });
   mtx.outputs.push(output);
   primaryAccount.receiveAddress);

   await mtx.fund(coins, {
     rate,
     changeAddress: rawPrimaryAccount.changeAddress,
     useSelectEstimate: true,
   });

   // Making it replacable
   mtx.inputs[0].sequence = 0xfffffffd;

   const options = {
     tx: mtx.toRaw().toString("hex"),
     passphrase: config.bitcoin.passphrase,
   };
   const signedTx = await wallet.sign(options);
   await this.nodeClient.broadcast(signedTx.hex);

My tx basically had:

50 inputs with 0.00010357 BTC each
10 sat/vbyte rate
two outputs: main with 0.00441359 BTC and change with 0.00003248 BTC
tx info

I've tried to rbf it with this method
/wallet/:id/bump/:hash POST --data {hash, rate, sign, passphrase}
and passed 5000 as rate parameter

Stacktrace:

[debug] (wallet) Bumping fee for wallet tx (pzitly3hfbr0bsfprlfh5pxzqnlk2x01): replacing d5de8af3bbf044c6a45496f1dcb209b665f49878cc86e917c0d79641fcbcdb5c with 1b420fdef6c94762d58a3505a719b67b055e9145bd023416d9f597b9eaa2ac31
[info] (wallet) Incoming transaction for 1 wallets in WalletDB (1b420fdef6c94762d58a3505a719b67b055e9145bd023416d9f597b9eaa2ac31).
[warning] (wallet) Handling conflicting tx: d5de8af3bbf044c6a45496f1dcb209b665f49878cc86e917c0d79641fcbcdb5c.
[warning] (wallet) Removed conflict: d5de8af3bbf044c6a45496f1dcb209b665f49878cc86e917c0d79641fcbcdb5c.
[warning] (wallet) Handling conflicting tx: d5de8af3bbf044c6a45496f1dcb209b665f49878cc86e917c0d79641fcbcdb5c.
[warning] (wallet) Handling conflicting tx: d5de8af3bbf044c6a45496f1dcb209b665f49878cc86e917c0d79641fcbcdb5c.
And many duplicated lines like above last two, pasting only a couple

[error] (node) Invalid type for database key.
    at Object.write (/opt/bcoin/node_modules/bdb/lib/key.js:118:7)
    at BaseKey.encode (/opt/bcoin/node_modules/bdb/lib/key.js:398:20)
    at Key.encode (/opt/bcoin/node_modules/bdb/lib/key.js:478:22)
    at TXDB.saveUnspentCredit (/opt/bcoin/lib/wallet/txdb.js:138:16)
    at TXDB.saveCredit (/opt/bcoin/lib/wallet/txdb.js:123:12)
    at TXDB.insert (/opt/bcoin/lib/wallet/txdb.js:606:18)
    at async Wallet._add (/opt/bcoin/lib/wallet/wallet.js:1915:21)
    at async Wallet.add (/opt/bcoin/lib/wallet/wallet.js:1899:14)
    at async WalletDB._addTX (/opt/bcoin/lib/wallet/walletdb.js:2184:11)
    at async WalletDB.addTX (/opt/bcoin/lib/wallet/walletdb.js:2146:14)

It looks like the error appears when it tries to add inputs or subtract fee from existing change. With small txes and rates, everything works. Obviously, my code is not perfect, but i think bumpTXFee might need some more additionl checks

andriibezkorovainyi · 2023-11-17T15:24:18Z

And also I've another situation of not working. It's not really seems like a bug, but kinda strange behaviour.
If I add all of my totalValue without multiplying on 0.9 and pass subtractFee: true to .fund method

const mtx = new MTX();

...
 let totalValue = 0;
 for (const coin of coins) totalValue += coin.value;

...

 await mtx.fund(coins, {
    rate,
    changeAddress: rawPrimaryAccount.changeAddress,
    subtractFee: true,
    useSelectEstimate: true,
 });
 ...

In this case there will be no change output
When I perform rbf of this tx, passing 1000 rate parameter, the method returns the hash of new tx, generates logs of successfull replacement, but the node then fails verification, indicating the insufficient fee, again it looks like it couldn't add some inputs to cover fee.

Stacktrace:

[debug] (wallet) Bumping fee for wallet tx (pzitly3hfbr0bsfprlfh5pxzqnlk2x01): replacing fae867cd27b6b583c297c544041b8ce0b91f1c263532d1bb3387e7813977f5fb with 5cb45349c5d733049044beabfe8d3d6d3841fe4d42b3cfea49014f640bc20962
[info] (wallet) Incoming transaction for 1 wallets in WalletDB (5cb45349c5d733049044beabfe8d3d6d3841fe4d42b3cfea49014f640bc20962).
[warning] (wallet) Handling conflicting tx: fae867cd27b6b583c297c544041b8ce0b91f1c263532d1bb3387e7813977f5fb.
[warning] (wallet) Removed conflict: fae867cd27b6b583c297c544041b8ce0b91f1c263532d1bb3387e7813977f5fb.
[warning] (wallet) Handling conflicting tx: fae867cd27b6b583c297c544041b8ce0b91f1c263532d1bb3387e7813977f5fb.
[warning] (wallet) Handling conflicting tx: fae867cd27b6b583c297c544041b8ce0b91f1c263532d1bb3387e7813977f5fb.
[info] (wallet) Added transaction to wallet in WalletDB: pzitly3hfbr0bsfprlfh5pxzqnlk2x01 (4).
[error] (node) Verification failure: insufficient fee: must pay for fees including conflicts (code=insufficientfee score=0 hash=5cb45349c5d733049044beabfe8d3d6d3841fe4d42b3cfea49014f640bc20962)
    at Mempool.verifyRBF (/opt/bcoin/lib/mempool/mempool.js:1135:13)
    at Mempool.verify (/opt/bcoin/lib/mempool/mempool.js:990:12)
    at async Mempool.insertTX (/opt/bcoin/lib/mempool/mempool.js:860:23)
    at async Mempool._addTX (/opt/bcoin/lib/mempool/mempool.js:715:17)
    at async Mempool.addTX (/opt/bcoin/lib/mempool/mempool.js:694:14)
    at async FullNode.sendTX (/opt/bcoin/lib/node/fullnode.js:391:17)
    at async FullNode.relay (/opt/bcoin/lib/node/fullnode.js:424:7)

pinheadmz added 5 commits August 11, 2023 12:40

mempool: Allow double spend of RBF transaction

0a3ecf2

"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.

mempool: replace by fee

70ef286

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.

mempool: clarify eviction of RBF conflicts

f3cd659

pinheadmz force-pushed the rbf1-wallet branch from 86b081b to 3b1dc6b Compare August 11, 2023 20:36

pinheadmz added 13 commits August 19, 2023 14:46

mempool: improve readability and documentation of RBF logic

bf43d9a

mempool: allow replacement of tx spending unconfirmed utxo

caabf10

mempool: check input index when comparing replacement to conflict

6ae1752

mempool: allow replacement of tx spending unconfirmed utxo

24408b9

mempool: allow RBF by default

f316989

wallet: make all txs replaceable by default, optionally opt out

f83e39d

test: add utility forEvent()

f99a7c2

wallet: implement and test bumpTXFee()

b8afe61

cli/http: add bwallet-cli bump <txid> <rate> <sign> <passphrase>

e83563a

wallet: do not replace TX with more than one change address

9a1135a

wallet: do not accept too-low RBF replacement fee rate

d5d302c

pinheadmz force-pushed the rbf1-wallet branch from 3b1dc6b to 368195a Compare September 21, 2023 20:08

wallet rbf: add new in/out if necessary

4d97480

pinheadmz force-pushed the rbf1-wallet branch from 368195a to 4d97480 Compare September 22, 2023 14:49

pinheadmz added 2 commits September 22, 2023 11:52

wallet rbf: do not violate rule 6

5f472ad

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

14e5743

pinheadmz marked this pull request as ready for review September 22, 2023 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet: add BIP125 RBF bump fee #1170

Wallet: add BIP125 RBF bump fee #1170

pinheadmz commented Aug 11, 2023 •

edited

Loading

codecov bot commented Aug 11, 2023 •

edited

Loading

bolaum commented Oct 5, 2023

andriibezkorovainyi commented Oct 27, 2023

andriibezkorovainyi commented Nov 17, 2023 •

edited

Loading

andriibezkorovainyi commented Nov 17, 2023

Wallet: add BIP125 RBF bump fee #1170

Are you sure you want to change the base?

Wallet: add BIP125 RBF bump fee #1170

Conversation

pinheadmz commented Aug 11, 2023 • edited Loading

codecov bot commented Aug 11, 2023 • edited Loading

Codecov Report

bolaum commented Oct 5, 2023

andriibezkorovainyi commented Oct 27, 2023

andriibezkorovainyi commented Nov 17, 2023 • edited Loading

Bug report

andriibezkorovainyi commented Nov 17, 2023

pinheadmz commented Aug 11, 2023 •

edited

Loading

codecov bot commented Aug 11, 2023 •

edited

Loading

andriibezkorovainyi commented Nov 17, 2023 •

edited

Loading