Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet: add BIP125 RBF bump fee #1170

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Aug 11, 2023

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
Copy link

codecov bot commented Aug 11, 2023

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.
@pinheadmz pinheadmz marked this pull request as ready for review September 22, 2023 22:58
@bolaum
Copy link
Contributor

bolaum commented Oct 5, 2023

This is awesome!

@andriibezkorovainyi
Copy link

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
Copy link

andriibezkorovainyi commented Nov 17, 2023

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
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants