feat: support for Bitcoin RBF (Replace-By-Fee) and CPFP (Child-Pays-for-Parent)#3306
feat: support for Bitcoin RBF (Replace-By-Fee) and CPFP (Child-Pays-for-Parent)#3306ws4charlie wants to merge 27 commits intodevelopfrom
Conversation
ws4charlie
added
zetaclient
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces comprehensive support for Bitcoin Replace-By-Fee (RBF) functionality across the ZetaChain node implementation. The changes span multiple packages and modules, focusing on enhancing Bitcoin transaction handling, fee management, and end-to-end testing capabilities. The implementation includes new methods for fee bumping, transaction signing, mempool monitoring, and robust error handling for RBF scenarios. Changes
Sequence DiagramsequenceDiagram
participant Observer
participant Signer
participant RPCClient
participant Mempool
participant ZetaCore
Observer->>Mempool: Monitor Stuck Transaction
Mempool-->>Observer: Identify Stuck Transaction
Observer->>Signer: Request Fee Bump
Signer->>RPCClient: Fetch Current Fee Rates
RPCClient-->>Signer: Return Fee Rates
Signer->>Mempool: Broadcast Replacement Transaction
Mempool-->>ZetaCore: Update Transaction Status
Possibly Related PRs
Suggested Labels
Suggested Reviewers
The implementation demonstrates a robust approach to implementing Replace-By-Fee functionality, with comprehensive test coverage and careful consideration of various edge cases in Bitcoin transaction management. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ws4charlie
changed the title
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
|
Context #3279 (comment) |
ws4charlie
changed the title
…; resolve code rabbit comments
…t-bitcoin-Replace-By-Fee
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito and
swift1337
| ) (math.Uint, math.Uint, error) { | ||
| // zetacore simply update 'GasPriorityFee', and zetaclient will use it to schedule RBF tx | ||
| // there is no priority fee in Bitcoin, the 'GasPriorityFee' is repurposed to store latest fee rate in sat/vB | ||
| cctx.GetCurrentOutboundParam().GasPriorityFee = medianGasPrice.String() |
There was a problem hiding this comment.
Where does the additional fees are taken from? If it's fully inferred by the protocol there might be a insolvency issue
For EVM with use gas stability pool. For Bitcoin we can have a gas stability pool as well, it would be not autoamatically filled like Ethereum, but could still be filled manually by sending BTC to the pool
There was a problem hiding this comment.
Reviewed 60% of the PR. I agree with @lumtis that it have too many changes in too many areas and thus needs to be split into multiple PRs. It also missed some context that can be expressed in .md docs in zetaclient/chains/bitcoin/`.
I would split this into:
- Introducing new client methods, feature doc
- Zetacore changes
- Refactoring signer
- Implementing observer
- E2E tests and E2E framework improvements
Also, imo RBF and CPFP should be separate PRs.
| // wgDeposit is a wait group for deposit runner to finish | ||
| var wgDepositRunner sync.WaitGroup | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
This logic is opaque and brittle. Let's implement a common logic of test dependencies. e.g. "B can be invoked only after A passes"
| // wgDeposit is a wait group for deposit runner to finish | ||
| var wgDepositRunner sync.WaitGroup | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
To solve this, we simply need to:
- Add
E2ETest.Dependencies[]string (e.g.[]string{TestBitcoinWithdrawRestrictedName}) - Update this
RunE2ETests:- It should keep track of "pending tests"
[]stringand "completed tests"map[string]struct{} - If a test has no dependencies, then
run(), otherwise skip and check later in the loop
- It should keep track of "pending tests"
Line 10 in 72dc5f0
NewE2ETest(
name, description string,
argsDefinition []ArgDefinition,
e2eTestFunc E2ETestFunc,
...deps string // NEW
) E2ETestLine 8 in 7c70809
| require.Len(r, args, 2) | ||
|
|
||
| // wait for block mining to stop | ||
| wgDepositRunner.Wait() |
e2e/utils/bitcoin.go
Outdated
| ) | ||
|
|
||
| // MustHaveDroppedTx ensures the given tx has been dropped | ||
| func MustHaveDroppedTx(ctx context.Context, client *rpcclient.Client, txHash *chainhash.Hash) { |
There was a problem hiding this comment.
Not clear that this is (a) related to BTC, (b) related to the mempool
There was a problem hiding this comment.
Let's use the following pattern:
type m struct {
Bitcoin Bitcoin
// for zetacore.go Zetacore Zetacore
}
var Must m
type Bitcoin string
(Bitcoin) MempoolTxDropped(ctx context.Context, client *rpcclient.Client, txHash *chainhash.Hash) {
...
}This trick effectively allows callers to do this:
// do some btc e2e stuff in e2etests/....
utils.Must.Bitcoin.MempoolTxDropped(...)As a result, we will have a clear and easy-to-read testing library.
zetaclient/chains/bitcoin/rpc/rpc.go
Outdated
|
|
||
| memplEntry, err := client.GetMempoolEntry(txHash) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "Transaction not in mempool") { |
There was a problem hiding this comment.
use sentinel errors, don't use strings. The new client also can solve this because it parses error as struct thus you can retrieve error code and cast to smth like ErrNotInMempool
zetaclient/chains/bitcoin/rpc/rpc.go
Outdated
| for { | ||
| memplEntry, err := client.GetMempoolEntry(parentHash) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "Transaction not in mempool") { |
zetaclient/chains/bitcoin/rpc/rpc.go
Outdated
| client interfaces.BTCRPCClient, | ||
| childHash string, | ||
| timeout time.Duration, | ||
| ) (int64, float64, int64, int64, error) { |
There was a problem hiding this comment.
Should return FeesSomething{} struct instead of 4 numbers
| RPCStatusCheckInterval = time.Minute | ||
|
|
||
| // MempoolStuckTxCheckInterval is the interval to check for stuck transactions in the mempool | ||
| MempoolStuckTxCheckInterval = 30 * time.Second |
There was a problem hiding this comment.
Should this be BitcoinMempoolStuckTxDuration ?
| } | ||
|
|
||
| // PkScriptTSS returns the TSS pkScript | ||
| func (signer *Signer) PkScriptTSS() ([]byte, error) { |
There was a problem hiding this comment.
| func (signer *Signer) PkScriptTSS() ([]byte, error) { | |
| func (signer *Signer) TSSToPkScript() ([]byte, error) { |
| ) | ||
|
|
||
| // WatchUTXOs watches bitcoin chain for UTXOs owned by the TSS address | ||
| func (ob *Observer) WatchUTXOs(ctx context.Context) error { |
| nonce uint64, | ||
| consolidateRank uint16, | ||
| test bool, | ||
| ) ([]btcjson.ListUnspentResult, float64, uint16, float64, error) { |
|
replaced by #3396, close this PR |
3 participants
Description
The goals:
The general idea:
RBFflag in every Bitcoin outbound.zetacorefeed latest fee rate (every 10 mins) to pending CCTXs in the newly created methodCheckAndUpdateCctxGasPriceBTC.WatchMempoolTxsinzetaclientto monitor pending outbound txs, so we know how long the txs have been sitting in the Bitcoin mempool.zetaclientwill mark the outbound status asstuckand trigger tx replacement usingRBFandCPFP.feeRateCap = 100zetaclientwill always use most recent fee rate (feed byzetacore) to initiate new outbound transactions. The reason is that using an outdatedGasPricein CCTX struct is usually the root cause (not the Bitcoin network traffic) of stuck transactions, the low-fee problem needs to be solved at the first place to reduce the possibility of stuck txs.Some concepts and parameters:
LastPendingOutbound:Bitcoin outbounds are sequentially chained transactions by nonce. Given
Npending txs[TX1, TX2, TX3, TX4]in the mempool,zetaclientonly need to watch and bump the fee ofTX4in order to clear all of them. According to BitcoinCPFPstrategy, the chained pending txs are treated as a package by miners. BumpingTX4will increase the average fee rate of the whole txs package and make it more attractive to miners.minCPFPFeeBumpPercent:It is set to
20%as an exercise for the initial play. It is designed to balance effectiveness in replacing stuck tx while avoiding excessive sensitivity to fee market fluctuations. For example, given apaidRate == 10, RBF will not happen until the market rate goes up toliveRate==12.feeRateCap:It is the maximum average fee rate for fee bumping.
100 sat/vBis a chosen heuristic based on Bitcoin mempool statistics to avoid excessive (or accidental) fees.reservedRBFFees:It is the amount of BTC reserved in the outbound transaction for fee bumping. It is set to
0.01 BTCby default, which can bmp 10 transactions (1KB each) by100 sat/vB. Most of the time, we have just 1 or 2 stuck transactions in the mempool and the signers will automatically stop signing new transactions by design, so the number0.01 BTCis good enough.Closes: #1695
How Has This Been Tested?
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
New Features
Improvements
Testing
Bug Fixes
Chores