docs: gas used and gas price estimation ADR #4238
Conversation
rach-id
requested review from
rootulp and
evan-forbes
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request introduces ADR 023, which outlines the gas used and gas price estimation for the celestia-app, in accordance with CIP-18. It details the implementation of gas and fee estimation mechanisms, including methods for calculating gas usage through baseapp simulation and a tiered approach for gas price estimation based on transaction priority levels. The document serves as a foundational reference for transaction building within the application. Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (3)
29-34: Enhance the code example with return value usage.The code snippet shows how to obtain
gasInfobut doesn't demonstrate how this information should be used or what specific fields fromgasInfoare relevant for the estimation. Consider adding:
- The expected structure of
gasInfo- Which fields should be used for estimation
- How to handle edge cases (e.g., minimum gas requirements)
40-43: Specify implementation details for gas price calculations.While the priority levels and their calculations are well-defined, the following implementation details should be clarified:
- How to handle blocks with very few transactions (statistical significance)
- Whether the gas prices should be weighted by gas used
- How often these calculations should be updated (every block or cached)
83-83: Remove trailing space.There is a trailing space at the end of this line.
-There are multiple ways to estimate the gas price. However, a first default implementation can be as basic as described above. +There are multiple ways to estimate the gas price. However, a first default implementation can be as basic as described above.🧰 Tools
🪛 Markdownlint (0.37.0)
83-83: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md
83-83: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: docker-security-build / docker-build (GHCR; ghcr.io/celestiaorg/celestia-app)
- GitHub Check: test / test-race
- GitHub Check: test / test-fuzz
- GitHub Check: test / test-coverage
- GitHub Check: test / test
- GitHub Check: markdown-linter / markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (1)
19-19: Clarify the fee estimation mechanism.The fee estimation mechanism is marked as TBD. Since this is a crucial part of the implementation, it would be beneficial to outline at least the high-level approach or options being considered for fee estimation.
Would you like me to help draft potential fee estimation approaches based on common blockchain patterns?
Co-authored-by: Rootul P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (2)
13-13: Improve context section clarity.The suggested improvement from the previous review provides better context by explicitly referencing CIP-18.
Apply this change:
-As per [CIP-18: Standardised Gas and Pricing Estimation Interface](https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md), celestia-app will provide a base implementation for the gas and fee estimation so it can be used when building transactions. +As per [CIP-18: Standardised Gas and Pricing Estimation Interface](https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md), celestia-app will provide a base implementation for the gas and fee estimation so it can be used when building transactions.
89-89: Fix markdown formatting.Remove trailing spaces to comply with markdown formatting guidelines.
🧰 Tools
🪛 Markdownlint (0.37.0)
89-89: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md
89-89: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: docker-security-build / docker-build (GHCR; ghcr.io/celestiaorg/celestia-app)
- GitHub Check: docker-txsim-build / docker-build (GHCR; ghcr.io/celestiaorg/txsim)
- GitHub Check: test / test-short
- GitHub Check: test / test-race
- GitHub Check: test / test-fuzz
- GitHub Check: test / test-coverage
- GitHub Check: test / test
- GitHub Check: markdown-linter / markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (2)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (2)
30-34: Consider gas adjustment multiplier for estimation.The current implementation might underestimate gas usage. Consider incorporating a gas adjustment multiplier (e.g., 1.1) to provide a safety margin, as mentioned in the CLI's
--gas-adjustmentflag.Would you like me to propose an implementation that includes the gas adjustment multiplier?
59-85: Improve numerical stability of statistical calculations.The current implementation might face numerical stability issues with floating-point arithmetic. Consider implementing the previously suggested improvements for better numerical stability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (1)
43-56:⚠️ Potential issueResolve inconsistency in gas price calculation methodology.
The document describes two different approaches:
- Using median of percentiles (lines 43-46)
- Using mean ± standard deviation (lines 53-56)
This creates ambiguity about which method will be implemented. Consider:
- Choosing one consistent approach
- Explaining why that approach was chosen
- Removing references to the unused approach
🧹 Nitpick comments (2)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (2)
63-87: Enhance statistical calculations for better accuracy.The standard deviation implementation could be improved:
- Use sample variance (n-1) instead of population variance (n)
- Consider numerical stability for large numbers
Apply these improvements:
func standardDeviation(gasPrices []float64) float64 { if len(gasPrices) < 2 { return 0 } meanGasPrice := mean(gasPrices) var variance float64 for _, gasPrice := range gasPrices { variance += math.Pow(gasPrice-meanGasPrice, 2) } - variance /= float64(len(gasPrices)) + variance /= float64(len(gasPrices) - 1) // Use n-1 for sample variance return math.Sqrt(variance) }
92-92: Fix markdown formatting.Remove trailing whitespace at the end of line 92.
🧰 Tools
🪛 Markdownlint (0.37.0)
92-92: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md
92-92: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: docker-txsim-build / docker-build (GHCR; ghcr.io/celestiaorg/txsim)
- GitHub Check: docker-security-build / docker-build (GHCR; ghcr.io/celestiaorg/celestia-app)
- GitHub Check: test / test-short
- GitHub Check: test / test-race
- GitHub Check: test / test-fuzz
- GitHub Check: test / test-coverage
- GitHub Check: test / test
- GitHub Check: markdown-linter / markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (2)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (2)
19-19: Clarify the timeline and scope of fee estimation mechanism.The document states that fee estimation is TBD. Consider adding more context about when this will be determined and what factors will influence the decision.
27-37: LGTM! Clear explanation of gas estimation approach.The implementation using baseapp simulation mode is appropriate, and the note about client-side multipliers clearly addresses the previous concerns about gas adjustment.
Co-authored-by: Rootul P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (1)
43-49:⚠️ Potential issueResolve inconsistency in gas price calculation methods.
There's a discrepancy between the two approaches described:
- Lines 43-46 use percentile-based approach (median of top/bottom 10%)
- Lines 53-56 use statistical approach (mean ± standard deviation)
Choose one consistent approach, preferably the percentile-based method, as it's more robust and doesn't assume normal distribution of gas prices.
🧹 Nitpick comments (3)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (3)
19-19: Fix trailing whitespace.Remove the trailing whitespace at the end of line 19.
76-87: Use sample standard deviation for better statistical accuracy.The current implementation uses population standard deviation (n divisor). For sample data, it's more appropriate to use sample standard deviation (n-1 divisor).
Apply this change:
- variance /= float64(len(gasPrices)) + variance /= float64(len(gasPrices) - 1) // Use n-1 for sample standard deviation
102-108: Consider additional consequences.The consequences section could benefit from considering:
- Performance impact of calculating statistics on historical block data
- Memory usage for storing historical gas prices
- Network load from frequent gas estimation requests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md
92-92: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: docker-security-build / docker-build (GHCR; ghcr.io/celestiaorg/celestia-app)
- GitHub Check: docker-txsim-build / docker-build (GHCR; ghcr.io/celestiaorg/txsim)
- GitHub Check: test / test-race
- GitHub Check: test / test-fuzz
- GitHub Check: test / test-coverage
- GitHub Check: test / test
- GitHub Check: test / test-short
- GitHub Check: markdown-linter / markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (2)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (2)
27-37: Well-documented gas estimation approach!The implementation using baseapp simulation mode is clear, and the note about client-side gas multipliers effectively addresses previous concerns about potential underestimation.
46-46: Remove redundant priority level.The "None Priority" is equivalent to "Medium Priority", making it redundant. Consider removing this priority level to simplify the API.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (3)
30-34: Consider additional error cases in simulation.The current error handling is basic. Consider handling specific error cases such as:
- Out of gas errors
- Invalid transaction format
- State-dependent failures
This would help clients better understand why their transaction simulation failed.
gasInfo, _, err := app.BaseApp.Simulate(tx) if err != nil { + switch { + case errors.Is(err, sdkerrors.ErrOutOfGas): + return nil, fmt.Errorf("out of gas error: %w", err) + case errors.Is(err, sdkerrors.ErrTxDecode): + return nil, fmt.Errorf("invalid transaction format: %w", err) + default: + return nil, fmt.Errorf("simulation failed: %w", err) + } - return nil, err }
92-92: Fix trailing whitespace.Remove the trailing whitespace at the end of line 92.
🧰 Tools
🪛 Markdownlint (0.37.0)
92-92: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
102-108: Consider additional consequences.The consequences section could be more comprehensive. Consider adding:
Negative consequences:
- Additional computational overhead for nodes
- Potential for gaming the estimation system
Neutral consequences:
- Need for periodic review and adjustment of estimation parameters
- Impact on node operators who need to maintain historical gas price data
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md
92-92: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: docker-security-build / docker-build (GHCR; ghcr.io/celestiaorg/celestia-app)
- GitHub Check: docker-txsim-build / docker-build (GHCR; ghcr.io/celestiaorg/txsim)
- GitHub Check: test / test-short
- GitHub Check: test / test-race
- GitHub Check: test / test-fuzz
- GitHub Check: test / test-coverage
- GitHub Check: test / test
- GitHub Check: markdown-linter / markdown-link-check
- GitHub Check: Summary
| - High Priority: The gas price is the price at the start of the top 10% of transactions’ gas prices from the last 5 blocks. | ||
| - Medium Priority: The gas price is the median of all gas prices from the last 5 blocks. | ||
| - Low Priority: The gas price is the value at the end of the lowest 10% of gas prices from the last 5 blocks. | ||
| - None Priority: This is equivalent to the Medium priority, using the median of all gas prices from the last 5 blocks. |
There was a problem hiding this comment.
Just something to be wary of: this doesn't consider the fullness of the blocks. For example if there is only one transaction in the last 5 blocks, The new pricing will be in direct relation of what the price of that transaction was. Yet if the blocks continue to be empty, the estimation should be the minimum
There was a problem hiding this comment.
yes, in this case, the returned gas price will be that of that transaction. There are many edge cases we can consider in this and multiple ways to enhance this estimation.
The most important IMO is to return a gas price estimate > than the minimum, only in the case of the last 5 blocks are all > 50% full or some other number.
But I guess we just want to provide an API for now, even if the numbers are skewed, to lay ground for the community to build more accurate estimation algorithms.
If not, we can rework the estimation mechanism.
## Overview Adds an ADR for CIP-18 --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit d3dcaba)
## Overview Adds an ADR for CIP-18 --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit d3dcaba)
Overview
Adds an ADR for CIP-18