test: E2E test for the Bitcoin RBF transaction#3417
Conversation
…ForRegnetAndTestnet; code simplification
…e fee rate according to Bitcoin network type
…t-bitcoin-RBF-zetaclient-refactor-minimum
|
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 📝 WalkthroughWalkthroughThe changes introduce a new end-to-end test for the Bitcoin Replace-By-Fee (RBF) withdrawal feature, refactor related helper functions for clarity, and add synchronization between deposit and withdrawal test routines. Additional utility functions are provided for Bitcoin transaction state assertions and Zetacore cross-chain transaction queries. The changelog is reorganized to better categorize feature and test entries. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as E2E Test Runner
participant ZEVM as ZEVM
participant Zetacore as Zetacore
participant BitcoinNode as Bitcoin Node
TestRunner->>ZEVM: Initiate BTC withdrawal (RBF test)
ZEVM->>Zetacore: Submit cross-chain withdrawal
Zetacore->>BitcoinNode: Create initial BTC tx (low fee)
Note right of BitcoinNode: Mining paused (tx unconfirmed)
Zetacore->>BitcoinNode: Create RBF tx (higher fee)
Note right of BitcoinNode: Mining resumed
BitcoinNode->>Zetacore: RBF tx mined, original dropped
Zetacore->>TestRunner: Return withdrawal status and tx details
TestRunner->>BitcoinNode: Assert RBF tx mined, original dropped
sequenceDiagram
participant TestSuite as Test Suite
participant DepositRoutine as Deposit Routine
participant WithdrawRoutine as Withdraw Routine (RBF)
TestSuite->>DepositRoutine: Start deposit tests
DepositRoutine-->>TestSuite: Signal completion (WaitGroup)
TestSuite->>WithdrawRoutine: Start withdraw RBF test (after deposit)
WithdrawRoutine->>DepositRoutine: Wait for deposit completion
WithdrawRoutine-->>TestSuite: Complete RBF withdraw test
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
ws4charlie
added
test
…t-bitcoin-RBF-zetaclient-refactor-minimum
…t-bitcoin-RBF-zetaclient-implementation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
cmd/zetae2e/local/bitcoin.go (1)
185-222:⚠️ Potential issueWaitGroup not released on early exit – can deadlock withdraw runner
thisRoutine.Done()is invoked only at the happy-path end of the function.
If an error occurs before reaching that line (e.g. failingGetE2ETestsToRunByNameor a test failure), the deposit wait-group is never decremented.
Since the RBF withdraw test blocks onwgDependency.Wait(), the withdraw routine may hang indefinitely.A simple fix is to
defertheDone()call immediately after incrementing the counter:@@ - var thisRoutine sync.WaitGroup - thisRoutine.Add(1) + var thisRoutine sync.WaitGroup + thisRoutine.Add(1) + return func() (err error) { - r.Logger.Print("🏃 starting bitcoin tests") + defer thisRoutine.Done() + r.Logger.Print("🏃 starting bitcoin tests") @@ - thisRoutine.Done() r.Logger.Print("🍾 bitcoin tests completed in %s", time.Since(startTime).String())
🧹 Nitpick comments (7)
changelog.md (1)
28-33: Duplicate “Tests” subsection causes section confusionThe file now contains two separate
### Testssubsections under the same UNRELEASED header (lines 28 and 51). Having duplicate section headers makes changelog automation brittle and can confuse readers or tooling that parses the file.
Move the new test entry under the existing Tests section (line 51) or merge the two sections.cmd/zetae2e/local/bitcoin.go (2)
62-65: Hard-coded instruction to edit source breaks reproducibilityThe commented guidance to “change the constant
minTxConfirmationsto 1” requires contributors to modify core observer code manually to enable the RBF test.
Embedding such ad-hoc steps:
- increases the chance of them being forgotten,
- makes the test non-deterministic across environments,
- and couples E2E tests to internal implementation details.
Provide a programmatic switch instead (e.g. env-var, test flag or config field) so the observer behaviour can be changed without editing production code.
130-132: Wait-group instance leaked for withdraw routine
createBitcoinTestRoutinereturns a*sync.WaitGroup, but the withdraw caller ignores the second value (_).
If a future caller needs to wait for withdraw completion, the handle is lost.Either capture the pointer or return only the routine when the wait-group is not required.
e2e/e2etests/test_bitcoin_withdraw_rbf.go (2)
36-37: Fix incorrect log formatting.The log statement uses a format string with
%sfor an argument that is already a stringer type (txHash). This is redundant and could cause confusion.- r.Logger.Info("got 1st tracker hash: %s", txHash) + r.Logger.Info("got 1st tracker hash: %s", txHash.String())
48-49: Fix incorrect log formatting.Similar to the previous issue, the log statement uses a format string with
%sfor an argument that is already a stringer type (txHashRBF).- r.Logger.Info("got 2nd tracker hash: %s", txHashRBF) + r.Logger.Info("got 2nd tracker hash: %s", txHashRBF.String())e2e/utils/zetacore.go (1)
233-238: Remove redundant fmt.Sprintf in require.False statement.The use of fmt.Sprintf inside require.False is redundant and less efficient.
require.False( t, time.Since(startTime) > timeout, - fmt.Sprintf("waiting outbound tracker timeout, chainID: %d, nonce: %d", chainID, nonce), + "waiting outbound tracker timeout, chainID: %d, nonce: %d", chainID, nonce, )e2e/e2etests/helpers.go (1)
84-88: Dynamically calculate approval amount based on fees.The approval amount is hardcoded to double the withdrawal amount, which might be insufficient if gas fees exceed the withdrawal amount. Calculate the approval amount dynamically based on the actual withdrawal amount and gas fees.
// approve more to cover withdraw fee tx, err := r.BTCZRC20.Approve( r.ZEVMAuth, r.BTCZRC20Addr, - big.NewInt(amount.Int64()*2), + new(big.Int).Add(amount, new(big.Int).Mul(gasFee, big.NewInt(2))), // amount + 2x gas fee for safety )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changelog.md(1 hunks)cmd/zetae2e/local/bitcoin.go(5 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/helpers.go(3 hunks)e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_rbf.go(1 hunks)e2e/utils/bitcoin.go(1 hunks)e2e/utils/zetacore.go(3 hunks)
💤 Files with no reviewable changes (1)
- e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/bitcoin.goe2e/e2etests/test_bitcoin_withdraw_rbf.goe2e/utils/bitcoin.goe2e/utils/zetacore.goe2e/e2etests/e2etests.goe2e/e2etests/helpers.go
🧬 Code Graph Analysis (1)
cmd/zetae2e/local/bitcoin.go (2)
e2e/runner/runner.go (1)
E2ERunner(80-207)e2e/e2etests/e2etests.go (1)
TestBitcoinWithdrawRBFName(142-142)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-zetanode
- GitHub Check: build-and-test
- GitHub Check: gosec
- GitHub Check: lint
- GitHub Check: analyze (go)
- GitHub Check: build
🔇 Additional comments (5)
e2e/e2etests/e2etests.go (1)
1142-1151: RBF test defined but never scheduled – risk of dead code
TestBitcoinWithdrawRBFis added toAllE2ETests, yet the withdraw-runner’s test lists (seecmd/zetae2e/local/bitcoin.go) keep the name commented-out.
As a result this new test is never executed in CI, even when the Zetacore version satisfies the minimum requirement, which may leave the feature un-tested.Consider one of:
- Uncomment the name in
bitcoinWithdrawTestsAdvancedso it runs automatically; or- Add an explanatory comment here stating that the test is intentionally disabled and how to enable it (e.g. via a flag).
e2e/e2etests/test_bitcoin_withdraw_rbf.go (1)
21-72: Test covers key RBF functionality effectively.This test comprehensively validates the Replace-By-Fee functionality by:
- Simulating a stuck transaction by stopping block mining
- Verifying the original transaction is dropped
- Confirming the RBF transaction with a higher fee is successfully mined
The structure and flow of the test provide excellent coverage of the critical path for Bitcoin RBF processing.
e2e/utils/zetacore.go (1)
34-50: Well-structured utility for CCTX retrieval.This function provides a clean abstraction for retrieving cross-chain transactions by inbound hash, with appropriate error handling and assertion logic.
e2e/utils/bitcoin.go (1)
13-49: Well-structured transaction state verification utilities.The
MustHaveDroppedTxandMustHaveMinedTxfunctions provide clear, thorough verification of Bitcoin transaction states through multiple conditions, making the tests more robust and reliable.e2e/e2etests/helpers.go (1)
68-102: Good extraction of approval and withdrawal logic.The refactoring to extract the
approveAndWithdrawBTCZRC20helper function improves code organization and reusability, making it easier to use this functionality in multiple test scenarios including the new RBF test.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Description
Added new E2E test
TestBitcoinWithdrawRBFto ensure the RBF transaction feature is working.How Has This Been Tested?
Summary by CodeRabbit
New Features
Tests
Documentation
Refactor
Style