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

feat(btc): support full RBF #150

Merged
merged 2 commits into from
Jun 14, 2024
Merged

feat(btc): support full RBF #150

merged 2 commits into from
Jun 14, 2024

Conversation

ShookLyngs
Copy link
Collaborator

@ShookLyngs ShookLyngs commented Apr 30, 2024

Changes

  • Add sendRbf() and createSendRbfBuilder() APIs to support Full-RBF of BTC transactions
  • Add excludeUtxos, skipInputsValidation options in the sendUtxos() API to support the RBF feature
  • Add onlyProvableUtxos option in the sendRgbppUtxos() API for future update supports
  • Add changeIndex in the return type of the BTC Builder APIs

Related issues

Reference

New APIs

The sendRbf() and createSendRbfBuilder() APIs are design for constructing BTC Full-RBF transactions.

interface SendRbfProps {
  from: string;
  txHex: string;
  source: DataSource;
  feeRate?: number;
  fromPubkey?: string;
  changeIndex?: number;
  changeAddress?: string;
  minUtxoSatoshi?: number;
  onlyConfirmedUtxos?: boolean;
  requireValidOutputsValue?: boolean;
  requireGreaterFeeAndRate?: boolean;

  // EXPERIMENTAL: the below props are unstable and can be altered at any time
  inputsPubkey?: Record<string, string>; // Record<address, pubkey>
}

declare function createSendRbfBuilder(props: SendRbfProps): Promise<{
  builder: TxBuilder;
  fee: number;
  feeRate: number;
  changeIndex: number;
}>;

declare function sendRbf(props: SendRbfProps): Promise<bitcoin.Psbt>;

txHex

The hex of the original transaction.

changeIndex

Optional, default to undefined.

By default, all the inputs and outputs from the original transaction remains the same in the RBF transaction, but if you wish to charge fee from or return change satoshi to the change output of the original transaction, you can specify the option.

requireValidOutputsValue

Optional, default to false.

Require the value of each output in the RBF transaction to be greater or equals to minUtxoSatoshi.

requireGreaterFeeAndRate

Optional, default to false.

Require the fee rate and the fee amount of the RBF transaction to be greater than the original transaction.

inputsPubkey

[EXPERIMENTAL] Optional, default to undefined.

A Map of addresses and public keys. It's useful when the original transaction contains multi-origin P2TR inputs. This option is currently unstable and can be updated, also note that in you won't need it most cases.

New options

skipInputsValidation

[EXPERIMENTAL] Optional, default to undefined in sendUtxos(), default to true in sendRbf().

Previously, when the onlyConfirmedUtxos is true, all inputs will be validated and an error will be thrown if any input is unconfirmed. With the skipInputsValidation option, you can skip validation of the props.inputs, while still apply the onlyConfirmedUtxos rule to the satoshi collector.

Which means if onlyConfirmedUtxos = true and skipInputsValidation = true:

  • UTXOs passed from the props.inputs option can be unconfirmed UTXOs
  • Only confirmed UTXOs will be collected during the pay fee process

In the sendRbf() API, this option is always set to true, allowing transaction chains to be RBF.

excludeUtxos

Optional, default to undefined.

This option is a list of UTXO[] to be excluded from the satoshi collection process when paying fee.

In the sendRbf() API, this option is used to exclude the outputs from the original transaction, preventing errors related to circular references (the RBF may uses outputs of the original transaction as the inputs if they're not excluded).

onlyProvableUtxos

[EXPERIMENTAL] Optional, default to true.

In the sendRgbppUtxos() API, this option is set to true by default, and it checks if each input's address is the same as the from address. If set to false, the address of the inputs can be different from the from address.

New return type

changeIndex

All BTC Builder APIs now return a new property changeIndex, which indicates the specific output in the generated transaction to be the change output. Note that the value of the changeIndex can be -1, meaning the transaction has no change output.

@ShookLyngs ShookLyngs self-assigned this Apr 30, 2024
@Flouse Flouse requested review from duanyytop, ahonn and yuche and removed request for ahonn May 6, 2024 03:56
@duanyytop
Copy link
Collaborator

This draft PR doesn't look ready yet

@ShookLyngs
Copy link
Collaborator Author

This draft PR doesn't look ready yet

Yes, the PR is still in draft, and it's currently for API interface preview only.
Some props can be modified during further tests, but the shape of it is relatively clear now.

@ShookLyngs ShookLyngs changed the title feat(btc): support full RBF [WIP] feat(btc): support full RBF May 8, 2024
@ShookLyngs ShookLyngs force-pushed the feat/full-rbf branch 4 times, most recently from 18dbb20 to 72b8e30 Compare May 16, 2024 12:11
@Dawn-githup
Copy link
Collaborator

Please help review 'full RBF' test analysis.
https://www.notion.so/full-RBF-Test-Case-f198159a01f045748605b9249bc0c649
@ShookLyngs @Flouse @duanyytop

@ShookLyngs ShookLyngs marked this pull request as ready for review May 20, 2024 02:59
@ShookLyngs ShookLyngs changed the title [WIP] feat(btc): support full RBF feat(btc): support full RBF May 20, 2024
@ShookLyngs
Copy link
Collaborator Author

The PR is now ready for review/test, please update your status here @Dawn-githup, thanks.

@ShookLyngs ShookLyngs force-pushed the feat/full-rbf branch 5 times, most recently from 590f0c8 to e6a87e0 Compare May 27, 2024 07:22
@Dawn-githup
Copy link
Collaborator

Dawn-githup commented May 27, 2024

todo list

Normal scenario:
  • No introduction of new input to increase handling fees
  • Add new input and increase handling fee
  • Specify the change output index
Abnormal scenario:
  • Change output cannot be changed
  • Rates are consistent
  • Unable to pay more fees
  • Verify that there are no redundant inputs
  • Verify replacement transaction does not have paymaster
  • Retry on failure
Boundary value scenario:
  • Minimum rate and minimum amount
  • Maximum rate and maximum amount
  • Add feeRate bounds test

@Flouse Flouse added the P-Low label May 29, 2024
@ShookLyngs
Copy link
Collaborator Author

ShookLyngs commented May 30, 2024

If we have an unconfirmed transaction chain:

  • TX_A: inputs=[O1], outputs=[O2], status=unconfirmed
  • TX_B: inputs=[O2], outputs=[O3], status=unconfirmed

And if we wish to construct an RBF transaction of the TX_A, let's call it TX_AR. When constructing the TX_AR, we should exclude O2 and O3 from being included in the inputs, preventing circular reference error since our goal is to replace TX_A and deduct its children (TX_B) from the blockchain. Kinda like the grandfather paradox.

Here's the question: how do we know that O3 should be excluded?

To address it, we may need to use this kind of API to help identify which transaction should be excluded in the RBF: https://mempool.space/docs/api/rest#get-transaction-outspends

The basic logic:

  1. Given a transaction, where txid=A
  2. Search the outspent status of A, and if any results found, record the txid of the results
  3. Get transactions based on the above results, and and then repeat step 2 for all mentioned transactions
  4. In the end, all recorded txid should be excluded when constructing an RBF transaction of A, avoiding circular referencing

What's more, things can get much more complicated if we're constructing an RBF of an RBF of a transaction. Another solution is to expose the excludeUtxos: BaseOutput[] option and leave it to the users. But obviously it's not a user-friendly solution.

On second thought, maybe just set onlyConfirmedUtxos=true and skipInputsValidation=true when construting RBF transactions, allowing the original inputs to be unconfirmed UTXOs, while only collecting confirmed UTXOs for paying fee.

@Flouse
Copy link
Contributor

Flouse commented Jun 2, 2024

txHex

The hex of the original transaction.

changeIndex

Optional, default to undefined.

By default, all the inputs and outputs from the original transaction remains the same in the RBF transaction, but if you wish to charge fee from or return change satoshi to the change output of the original transaction, you can specify the option.

requireValidOutputsValue

Optional, default to false.

Require the value of each output in the RBF transaction to be greater or equals to minUtxoSatoshi.

requireGreaterFeeAndRate

Optional, default to false.

Require the fee rate and the fee amount of the RBF transaction to be greater than the original transaction.

inputsPubkey

[EXPERIMENTAL] Optional, default to undefined.

A Map of addresses and public keys. It's useful when the original transaction contains multi-origin P2TR inputs. This option is currently unstable and can be updated, also note that in you won't need it most cases.

What about adding these descriptions to the README or code comments?

@Flouse
Copy link
Contributor

Flouse commented Jun 2, 2024

New options

skipInputsValidation

Previously, when the onlyConfirmedUtxos is true, all inputs will be validated and an error will be thrown if any input is unconfirmed. With the skipInputsValidation option, you can skip validation of the props.inputs, while still apply the onlyConfirmedUtxos rule to the satoshi collector.

Which means if onlyConfirmedUtxos = true and skipInputsValidation = true:

  • UTXOs passed from the props.inputs option can be unconfirmed UTXOs
  • Only confirmed UTXOs will be collected during the pay fee process

In the sendRbf() API, this option is always set to true, allowing transaction chains to be RBF.

excludeUtxos

This option is a list of UTXO[] to be excluded from the satoshi collection process when paying fee.

In the sendRbf() API, this option is used to exclude the outputs from the original transaction, preventing errors related to circular references (the RBF may uses outputs of the original transaction as the inputs if they're not excluded).

New return type

changeIndex

All BTC Builder APIs now return a new property changeIndex, which indicates the specific output in the generated transaction to be the change output. Note that the value of the changeIndex can be -1, meaning the transaction has no change output.

Same recommendation as above

@ShookLyngs
Copy link
Collaborator Author

What about adding these descriptions to the README or code comments?

I was thinking of maybe creating Markdown docs for each API, like this: packages/btc/docs/send-rbf.md.
Adding comments to the props interface may also work; I'll try it in another PR.

packages/btc/README.md Outdated Show resolved Hide resolved
Flouse
Flouse previously approved these changes Jun 13, 2024
packages/btc/src/api/sendRbf.ts Outdated Show resolved Hide resolved
packages/btc/src/api/sendRbf.ts Outdated Show resolved Hide resolved
Flouse
Flouse previously approved these changes Jun 14, 2024
@Flouse Flouse requested a review from ahonn June 14, 2024 10:20
@ShookLyngs
Copy link
Collaborator Author

Conflicts have been resolved, and the history commits are squashed into 8ce3593.

Additionally, all changes are documented in the changeset: https://github.com/ckb-cell/rgbpp-sdk/blob/08200c974ef336661723cc7556a003932babda9a/.changeset/spicy-cups-cheat.md

@Flouse Flouse disabled auto-merge June 14, 2024 12:08
@Flouse Flouse merged commit 26e7070 into develop Jun 14, 2024
1 of 3 checks passed
@Flouse Flouse deleted the feat/full-rbf branch June 14, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P-Low
Projects
None yet
4 participants