refactor: replace docker-based inscription builder sidecar with Golang implementation #3082
Conversation
|
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 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the ZetaChain node, including new features for Bitcoin integration, such as support for stateful precompiled contracts, enhancements for multiple Bitcoin chains, and the introduction of a staking precompiled contract. It also includes extensive refactoring to improve code quality, testing enhancements with new end-to-end tests, and various bug fixes. Additionally, the changelog has been updated to document all changes made in version 21.0.0. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
ws4charlie
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3082 +/- ##
===========================================
- Coverage 63.82% 63.77% -0.05%
===========================================
Files 422 421 -1
Lines 29700 29615 -85
===========================================
- Hits 18955 18888 -67
+ Misses 9909 9893 -16
+ Partials 836 834 -2
|
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (16)
e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go (2)
51-51: Add debug logging for transaction details.For better test observability, consider logging the transaction details.
+r.Logger.Debug("Sending inscribed transaction", "amount", amount, "memoSize", len(memoBytes), "feeRate", feeRate) txHash, depositAmount := r.InscribeToTSSFromDeployerWithMemo(amount, memoBytes, int64(feeRate)) +r.Logger.Debug("Transaction sent", "txHash", txHash, "depositAmount", depositAmount)
57-63: Enhance assertion error messages.Add descriptive error messages to help diagnose test failures.
-utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_OutboundMined) +utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_OutboundMined, "CCTX should be mined") depositFeeSats, err := zetabitcoin.GetSatoshis(zetabitcoin.DefaultDepositorFee) -require.NoError(r, err) +require.NoError(r, err, "Failed to calculate deposit fee") receiveAmount := depositAmount - depositFeeSats -utils.MustHaveCalledExampleContract(r, contract, big.NewInt(receiveAmount)) +utils.MustHaveCalledExampleContract(r, contract, big.NewInt(receiveAmount), "Contract should be called with correct amount")zetaclient/chains/bitcoin/observer/witness.go (1)
Test coverage exists but error cases need enhancement
The test file shows good coverage for successful cases but lacks specific tests for error handling scenarios mentioned in the review comment. Current test cases:
- Successful script decoding
- Empty witness handling
- Key spending path handling
However, missing test cases for:
- Invalid hex decoding errors
- Annex tag handling
- Logger error message verification
• Add test case in
TestParseScriptFromWitnessfor hex decoding errors
• Add test case for annex tag handling
• Add assertions to verify logger messages🔗 Analysis chain
Line range hint
97-108: Consider enhancing error handling and documentationThe function's comment suggests this should be handled by the Bitcoin library. Consider:
- Adding a specific TODO with tracking issue
- Enhancing error messages with more context
- Adding unit tests as mentioned in the PR description
Consider updating the function comment and error handling:
-// ParseScriptFromWitness attempts to parse the script from the witness data. Ideally it should be handled by -// bitcoin library, however, it's not found in existing library version. Replace this with actual library implementation -// if libraries are updated. +// ParseScriptFromWitness parses the script from witness data according to BIP341. +// TODO(#ISSUE): Replace with btcd library implementation when available func ParseScriptFromWitness(witness []string, logger zerolog.Logger) []byte { length := len(witness) if length == 0 { + logger.Debug().Msg("empty witness data") return nil } lastElement, err := hex.DecodeString(witness[length-1]) if err != nil { - logger.Debug().Msgf("invalid witness element") + logger.Debug().Err(err).Msg("failed to decode last witness element") return nil }Let's verify the test coverage:
Let me analyze the test file content to verify if it adequately covers the error cases mentioned in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files covering witness parsing rg -l "TestParseScriptFromWitness"Length of output: 85
Script:
#!/bin/bash # Let's examine the test coverage for witness parsing cat zetaclient/chains/bitcoin/observer/witness_test.goLength of output: 11481
e2e/runner/accounting.go (4)
Line range hint
125-129: Consider using structured logging for better observability.The logging statement could be improved by using structured fields for numeric values to ensure consistent parsing and avoid potential formatting issues.
-r.Logger.Info( - "BTC: Balance (%d) >= ZRC20 TotalSupply (%d)", - int64(tssTotalBalance*1e8), - zrc20Supply.Int64()-10000000, -) +r.Logger.Info("BTC balance check passed", + "tss_balance_sats", int64(tssTotalBalance*1e8), + "zrc20_supply_adjusted", zrc20Supply.Int64()-10000000, +)
Line range hint
82-129: Critical: Replace float64 with big.Int for Bitcoin amounts.Using float64 for Bitcoin amounts can lead to precision loss. Bitcoin amounts should always be handled as integers (satoshis) or big.Int to maintain precision.
-tssTotalBalance := float64(0) +tssTotalBalance := big.NewInt(0) for _, tssAddress := range allTssAddress.TssList { btcTssAddress, err := zetacrypto.GetTssAddrBTC(tssAddress.TssPubkey, r.BitcoinParams) if err != nil { continue } utxos, err := r.BtcRPCClient.ListUnspent() if err != nil { continue } for _, utxo := range utxos { if utxo.Address == btcTssAddress { - tssTotalBalance += utxo.Amount + // Convert BTC amount to satoshis and add to total + sats := big.NewInt(int64(utxo.Amount * 1e8)) + tssTotalBalance.Add(tssTotalBalance, sats) } } } -if int64(tssTotalBalance*1e8) < (zrc20Supply.Int64() - 10000000) { +if tssTotalBalance.Cmp(new(big.Int).Sub(zrc20Supply, big.NewInt(10000000))) < 0 { return fmt.Errorf( "BTC: TSS Balance (%d) < ZRC20 TotalSupply (%d)", - int64(tssTotalBalance*1e8), - zrc20Supply.Int64()-10000000, + tssTotalBalance, + new(big.Int).Sub(zrc20Supply, big.NewInt(10000000)), ) }
Line range hint
82-129: Extract magic numbers into named constants.The value 10000000 appears to be a significant constant but its purpose is unclear. Consider extracting it into a named constant for better maintainability.
+const ( + // BTCInitialPoolSupply represents the amount of BTC minted to initialize the pool + BTCInitialPoolSupply = 10000000 +)
Line range hint
82-129: Consider improving error handling consistency.The function silently continues on errors during UTXO fetching. This could mask important issues and make debugging difficult.
for _, tssAddress := range allTssAddress.TssList { btcTssAddress, err := zetacrypto.GetTssAddrBTC(tssAddress.TssPubkey, r.BitcoinParams) if err != nil { - continue + return fmt.Errorf("failed to get BTC TSS address: %w", err) } utxos, err := r.BtcRPCClient.ListUnspent() if err != nil { - continue + return fmt.Errorf("failed to list unspent outputs: %w", err) }zetaclient/chains/bitcoin/tx_script.go (3)
Line range hint
219-229: LGTM! Consider enhancing error messages.The transition to
txscript.MakeScriptTokenizeris a good improvement. However, the error messages could be more descriptive.Consider this enhancement for better error context:
- return nil, false, errors.Wrap(err, "checkInscriptionEnvelope: unable to check the envelope") + return nil, false, errors.Wrap(err, "failed to validate inscription envelope format") - return nil, false, errors.Wrap(err, "decodeInscriptionPayload: unable to decode the payload") + return nil, false, errors.Wrap(err, "failed to extract inscription payload data")
Line range hint
309-337: Consider defining constants for opcode ranges.The implementation correctly handles inscription payload decoding, but readability could be improved by defining constants for the opcode ranges.
Consider adding these constants at the package level:
const ( // Minimum and maximum opcodes for data push operations minDataPushOpcode = txscript.OP_DATA_1 maxDataPushOpcode = txscript.OP_PUSHDATA4 )Then update the validation:
- if next < txscript.OP_DATA_1 || next > txscript.OP_PUSHDATA4 { + if next < minDataPushOpcode || next > maxDataPushOpcode {
Line range hint
338-348: Enhance documentation and error messages.While the validation logic is correct, the function would benefit from better documentation and more descriptive error messages.
Consider these improvements:
-// checkInscriptionEnvelope decodes the envelope for the script monitoring. The format is -// OP_PUSHBYTES_32 <32 bytes> OP_CHECKSIG <Content> +// checkInscriptionEnvelope validates the Bitcoin inscription envelope format. +// Expected format: +// 1. OP_PUSHBYTES_32 <32 bytes of public key> +// 2. OP_CHECKSIG +// Any deviation from this format indicates an invalid inscription envelope. func checkInscriptionEnvelope(t *txscript.ScriptTokenizer) error { if !t.Next() || t.Opcode() != txscript.OP_DATA_32 { - return fmt.Errorf("cannot obtain public key bytes op %d or err %s", t.Opcode(), t.Err()) + return fmt.Errorf("invalid inscription envelope: expected OP_DATA_32 for public key, got opcode %d", t.Opcode()) } if !t.Next() || t.Opcode() != txscript.OP_CHECKSIG { - return fmt.Errorf("cannot parse OP_CHECKSIG, op %d or err %s", t.Opcode(), t.Err()) + return fmt.Errorf("invalid inscription envelope: expected OP_CHECKSIG, got opcode %d", t.Opcode()) } return nil }go.mod (1)
Line range hint
3-5: Align Go version with toolchain version.The Go version (1.22.7) and toolchain version (1.22.8) are mismatched. This could potentially lead to unexpected behavior.
Apply this diff to align the versions:
-go 1.22.7 +go 1.22.8 -toolchain go1.22.8 +toolchain go1.22.8cmd/zetae2e/local/local.go (1)
Line range hint
1-1024: Consider splitting the test configuration into separate files.The file has grown quite large and handles multiple responsibilities. Consider refactoring the test configuration into separate files:
- Test suite definitions (arrays of test cases)
- Test runner initialization
- Network setup logic
- Test execution orchestration
This would improve maintainability and make the codebase more modular.
Create a new package structure:
cmd/zetae2e/ local/ local.go # Main entry point test_suites.go # Test suite definitions runner_setup.go # Runner initialization network_setup.go # Network setup logic test_executor.go # Test execution orchestrationThis structure would make the code more maintainable and easier to test.
changelog.md (1)
41-41: Enhance the changelog entry with more details.The current entry could be more informative about the benefits and impact of this change. Consider expanding it to highlight key improvements.
-* [3082](https://github.com/zeta-chain/node/pull/3082) - replace docker-based bitcoin sidecar inscription build with Golang implementation +* [3082](https://github.com/zeta-chain/node/pull/3082) - replace docker-based bitcoin sidecar inscription build with native Golang implementation, improving reliability and reducing dependenciese2e/runner/bitcoin_inscription.go (3)
65-68: Add parameter validation for robustnessValidating input parameters enhances the reliability of the function and prevents unexpected behaviors.
Include checks to ensure
feeRateandcommitAmountare positive values:if feeRate <= 0 { return nil, fmt.Errorf("feeRate must be positive") } if commitAmount <= 0 { return nil, fmt.Errorf("commitAmount must be positive") }
135-137: Handle potential errors in Taproot output key computationWhile
ComputeTaprootOutputKeycurrently does not return an error, it's prudent to handle future changes where error handling may be introduced.Prepare for potential error returns:
- taprootOutputKey := txscript.ComputeTaprootOutputKey(internalKey.PubKey(), tapScriptRoot[:]) + taprootOutputKey, err := txscript.ComputeTaprootOutputKey(internalKey.PubKey(), tapScriptRoot[:]) + if err != nil { + return errors.Wrap(err, "failed to compute Taproot output key") + }
195-198: Consider unexportingLeafScriptBuilderif not used outside the packageIf
LeafScriptBuilderis intended for internal use only, unexporting it prevents unintended usage and maintains encapsulation.Rename the type to unexport it:
-type LeafScriptBuilder struct { +type leafScriptBuilder struct { script txscript.ScriptBuilder }Also update constructor and usages accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)contrib/localnet/docker-compose.yml(0 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go(1 hunks)e2e/e2etests/test_extract_bitcoin_inscription_memo.go(0 hunks)e2e/runner/accounting.go(1 hunks)e2e/runner/bitcoin.go(1 hunks)e2e/runner/bitcoin_inscription.go(1 hunks)go.mod(1 hunks)zetaclient/chains/bitcoin/observer/witness.go(2 hunks)zetaclient/chains/bitcoin/tokenizer.go(0 hunks)zetaclient/chains/bitcoin/tx_script.go(3 hunks)zetaclient/chains/bitcoin/tx_script_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- contrib/localnet/docker-compose.yml
- e2e/e2etests/test_extract_bitcoin_inscription_memo.go
- zetaclient/chains/bitcoin/tokenizer.go
🧰 Additional context used
📓 Path-based instructions (9)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/accounting.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/bitcoin_inscription.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/witness.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/tx_script.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/tx_script_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (12)
e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go (1)
31-32: Verify contract deployment success.
While the error is checked, we should also verify the contract address is non-zero.
contractAddr, _, contract, err := testcontract.DeployExample(r.ZEVMAuth, r.ZEVMClient)
require.NoError(r, err)
+require.NotEqual(r, contractAddr, common.Address{}, "Contract deployment returned zero address")zetaclient/chains/bitcoin/observer/witness.go (2)
9-9: LGTM: Appropriate use of btcd/txscript package
The addition of the txscript package aligns with the PR objective of leveraging native Bitcoin script handling capabilities.
108-108: LGTM: Improved use of standard constant
The replacement of the magic number 0x50 with txscript.TaprootAnnexTag improves code maintainability and aligns with Bitcoin specifications.
e2e/runner/accounting.go (1)
Line range hint 82-129: Remove or justify #nosec comments.
The function contains multiple #nosec comments suppressing security checks. These should either be removed by fixing the underlying issues or documented with clear justification.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context about the #nosec comments in the codebase.
The #nosec comments in the function are properly justified and can be retained.
The #nosec comments in the file e2e/runner/accounting.go are specifically marked with "test" justification, indicating they are used in test code. The comments suppress the following checks:
- G701: Integer overflow - justified as the values are always in range in test scenarios
- G115: Integer conversions - justified as the values are always in range in test scenarios
These suppressions follow the same pattern as other test files in the codebase where similar justifications are provided. The comments are consistent with the project's security practices where test-specific suppressions are clearly documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all #nosec comments in the codebase to identify patterns
rg "#nosec" --type go
Length of output: 19820
go.mod (1)
Line range hint 391-397: LGTM: Well-documented dependency replacements.
The replace directives for ZetaChain maintained forks are well-documented and use specific commit hashes, which is a good practice for version control.
cmd/zetae2e/local/local.go (2)
313-313: LGTM: New Bitcoin inscription test case added.
The new test case TestBitcoinStdMemoInscribedDepositAndCallName is well-placed within the Bitcoin tests array and follows the established naming convention.
Line range hint 4-5: Verify the status of TODO for file simplification.
The TODO comment references issue #2762 for simplifying this file. Let's verify the status of this task.
e2e/e2etests/e2etests.go (1)
548-556: LGTM! Verify test implementation exists.
The test case definition is well-structured and consistent with other Bitcoin-related tests. The arguments for amount and fee rate are properly defined with appropriate default values.
Let's verify the test implementation exists:
✅ Verification successful
Test implementation verified and properly structured
The test implementation exists in e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go and follows a clear Arrange-Act-Assert pattern with proper error handling and validation steps:
- Arranges test prerequisites (BTC address, mining setup, contract deployment)
- Acts by sending BTC with inscribed memo
- Asserts the cross-chain transaction status and contract state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test implementation exists
# Test: Search for the implementation of TestBitcoinStdMemoInscribedDepositAndCall
# Expect: Function definition should be found
ast-grep --pattern 'func TestBitcoinStdMemoInscribedDepositAndCall($$$)'
Length of output: 5126
changelog.md (1)
Line range hint 1-1000: LGTM! Well-structured changelog.
The changelog follows best practices with clear categorization, PR links, and detailed descriptions. The reverse chronological order and consistent formatting make it easy to track changes across versions.
e2e/runner/bitcoin_inscription.go (3)
187-188: Verify fee calculation units for accuracy
Ensure that feeRate is in satoshis per vByte to accurately calculate the transaction fee. Misalignment in units can lead to incorrect fee estimations.
Please confirm that feeRate is specified in satoshis per virtual byte, and adjust if necessary.
217-223: Validate data chunking logic to avoid off-by-one errors
In the PushData method, verify that the loop correctly processes all data without missing or overlapping bytes, ensuring the entire payload is included.
Review the loop conditions and index calculations to confirm accurate data chunking.
95-101: Confirm correct input index in signature hash calculation
In BuildRevealTxn, ensure the input index used in CalcTapscriptSignaturehash matches the index of the input being signed, especially if the transaction structure changes.
Validate that int(commitTxn.Index) correctly corresponds to revealTx.TxIn[0]. If multiple inputs are introduced later, this needs careful handling.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Golang.text_extract_bitcoin_inscription_memoto make a deposit and call using inscription.tokenizer.gosource file, use the the nativetokenizer.goin upgradedtxscriptpackage.Closes: #2759
Closes: #2900
How Has This Been Tested?
Summary by CodeRabbit
Release Notes for Version 21.0.0
New Features
Bug Fixes
Testing Enhancements