search

bcoin-org / bcoin

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

Discussion: replace-by-fee policy #814

Open pinheadmz opened 5 years ago

pinheadmz commented 5 years ago

I have a PR up for enabling RBF in the mempool: https://github.com/bcoin-org/bcoin/pull/811 and I'm going to start working on the wallet functionality next. Question for everyone is how to implement the settings for RBF?

Proposal

  1. Remove the replace-by-fee mempool option completely. There is really no reason to reject RBF transactions from the mempool. It will only adversely affect fee estimation and doesn't really add any extra protection to the wallet since unconfirmed transactions are always replaceable by a confirmed transaction anyway. The mempool must accept RBF transactions or the wallet will not be able to generate them.

  2. Allow new config option wallet-rbf or wallet-replace-by-fee that sets the default behavior for ALL outgoing transactions. This option will apply to ALL wallets and accounts, basically a global option for the entire walletDB instance whether it be a plugin or standalone server. My recommendation is that this will be default true.

  3. No matter how the global option is set, rbf can be set on a per-transaction basis for API calls like send, createrawtransaction, etc. It will be implemented in wallet.createTX() somewhere...

  4. Obviously new commands and API endpoints will be needed to actually BUMP the fee on a sent transaction. According to BIP125, the minimum bump amount is the minimum network relay fee. That would be the default bump unless the user specifies a larger fee bump. The API endpoint could look like this:

    POST /wallet/:id/bump/:txid?amount=amt

    id (string) wallet id txid (string) hash of original RBF transaction amt (int) number of satoshis to INCREASE fee RATE

  5. TX.getJSON() should return a new bool rbf, which should be displayed in wallet history / listtransactions, etc. commands

Other notes:

This is basically how Electrum Wallet works.

Bitcoin Core is always default RBF in the GUI, with an opt-out option: https://github.com/bitcoin/bitcoin/pull/11605

Creating an RBF transaction ONLY means setting at least one input's sequence to 0xfffffffd. Actually replacing a TX is more complicated.

braydonf commented 5 years ago

I am assuming the appeal of list item 1 is that "opt-in RBF" can be assumed to be supported by the node from the wallet, and thus no additional complexity.

pinheadmz commented 5 years ago

Yes, but it's also accepted relay policy on 99% of network nodes. Unless they are accepting 0-conf transactions as final, I'm not sure why any user would reject RBF from the mempool.

braydonf commented 5 years ago

Also worth looking into is CPFP, as it's another way to "bump the fee", even without opt-in RBF, assuming that another transaction is necessary anyways. I think prioritizing unconfirmed coins during selection would take care of this automatically, such that the next transaction had a higher fee.

pinheadmz commented 5 years ago

That's a good idea as well, although personally I've had issues with wallets (BRD, on iPhone) spending unconfirmed change outputs during the 2017 "fee event" leaving me with a long chain of unconfirmed transactions 😬 . Will have to look into Bitcoin Core CPFP handling to make sure it's a useful strategy.

pinheadmz commented 5 years ago

In addition, I believe the version check here should be removed. Bitcoin Core does not take transaction version into account when determining opt-in RBF. The only thing that matters is the sequence value.

BIP68 defined TX version 2 to indicate that sequence values represented relative locktime. So a transaction with OP_CHECKSEQUENCEVERIFYmay have a sequence < 0xfffffffd for timelock reasons, but that transaction is by definition opt-in replaceable.

I think the reason we have this version check here is so that bcoin, which by default rejects RBF transactions, will not reject CSV transactions from the mempool, wallet, etc.

In accordance with my recommendation to remove the mempool replace-by-fee option and allow all RBF transactions into the mempool, this check should be removed so that it doesn't return false in a way that would deviate from Bitcoin Core.

So, if we all agree, I will update this PR to remove this check as well as the mempool option.

https://github.com/bcoin-org/bcoin/blob/cec3c3e788834d3e9fc3c0b21bc3e340d573dcfd/lib/primitives/tx.js#L966-L977

IMPranshu commented 2 years ago

That's a good idea as well, although personally I've had issues with wallets (BRD, on iPhone) spending unconfirmed change outputs during the 2017 "fee event" leaving me with a long chain of unconfirmed transactions grimacing . Will have to look into Bitcoin Core CPFP handling to make sure it's a useful strategy.

Well, I think both CPFP and RBF do the same thing with a major difference in who is bumping the fees. In the case of CPFP, the receiver is bumping up the fees for the transaction to go through and in the case of RBF, the sender is bumping up the fees.

IMPranshu commented 2 years ago

In addition, I believe the version check here should be removed. Bitcoin Core does not take transaction version into account when determining opt-in RBF. The only thing that matters is the sequence value.

BIP68 defined TX version 2 to indicate that sequence values represented relative locktime. So a transaction with OP_CHECKSEQUENCEVERIFYmay have a sequence < 0xfffffffd for timelock reasons, but that transaction is by definition opt-in replaceable.

I think the reason we have this version check here is so that bcoin, which by default rejects RBF transactions, will not reject CSV transactions from the mempool, wallet, etc.

In accordance with my recommendation to remove the mempool replace-by-fee option and allow all RBF transactions into the mempool, this check should be removed so that it doesn't return false in a way that would deviate from Bitcoin Core.

So, if we all agree, I will update this PR to remove this check as well as the mempool option.

https://github.com/bcoin-org/bcoin/blob/cec3c3e788834d3e9fc3c0b21bc3e340d573dcfd/lib/primitives/tx.js#L966-L977

ACK. So what this will do is:

  1. Even if a transaction's sequence < 0xfffffffd , RBF will be allowed.
pinheadmz commented 2 years ago

Even if a transaction's sequence < 0xfffffffd , RBF will be allowed.

Well what it means is that even TX with version === 1 will be considered opt-in RBF as long as one sequence is < 0xfffffffd. But if we remove that version check we must also just allow all TX into the mempool regardless of sequences or RBF flags.