search

bcoin-org / bcoin

Javascript bitcoin library for node.js and browsers
https://bcoin.io
Other
3.01k stars 811 forks source link

Wallet: add BIP125 RBF bump fee #1170

Open pinheadmz opened 1 year ago

pinheadmz commented 1 year ago

based on https://github.com/bcoin-org/bcoin/pull/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:

codecov[bot] commented 1 year ago

Codecov Report

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

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org) | Coverage Δ | | |---|---|---| | [lib/client/wallet.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL2NsaWVudC93YWxsZXQuanM=) | `54.19% <0.00%> (-0.85%)` | :arrow_down: | | [lib/wallet/http.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3dhbGxldC9odHRwLmpz) | `52.75% <9.09%> (-0.80%)` | :arrow_down: | | [lib/mempool/mempool.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL21lbXBvb2wvbWVtcG9vbC5qcw==) | `69.18% <87.87%> (+3.64%)` | :arrow_up: | | [lib/wallet/wallet.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3dhbGxldC93YWxsZXQuanM=) | `76.50% <95.45%> (+7.30%)` | :arrow_up: | | [lib/primitives/mtx.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3ByaW1pdGl2ZXMvbXR4Lmpz) | `77.29% <100.00%> (+0.12%)` | :arrow_up: | | [lib/protocol/policy.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3Byb3RvY29sL3BvbGljeS5qcw==) | `88.88% <100.00%> (+0.20%)` | :arrow_up: | | [lib/wallet/coinselector.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3dhbGxldC9jb2luc2VsZWN0b3IuanM=) | `86.75% <100.00%> (+0.96%)` | :arrow_up: | ... and [10 files with indirect coverage changes](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1170/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bolaum commented 1 year ago

This is awesome!

andriibezkorovainyi commented 11 months ago

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 commented 10 months ago

Bug report

Basically what I'm doing is inputs consolidation:

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:

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 commented 10 months ago

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)