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: (BitcoinRBF-Step1) zetacore increast gas price in pending Bitcoin CCTXs and burns additional fees #3377

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

ws4charlie
Copy link
Contributor

@ws4charlie ws4charlie commented Jan 17, 2025

Description

The problem:

In some scenarios, a broadcasted Bitcoin outbound tx can get stuck in the mempool when its CCTX carries an outdated low fee rate in the struct. Bitcoin block time is 10 mins, which is much longer than other chains. When a Bitcoin tx gets stuck, it can sit in the mempool for hours or days depending on traffic.

The change: RBF (Replace-by-Fee)

  • Have zetacore update gas prices for all long-pending Bitcoin CCTXs (older than GasPriceUpdateRetryIntervalBTC = 40 mins).
  • The zetacore increases all pending CCTXs' gas price to latest medianGasPrice. The latest medianGasPrice is the recent market price that will be used by zetaclient to bump the average fee rate of all stuck txs to the market level. Note: we also have a gas price multiplier of 2.0 to tolerate fee market fluctuation.
  • When zetacore increasing CCTXs' gas price, it also set the same medianGasPrice value to field GasPriorityFee as a signal to zetaclient that the CCTX's gas price is increased and RBF is allowed.
  • Gas price in CCTXs are increase only when there is enough gas fees in the gas stability pool. Otherwise, do nothing.
  • When zetaclient detects stuck outbound in mempool, it will use the increased gas price to perform an RBF.

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced gas price management for cross-chain transactions, allowing Zetacore to feed the latest gas prices to pending Bitcoin cross-chain transactions.
    • Improved support for gas price updates across different blockchain networks.
  • Improvements

    • Refined gas price update mechanisms for EVM and Bitcoin chains.
    • Updated naming conventions for cross-chain transaction gas price functions.
    • Added more robust error handling for gas price updates.
    • Introduced a retry interval for gas price adjustments to improve efficiency.
  • Technical Updates

    • Modified Bitcoin address handling in test utilities to ensure consistent string representation.
    • Introduced new methods for chain-specific gas price calculations.

Copy link
Contributor

coderabbitai bot commented Jan 17, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This pull request introduces enhancements to the ZetaChain cross-chain transaction (CCTX) gas price management system, with a primary focus on Bitcoin transaction handling. The changes include updating naming conventions, introducing new methods for gas price updates across different blockchain types, and modifying address handling to ensure consistent string representations. The modifications aim to improve the flexibility and precision of gas price management for cross-chain transactions.

Changes

File Change Summary
changelog.md Added "Features" section referencing PR #3377 for Bitcoin cross-chain gas price updates
testutil/sample/crypto.go Updated Bitcoin address handling, added new script generation function
x/crosschain/keeper/abci.go Refactored gas price update methods, added chain-specific gas price update logic
x/crosschain/module.go Updated method naming for gas price iteration
x/observer/types/crosschain_flags.go Added RetryInterval to gas price increase flags
Multiple test files Converted Bitcoin addresses to string representations

Sequence Diagram

sequenceDiagram
    participant ZetaCore
    participant CCTXKeeper
    participant EVMChain
    participant BTCChain
    
    ZetaCore->>CCTXKeeper: Initiate Gas Price Update
    CCTXKeeper->>CCTXKeeper: Determine Chain Type
    alt EVM Chain
        CCTXKeeper->>EVMChain: Update EVM Gas Price
    else Bitcoin Chain
        CCTXKeeper->>BTCChain: Update Bitcoin Gas Price
    end
    CCTXKeeper-->>ZetaCore: Return Updated Gas Prices
Loading

Possibly related PRs

Suggested Labels

gas-price-update, bitcoin-tx, cross-chain-optimization

Suggested Reviewers

  • fbac
  • skosito
  • lumtis
  • kingpinXD
  • gartnera

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@ws4charlie ws4charlie changed the title feat(Bitcoin-RBF-Step-1):zetacore feeds latest gas price to pending Bitcoin CCTXs feat(BitcoinRBFStep1):zetacore feeds latest gas price to pending Bitcoin CCTXs Jan 17, 2025
@ws4charlie ws4charlie changed the title feat(BitcoinRBFStep1):zetacore feeds latest gas price to pending Bitcoin CCTXs feat: (BitcoinRBF-Step1) zetacore feeds latest gas price to pending Bitcoin CCTXs Jan 17, 2025
@ws4charlie ws4charlie marked this pull request as ready for review January 17, 2025 22:17
@ws4charlie ws4charlie requested a review from a team as a code owner January 17, 2025 22:17
@ws4charlie ws4charlie added zetacore Issues related to ZetaCore chain:bitcoin Bitcoin chain related labels Jan 17, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
x/observer/types/crosschain_flags.go (1)

8-10: LGTM! Consider adding documentation about the rationale.

The switch from block-based to time-based intervals is well-aligned with Bitcoin's characteristics. The 10-minute retry interval matches Bitcoin's average block time.

Consider adding a comment explaining why 10 minutes was chosen as the retry interval, relating it to Bitcoin's average block time.

testutil/sample/crypto.go (1)

106-111: LGTM! Well-structured script generation function.

The new function properly generates P2WPKH scripts with appropriate error handling.

Consider adding a comment explaining that this is specifically for testing purposes and shouldn't be used in production code.

x/crosschain/keeper/abci.go (2)

204-217: Add documentation for Bitcoin fee rate implementation.

The repurposing of GasPriorityFee for Bitcoin fee rate should be documented more clearly. Consider adding a comment explaining that in Bitcoin's context, this field stores the fee rate in sat/vB.

 // CheckAndUpdateCCTXGasPriceBTC updates the fee rate for the given Bitcoin chain CCTX
+// Note: Bitcoin does not have a priority fee concept. The GasPriorityFee field is
+// repurposed to store the current fee rate in satoshis per virtual byte (sat/vB).
 func CheckAndUpdateCCTXGasPriceBTC(

219-231: Consider using constants for supported chains.

The chain support logic is clear, but consider defining constants for supported chain types to make maintenance easier and prevent magic numbers.

+// Supported chain types for gas price updates
+const (
+    ChainTypeEVM     = "evm"
+    ChainTypeBitcoin = "bitcoin"
+    ChainTypeZeta    = "zeta"
+)

 func IsCCTXGasPriceUpdateSupported(chainID int64, additionalChains []zetachains.Chain) bool {
     switch {
     case zetachains.IsZetaChain(chainID, additionalChains):
         return false
     case zetachains.IsEVMChain(chainID, additionalChains):
         return true
     case zetachains.IsBitcoinChain(chainID, additionalChains):
         return true
     default:
         return false
     }
 }
x/crosschain/keeper/abci_test.go (1)

609-672: Add edge cases to Bitcoin gas price tests.

While the basic functionality is well tested, consider adding edge cases such as:

  • Zero fee rate handling
  • Maximum fee rate handling
  • Invalid fee rate handling
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bdfc0b and ef21aa2.

📒 Files selected for processing (8)
  • changelog.md (1 hunks)
  • testutil/sample/crypto.go (2 hunks)
  • x/crosschain/keeper/abci.go (5 hunks)
  • x/crosschain/keeper/abci_test.go (11 hunks)
  • x/crosschain/module.go (1 hunks)
  • x/crosschain/types/cctx_test.go (1 hunks)
  • x/crosschain/types/revert_options_test.go (1 hunks)
  • x/observer/types/crosschain_flags.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
x/observer/types/crosschain_flags.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/module.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/types/revert_options_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/types/cctx_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

testutil/sample/crypto.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/abci_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/abci.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

🔇 Additional comments (8)
x/crosschain/types/revert_options_test.go (1)

52-52: LGTM! Clean adaptation to the new BtcAddressP2WPKH return type.

The test correctly handles the new return type by explicitly converting the address to string format.

x/crosschain/module.go (1)

175-175: LGTM! Consistent naming convention applied.

The method name change follows Go's convention of treating acronyms as single words in camelCase (CCTX → CCTx).

testutil/sample/crypto.go (1)

95-104: LGTM! Enhanced type safety with structured address type.

The return type change from string to *btcutil.AddressWitnessPubKeyHash improves type safety and provides direct access to address functionality.

x/crosschain/keeper/abci.go (2)

22-24: LGTM! Function type renamed following Go conventions.

The renaming of CheckAndUpdateCctxGasPriceFunc to CheckAndUpdateCCTXGasPriceFunc follows Go's standard naming convention for acronyms.


Line range hint 30-102: LGTM! Robust implementation of gas price update logic.

The implementation correctly:

  • Fetches and validates gas price increase flags
  • Iterates through supported chains
  • Handles errors appropriately
  • Emits typed events for gas price increases
x/crosschain/types/cctx_test.go (1)

143-143: LGTM! Improved Bitcoin address handling in tests.

The change correctly uses the string representation of the Bitcoin address, ensuring proper address format handling in tests.

x/crosschain/keeper/abci_test.go (1)

674-729: LGTM! Comprehensive chain support test coverage.

The test cases thoroughly cover all supported and unsupported chains, ensuring proper gas price update behavior.

changelog.md (1)

5-8: LGTM! Clear and concise feature documentation.

The changelog entry clearly describes the new Bitcoin gas price update feature.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 85.14851% with 15 lines in your changes missing coverage. Please review.

Project coverage is 64.48%. Comparing base (9106fe6) to head (588fec2).

Files with missing lines Patch % Lines
x/crosschain/keeper/abci.go 84.21% 11 Missing and 4 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3377      +/-   ##
===========================================
+ Coverage    64.40%   64.48%   +0.08%     
===========================================
  Files          436      436              
  Lines        30379    30444      +65     
===========================================
+ Hits         19566    19633      +67     
+ Misses        9972     9970       -2     
  Partials       841      841              
Files with missing lines Coverage Δ
x/crosschain/types/message_vote_gas_price.go 100.00% <100.00%> (ø)
x/observer/types/crosschain_flags.go 100.00% <ø> (ø)
...r/types/message_update_gas_price_increase_flags.go 100.00% <100.00%> (ø)
x/crosschain/keeper/abci.go 86.39% <84.21%> (+9.11%) ⬆️

testutil/sample/crypto.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci_test.go Show resolved Hide resolved
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problematic as in the old PR.
The increase in the fee should be paid somewhere. We should pay from the gas stability pool.

x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
x/crosschain/keeper/abci.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (9)
x/observer/types/crosschain_flags.go (1)

8-8: Consider documenting the rationale for the 10-minute interval.

The change from block-based to time-based interval aligns well with Bitcoin's block time. Consider adding a comment explaining this relationship to help future maintainers understand the chosen duration.

 	// RetryInterval is the number of blocks to wait before incrementing the gas price again
+	// Set to 10 minutes to align with Bitcoin's average block time
 	RetryInterval: time.Minute * 10,
testutil/sample/crypto.go (1)

94-104: Consider accepting randomness as input for deterministic testing.

The function could benefit from accepting a source of randomness for the private key generation, similar to other functions in this file.

-func BtcAddressP2WPKH(t *testing.T, net *chaincfg.Params) *btcutil.AddressWitnessPubKeyHash {
+func BtcAddressP2WPKH(t *testing.T, net *chaincfg.Params, r *rand.Rand) *btcutil.AddressWitnessPubKeyHash {
-	privateKey, err := btcec.NewPrivateKey()
+	seed := make([]byte, 32)
+	_, err := r.Read(seed)
+	require.NoError(t, err)
+	privateKey, err := btcec.PrivKeyFromBytes(seed)
 	require.NoError(t, err)
x/crosschain/keeper/abci.go (3)

59-69: Add context to error logging.

The error logging could be more informative by including additional context about the operation being performed.

-			ctx.Logger().Info("GasStabilityPool: fetching pending cctx failed",
+			ctx.Logger().Info("GasStabilityPool: failed to fetch pending CCTXs for gas price update",
 				"chainID", chain.ChainId,
 				"err", err.Error(),
 			)
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 64-68: x/crosschain/keeper/abci.go#L64-L68
Added lines #L64 - L68 were not covered by tests


208-221: Enhance documentation for Bitcoin gas price updates.

The function's documentation should clarify that for Bitcoin, GasPriorityFee is repurposed to store the fee rate in sat/vB.

-// CheckAndUpdateCCTXGasPriceBTC updates the fee rate for the given Bitcoin chain CCTX
+// CheckAndUpdateCCTXGasPriceBTC updates the fee rate for the given Bitcoin chain CCTX.
+// Note: For Bitcoin transactions, GasPriorityFee is repurposed to store the fee rate in sat/vB,
+// which zetaclient will use to schedule Replace-By-Fee (RBF) transactions.

223-234: Consider using a map for better performance.

For better performance when checking multiple chain types, consider using a map instead of a switch statement.

+var supportedChainTypes = map[string]bool{
+    "evm": true,
+    "bitcoin": true,
+}
+
 func IsCCTXGasPriceUpdateSupported(chainID int64, additionalChains []chains.Chain) bool {
-    switch {
-    case chains.IsZetaChain(chainID, additionalChains):
+    if chains.IsZetaChain(chainID, additionalChains) {
         return false
-    case chains.IsEVMChain(chainID, additionalChains),
-        chains.IsBitcoinChain(chainID, additionalChains):
-        return true
-    default:
-        return false
     }
+    chainType := chains.GetChainType(chainID, additionalChains)
+    return supportedChainTypes[chainType]
 }
x/crosschain/keeper/abci_test.go (3)

Line range hint 66-82: Consider organizing test cases using table-driven tests.

The test setup could be more maintainable using table-driven tests to verify both default and custom crosschain flags behavior.

testCases := []struct {
    name           string
    flags          *observertypes.CrosschainFlags
    blockHeight    int64
    expectedCount  int
    expectedFlags  observertypes.GasPriceIncreaseFlags
}{
    {
        name:          "default flags",
        flags:         nil,
        blockHeight:   observertypes.DefaultCrosschainFlags().GasPriceIncreaseFlags.EpochLength + 1,
        expectedCount: 0,
        expectedFlags: *observertypes.DefaultCrosschainFlags().GasPriceIncreaseFlags,
    },
    {
        name:          "custom flags",
        flags:         crosschainFlags,
        blockHeight:   observertypes.DefaultCrosschainFlags().GasPriceIncreaseFlags.EpochLength + 1,
        expectedCount: 0,
        expectedFlags: customFlags,
    },
}

88-104: Add documentation to clarify test expectations.

The test verifies gas price updates across multiple chains but lacks clear documentation of the expected behavior. Consider adding comments to explain:

  1. Why specific number of updates are expected (2 ETH + 5 BTC + 5 BSC = 12)
  2. The significance of each chain's update count
// Test expects total of 12 updates:
// - 2 updates for Ethereum (nonce range 10-11)
// - 5 updates for Bitcoin (nonce range 20-24)
// - 5 updates for BSC (nonce range 30-34)
// ZetaChain updates (nonce range 40-45) are skipped as expected

684-779: Consider grouping test cases by chain category.

The test cases for chain support verification could be better organized by grouping similar chains together (e.g., all EVM chains, all Bitcoin variants, etc.).

// Group test cases by chain category
var chainTests = []struct {
    category  string
    testCases []struct {
        name      string
        chainID   int64
        isSupport bool
    }
}{
    {
        category: "EVM Chains",
        testCases: []struct {
            name      string
            chainID   int64
            isSupport bool
        }{
            {"Ethereum", chains.Ethereum.ChainId, true},
            {"Ethereum Sepolia", chains.Sepolia.ChainId, true},
            // ... other EVM chains
        },
    },
    {
        category: "Bitcoin Chains",
        testCases: []struct {
            name      string
            chainID   int64
            isSupport bool
        }{
            {"Bitcoin Mainnet", chains.BitcoinMainnet.ChainId, true},
            {"Bitcoin Testnet", chains.BitcoinTestnet4.ChainId, true},
        },
    },
    // ... other chain categories
}
changelog.md (1)

5-8: Enhance the changelog entry with more details.

The changelog entry for PR #3377 could be more descriptive to help users understand the impact and benefits of the change.

- * [3377](https://github.com/zeta-chain/node/pull/3377) - have zetacore feed latest gas price to pending Bitcoin cctxs
+ * [3377](https://github.com/zeta-chain/node/pull/3377) - Enhance Bitcoin transaction efficiency by having zetacore feed latest gas prices to pending Bitcoin CCTXs
+   - Reduces transaction stuck time in mempool
+   - Updates gas prices every 10 minutes
+   - Reduces BTCOutboundGasPriceMultiplier from 2.0 to 1.2
+   - Improves balance between transaction finality and withdrawal fees
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bdfc0b and 279335b.

📒 Files selected for processing (10)
  • changelog.md (1 hunks)
  • testutil/sample/crypto.go (2 hunks)
  • x/crosschain/keeper/abci.go (7 hunks)
  • x/crosschain/keeper/abci_test.go (12 hunks)
  • x/crosschain/module.go (1 hunks)
  • x/crosschain/types/cctx_test.go (1 hunks)
  • x/crosschain/types/revert_options_test.go (1 hunks)
  • x/observer/types/crosschain_flags.go (1 hunks)
  • zetaclient/chains/bitcoin/observer/event_test.go (3 hunks)
  • zetaclient/chains/bitcoin/observer/inbound_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
x/observer/types/crosschain_flags.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/module.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/types/revert_options_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/types/cctx_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/bitcoin/observer/event_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

testutil/sample/crypto.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/bitcoin/observer/inbound_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/abci_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/abci.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

📓 Learnings (2)
zetaclient/chains/bitcoin/observer/event_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2987
File: pkg/memo/fields_v0_test.go:270-320
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `FieldsV0` struct of the `memo` package, `RevertAddress` may be left empty when `CallOnRevert` is set to true.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:37-38
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.
🪛 GitHub Check: codecov/patch
x/crosschain/keeper/abci.go

[warning] 64-68: x/crosschain/keeper/abci.go#L64-L68
Added lines #L64 - L68 were not covered by tests


[warning] 74-74: x/crosschain/keeper/abci.go#L74
Added line #L74 was not covered by tests


[warning] 86-86: x/crosschain/keeper/abci.go#L86
Added line #L86 was not covered by tests


[warning] 96-99: x/crosschain/keeper/abci.go#L96-L99
Added lines #L96 - L99 were not covered by tests


[warning] 142-143: x/crosschain/keeper/abci.go#L142-L143
Added lines #L142 - L143 were not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: start-e2e-test / e2e
🔇 Additional comments (9)
x/crosschain/types/revert_options_test.go (1)

52-52: LGTM! Explicit string conversion improves clarity.

The explicit conversion to string using .String() makes the address format clear and consistent.

x/crosschain/module.go (1)

175-175: LGTM! Consistent method naming.

The updated method name follows the standard casing convention for the CCTX acronym.

testutil/sample/crypto.go (1)

106-111: LGTM! Well-structured utility function.

The function provides a clean interface for generating P2WPKH scripts with proper error handling.

x/crosschain/keeper/abci.go (2)

22-27: LGTM! Clean function type declaration.

The function type declaration is well-structured and follows Go conventions.


73-75: Consider inverting the nil check to reduce nesting.

Following the feedback from past reviews, let's improve code readability by inverting the nil check.

-			if pendingCctx == nil {
-				continue
-			}
+			if pendingCctx != nil {
+				// Rest of the logic
+			}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 74-74: x/crosschain/keeper/abci.go#L74
Added line #L74 was not covered by tests

x/crosschain/types/cctx_test.go (1)

143-143: LGTM! Consistent address type handling.

The conversion of Bitcoin address to string representation ensures type consistency and prevents potential type mismatches in cross-chain transactions.

zetaclient/chains/bitcoin/observer/event_test.go (2)

35-35: LGTM! Standardized address handling in test event creation.

Converting the Bitcoin address to string representation in createTestBtcEvent ensures consistent type handling across test cases.


252-252: LGTM! Consistent revert address handling.

Converting the Bitcoin address to string in revert options test maintains type consistency and matches production behavior.

zetaclient/chains/bitcoin/observer/inbound_test.go (1)

170-170: LGTM! Standardized address handling in inbound tests.

Converting the Bitcoin address to string representation maintains consistency with the address handling pattern across the codebase.

@ws4charlie
Copy link
Contributor Author

ws4charlie commented Jan 21, 2025

Same problematic as in the old PR. The increase in the fee should be paid somewhere. We should pay from the gas stability pool.

For the draft PR:

In the draft PR, it is the TSS self who pays the additional bump fee. The difficulties I have seen for the GasStabilityPool to pay the bump fee in block begin are:

  1. Say we have pending CCTXS cctx1, cctx2, cctx3, cctx4, cctx5, ..., cctx10 and have only Tx1, Tx2, Tx3 broadcasted stuck in mempool. Only Tx3 is the LastStuckOutbound needing a fee bumping. The zetacore does not know which one is the last stuck tx. What it can do is to bump fees for all the pending CCTXs, and it is far from correct (even less correct than EVM chains)

  2. Determining which tx becomes the LastStuckOutbound is done by actively checking mempool and see how long the tx has been pending in Bitcoin mempool, it involves RPC calls. Determing whether or not a stuck tx is no longer stuck is a similar process. The zetacore is not aware of outbound status.



Below are the considerations during RBF implementation and open to discussions

  1. For future gas fee saving:

Now, ignore the difference in tx replacement mechanism. I believe a long-term goal for Zetachain will be lowering gas fee for cross-chain service. Here is some understanding around the current GasStabilityPool model and some thoughts around gas fee saving.

The GasStabilityPool works in a always-increase-gas-price model in zetacore.

Taking Ethereum as example and assuming a scenario when we've got Tx1, Tx2, Tx3, Tx4,...Tx10 stuck due to outdated gasPrice (low price), later on we get 10 more CCTXs and broadcasted another Tx10, Tx11, Tx12, Tx13,...Tx20 with normal gasPrice. The latter 10 also get stuck because the initial txs can't go through. Now, the GasStabilityPool bumps all 20 pending txs, hoping zetaclient to refresh gas prices; after zetaclient completing re-broadcastings for Tx1, Tx2, Tx3, Tx4,...Tx10 with higher gas price (haven't do it for 10~20 yet), the Ethereum network just take all 20 pending txs because the latter 10 already carry market gas prices. So, the prior burned fee (in zetacore) for Tx10, Tx11, Tx12, Tx13,...Tx20 needs to be compensated through FundGasStabilityPool, which is perfect.

It is reasonable to try increasing gas price in the pending EVM-chain CCTXs if they don't finalize in a period of time (10 mins for now). In most cases, the gas price is probably too low is a good and easy guess. For scenarios like TSS signers are not working well or RPC not working well. From the fee saving perspective, the GasStabilityPool dare not try lowering the gas price in pending CCTXs because the external chain tx status is invisible to it.

If lowering gas fee and multiplier are the goals on our roadmap, a always-use-market-gas-price model worths consideration. Like Metamask wallet is doing, it always broadcasts tx with market rate by default and rarely gets stuck. To make it happen, handing over the fee bumping logic from zetacore to an actor (most likely zetaclient) who knows the fee market and effectively sends out the txs is a good direction to move, because zetacore lacks confidence in coordinating the fee bumping in a timely manner.

  1. For future gas fee accounting:

The moment GasStabilityPool updates the GasPrice in original CCTX struct, the CCTX losts track of the original fees paid on withdraw because the parameter changed in the formula: fees = gasPrice * gasLimit + protocolFlatFee. Also, the new gasPrice doesn't really reflect the actual price (it will be higher after bumping) used for external transaction.

To accurately track outbound txs fees, we could:

  • Add one additional field Fees in CCTX outbound, and have zetaclient to provide the fee amount in MsgVoteOutbound.
  • GasStabilityPool stops bumping fees and updating GasPrice in the zetacore, leaving the params intact in the formula: fees = gasPrice * gasLimit + protocolFlatFee

@ws4charlie ws4charlie requested a review from lumtis January 21, 2025 22:41
@lumtis
Copy link
Member

lumtis commented Jan 22, 2025

For the draft PR:
In the draft PR, it was the TSS self pays the additional bump fee. The difficulties I have seen for the GasStabilityPool to pay the bump fee in block begin are:
Say we have pending CCTXS cctx1, cctx2, cctx3, cctx4, cctx5, ..., cctx10 and have only Tx1, Tx2, Tx3 broadcasted stuck in mempool. Only Tx3 is the LastStuckOutbound needing a fee bumping. The zetacore does not know which one is the last stuck tx. What it can do is to bump fees for all the pending CCTXs, and it is far from correct (even less correct than EVM chains)
Determining which tx becomes the LastStuckOutbound is done by actively checking mempool and see how long the tx has been pending in Bitcoin mempool, it involves RPC calls. Determing whether or not a stuck tx is no longer stuck is a similar process. The zetacore is not aware of outbound status.

I don't understand, in the PR we already have the logic iterating the pending CCTX and increasing the fee in the CCTX, what I'm asking is to pay this increase from the gas stability pool, I don't think this adds up to the problematic you mention based on the current implementation. TSS should never infer the cost for a withdraw.

On a second hand I don't think it's problematic increasing all cctx with outdated fee, not paying the fees is problematic.

@ws4charlie
Copy link
Contributor Author

ws4charlie commented Jan 27, 2025

@kingpinXD

What I see is that the current implementation funds (EffectiveGasLimit - EffectiveGasUsed) * EffectiveGasPrice back to the GasStabilityPool. If we want it to track a number of surplus (or deficit), we need

  1. Add (EffectiveGasUsed * EffectiveGasPrice) - (OriginalGasLimitByUser * OriginalGasPriceByUser), it can be either a positive or negative number.
  2. The balance ofGasStabilityPool can be either positive (surplus) or negative (deficit).
  3. The current accounting above is incorrect.

Accounting is still a separate thing, it just visualizes the system and provides some hints. If the balance becomes negative, then we could increase the charge (by some multiplier) from users; if the balance grows, we could consider lower the charge (by same multiplier).

If the balance becomes negative, it doesn't mean we should stop bumping fees, as long as the current gas price isn't suspiciously high. So what we could control are 3 parameters:

  1. gasPriceMultiplierChargeUser = 1.2, this collects surplus from users on withdraw.
  2. gasPriceMultiplierSendOutbound = 1.1, this helps finality (slightly > market fee) and also makes profit (< 1.2)
  3. gasPriceCap = some configurable number, this protect against accidental high gas price.
if gasLimit > 0 && gasUsed > 0 && !gasPrice.IsZero() {
	if gasLimit > gasUsed {
		remainingGas := gasLimit - gasUsed
		remainingFees := math.NewUint(remainingGas).Mul(gasPrice).BigInt()

		// We fund the stability pool with a portion of the remaining fees.
		remainingFees = percentOf(remainingFees, RemainingFeesToStabilityPoolPercent)

		// Fund the gas stability pool.
		if err := k.fungibleKeeper.FundGasStabilityPool(ctx, chainID, remainingFees); err != nil {
			return err
                 }
                 ...
	}
}

Here is the follow-up after discussion with @kingpinXD

  1. It's confirmed that the GasStabilityPool is NOT an account that tracks surplus or deficit.
  2. We overpay gas fee for the pending CCTXs in some cases, e.g. TSS not signing (or signing slow).

@lumtis
Copy link
Member

lumtis commented Jan 28, 2025

Again the gas stability pool mechanism is up to be optimized. The overall vision of the mechanism is to have an automated way, that doesn't require supervision, to unblock stuck cctx. On EVM if a nonce is long pending, we make the assumption that it is stuck because of gas price as the blockchain doesn't have access to the external world, we also iterate next cctxs for time gain but this is a part to be optimized as well, tracked by an issue.

I think the fact we iterate multiple CCTX even the non-stuck ones for Bitcoin for now is a detail, we can easily subsidized the cost.

My original question where I don't think I had an answer yet is who pay for the fee bump?

The TSS actually doesn't infer any cost for the withdraw in RBF tx, it uses the updated fee rate (the GasPriorityFee field) to bump the RBF tx.

This doesn't answer the question, where does the fund come from to update the fee rate? If the fee rate is increase, the cost is higher for the outbound, who pay for this difference of cost?

@ws4charlie
Copy link
Contributor Author

ws4charlie commented Jan 29, 2025

I think the fact we iterate multiple CCTX even the non-stuck ones for Bitcoin for now is a detail

EVM chain has same problem as long as the bumped pending CCTX don't haven an outbound sent out yet (due to any type of delay, e.g. RPC error, TSS keysign delay).

This doesn't answer the question, where does the fund come from to update the fee rate? If the fee rate is increase, the cost is higher for the outbound, who pay for this difference of cost?

In the RBF implementation, the users (who have ever withdrawn funds) will pays for it and Zetachain is going to make profit under the current multiplier 2.0. We never make profit since mainnet launch even if we keep charging EVM chain gas fee at multiplier 1.2 and Bitcoin chain at 2.0.

Zetachain charge withdraw fee at multiplier 2.0 and pays Bitcoin network gas fee at multiplier 1.5 here: https://github.com/zeta-chain/node/blob/feat-bitcoin-RBF-zetaclient-implementation/zetaclient/chains/bitcoin/signer/outbound_data.go#L83

@lumtis
Copy link
Member

lumtis commented Jan 29, 2025

In the RBF implementation, the users (who have ever withdrawn funds) will pays for it and Zetachain is going to make profit under the current multiplier 2.0. We never make profit since mainnet launch even if we keep charging EVM chain gas fee at multiplier 1.2 and Bitcoin chain at 2.0.
Zetachain charge withdraw fee at multiplier 2.0 and pays Bitcoin network gas fee at multiplier 1.5 here: https://github.com/zeta-chain/node/blob/feat-bitcoin-RBF-zetaclient-implementation/zetaclient/chains/bitcoin/signer/outbound_data.go#L83

Ok, so the user pay 33% surplus on the initial outbound fee. What if the increase for RBF is more than this 33% surplus? Is it a impossible scenario?

@lumtis
Copy link
Member

lumtis commented Jan 29, 2025

And why did we decided to have this 33% surplus in the first place instead of the user paying only the actual fee?

@ws4charlie
Copy link
Contributor Author

ws4charlie commented Jan 29, 2025

Ok, so the user pay 33% surplus on the initial outbound fee. What if the increase for RBF is more than this 33% surplus? Is it a impossible scenario?

User pays 2.0 * gasPrice * gasLimit, we use 1.5 * gasPrice * gasLimit, so we make 25% profit from user as an insurance. These two parameters are configurable so we can either increase profit, or lower it to allow users enjoying lower fee.

Yes, it is possible for individual CCTX. The model isn't focusing on profit of one single CCTX, it it's a insurance.

And why did we decided to have this 33% surplus in the first place instead of the user paying only the actual fee?

Where is the percentage 33% coming from?, maybe I misunderstood.

Paying actual fee means we charge on multiplier 1.0 and TSS send tx on same multiplier 1.0, the ideal case is a revenue-expenditure balance in a statistical sense. Leaving a gap between the two numbers ensures that we always make profit moving forward.

@lumtis
Copy link
Member

lumtis commented Jan 30, 2025

Where is the percentage 33% coming from?, maybe I misunderstood.

If user pays 2.0x, and TSS use 1.5x, this is a 33% surplus

User pays 2.0 * gasPrice * gasLimit, we use 1.5 * gasPrice * gasLimit, so we make 25% profit from user as an insurance. These two parameters are configurable so we can either increase profit, or lower it to allow users enjoying lower fee.

You can't make assumption that this covers the funds to increase the CCTX, if a CCTX fee is increased, this increase needs to be covered as ZRC20 at the protocol level, there is no way around.

@ws4charlie
Copy link
Contributor Author

Here is the new commit to burn additional fees from gas stability pool: a509d88, @lumtis could you pls check if this looks better?

@ws4charlie ws4charlie changed the title feat: (BitcoinRBF-Step1) zetacore feeds latest gas price to pending Bitcoin CCTXs feat: (BitcoinRBF-Step1) zetacore increast gas price in pending Bitcoin CCTXs and burns additional fees Jan 31, 2025
@ws4charlie ws4charlie marked this pull request as draft January 31, 2025 17:16
@ws4charlie ws4charlie marked this pull request as ready for review January 31, 2025 22:14
@@ -19,6 +19,10 @@ message GasPriceIncreaseFlags {
// Maximum number of pending crosschain transactions to check for gas price
// increase
uint32 maxPendingCctxs = 5;

// Retry interval for Bitcoin chain pending cctx gas price increase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using a different vallue compared to other chain? block time?

Comment on lines +274 to +282
func IsCCTXGasPriceUpdateSupported(chainID int64, additionalChains []chains.Chain) bool {
switch {
case chains.IsZetaChain(chainID, additionalChains):
return false
case chains.IsEVMChain(chainID, additionalChains),
chains.IsBitcoinChain(chainID, additionalChains):
return true
default:
return false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func IsCCTXGasPriceUpdateSupported(chainID int64, additionalChains []chains.Chain) bool {
switch {
case chains.IsZetaChain(chainID, additionalChains):
return false
case chains.IsEVMChain(chainID, additionalChains),
chains.IsBitcoinChain(chainID, additionalChains):
return true
default:
return false
func IsCCTXGasPriceUpdateSupported(chainID int64, additionalChains []chains.Chain) bool {
switch {
case chains.IsEVMChain(chainID, additionalChains),
chains.IsBitcoinChain(chainID, additionalChains):
return true
default:
return false


// withdraw additional fees from the gas stability pool
chainID := cctx.GetCurrentOutboundParam().ReceiverChainId
gasLimit := math.NewUint(cctx.GetCurrentOutboundParam().CallOptions.GasLimit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this paramter with Bitcoin, don't we?

// The function returns the number of cctxs updated and the gas price increase flags used
func (k Keeper) IterateAndUpdateCctxGasPrice(
func (k Keeper) IterateAndUpdateCCTXGasPrice(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing this function makes be think this can be broken down into several functions , around validation , fetch cctx's and processGas price updates. We shouldn't do that change in this pr, but wanted to get your thoughts
If we agree, it would be a good idea to create an issue.

@@ -102,7 +107,7 @@ IterateChains:

// CheckAndUpdateCctxGasPrice checks if the retry interval is reached and updates the gas price if so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// CheckAndUpdateCctxGasPrice checks if the retry interval is reached and updates the gas price if so
// CheckAndUpdateCCTXGasPrice checks if the retry interval is reached and updates the gas price if so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli breaking:proto chain:bitcoin Bitcoin chain related SIM_TESTS zetacore Issues related to ZetaCore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants