feat: cherry pick gas estimation API #4271
Conversation
## Overview Adds an ADR for CIP-18 --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit d3dcaba)
rach-id
requested review from
cmwaters and
evan-forbes
and removed request for
a team
📝 WalkthroughWalkthroughThis pull request introduces several significant enhancements to the Celestia App, focusing on transaction size limits, gas estimation, and configuration improvements. The changes span multiple components, including transaction validation, gas estimation services, error handling, and testing infrastructure. Key modifications include implementing a 2 MiB transaction size limit, adding a gas estimation gRPC service, updating configuration options, and refining error handling mechanisms across the application. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GRPCServer
participant App
participant Mempool
participant Blockchain
Client->>GRPCServer: EstimateGasPrice(request)
GRPCServer->>Blockchain: Query last 5 blocks
Blockchain-->>GRPCServer: Transaction data
GRPCServer->>GRPCServer: Calculate gas price
GRPCServer->>Client: Return estimated gas price
Client->>GRPCServer: SubmitTransaction
GRPCServer->>App: CheckTx
App->>App: Validate transaction size
alt Transaction too large
App-->>GRPCServer: Reject transaction
GRPCServer-->>Client: Return error
else Transaction valid
App->>Mempool: Add transaction
Mempool->>Blockchain: Propose transaction
end
Possibly related PRs
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (28)
app/grpc/gasestimation/gas_estimator.go (4)
63-69: Consider returning structured errors for clarity.
Currently, theEstimateGasPricemethod returns raw errors directly (line 64, 66). Returning structured errors or wrapping them with context can be more insightful in diagnostics and debugging.
108-163: Consolidate repeated TxSearch calls.
This logic currently pages through transactions multiple times (lines 140-161). While this approach works for the last five blocks, retrieving transactions in a single call—when supported—could simplify the code and improve performance.
191-223: Improve fee parsing reliability.
Parsing the fee from events (lines 197-221) relies heavily on conventional event/attribute naming. If these fields ever change upstream, this code may silently fail. Consider validating attribute keys more robustly or using structured fee data if available in the SDK.
225-248: Optimize repeated metric calculations.
TheMeanandStandardDeviationfunctions are called multiple times. If the same dataset is used more than once, caching intermediate values (e.g., sum or sum of squares) might be beneficial. This is purely an optimization consideration if performance becomes critical.pkg/user/e2e_test.go (1)
48-50: Consider capturing all errors during concurrent executionThe current implementation with a buffered channel of size 1 only captures the first error. Consider collecting all errors to better understand potential failure patterns in concurrent execution.
- errCh = make(chan error, 1) + var errors []error + errorsMu sync.MutexAnd modify the error handling:
- select { - case errCh <- err: - cancel() - default: - } + errorsMu.Lock() + errors = append(errors, err) + errorsMu.Unlock() + cancel()test/e2e/testnet/txsimNode.go (1)
50-53: Consider using structured logging.The standard logger loses the structured logging benefits that zerolog provided. Consider using a structured logging interface that supports both formats.
-logger.Println("setting image for tx client", "name", name, "image", image) +logger.Printf("setting image for tx client: name=%s image=%s", name, image) -logger.Println("failed to set image for tx client", "name", name, "image", image, "error", err) +logger.Printf("failed to set image for tx client: name=%s image=%s error=%v", name, image, err) -logger.Println("created tx client", "name", name, "image", image, "args", strings.Join(args, " ")) +logger.Printf("created tx client: name=%s image=%s args=%s", name, image, strings.Join(args, " "))Also applies to: 87-87
app/grpc/gasestimation/gas_estimator_test.go (2)
10-46: Consider adding test cases for negative numbers.The test cases for
Meancover empty slice, single element, multiple elements, and mixed floats. Consider adding test cases for negative numbers to ensure the function handles them correctly.{ name: "Mixed floats", gasPrices: []float64{1.5, 2.5, 3.5}, want: 2.5, }, + { + name: "Negative numbers", + gasPrices: []float64{-1.0, -2.0, -3.0}, + want: -2.0, + }, + { + name: "Mixed positive and negative", + gasPrices: []float64{-1.0, 0.0, 1.0}, + want: 0.0, + },
48-87: Consider adding test cases for negative numbers and improve error messages.The test cases for
StandardDeviationare comprehensive but could be improved:
- Add test cases for negative numbers
- Include the tolerance value in error messages for better debugging
{ name: "Identical elements", gasPrices: []float64{5, 5, 5, 5}, want: 0, }, + { + name: "Negative numbers", + gasPrices: []float64{-1.0, -2.0, -3.0}, + want: 1.0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { meanVal := Mean(tt.gasPrices) got := StandardDeviation(meanVal, tt.gasPrices) // We'll do a tolerance check for floating-point comparisons. if math.Abs(got-tt.want) > 1e-9 { - t.Errorf("stdDev(%v) = %v, want %v", tt.gasPrices, got, tt.want) + t.Errorf("stdDev(%v) = %v, want %v (tolerance: %v)", tt.gasPrices, got, tt.want, 1e-9) }app/test/big_blob_test.go (1)
93-126: Consider adding more test cases for size limits.The test verifies the 2 MiB limit but could benefit from additional test cases:
- A blob slightly under the limit
- Multiple blobs that collectively exceed the limit
testCases := []testCase{ + { + name: "1.99 MiB blob (just under limit)", + blob: newBlobWithSize(2087152), + expectedCode: 0, // Success + expectedErr: "", + }, { name: "2 MiB blob", blob: newBlobWithSize(2097152), expectedCode: apperrors.ErrTxExceedsMaxSize.ABCICode(), expectedErr: apperrors.ErrTxExceedsMaxSize.Error(), }, + { + name: "Two 1 MiB blobs (collectively exceed limit)", + blob: []*share.Blob{newBlobWithSize(1048576), newBlobWithSize(1048576)}, + expectedCode: apperrors.ErrTxExceedsMaxSize.ABCICode(), + expectedErr: apperrors.ErrTxExceedsMaxSize.Error(), + }, }app/default_overrides_test.go (1)
68-69: Consider using a named constant for mebibyte.The mebibyte value should be defined as a named constant at package level for better reusability and maintainability.
+const mebibyte = 1048576 + func TestDefaultAppConfig(t *testing.T) { cfg := DefaultAppConfig() - mebibyte := 1048576 assert.Equal(t, 20*mebibyte, cfg.GRPC.MaxRecvMsgSize)test/e2e/benchmark/throughput.go (1)
Line range hint
102-105: Use consistent logging in cleanup functions.The cleanup functions still use
log.Printinstead of the structuredlogger. This should be updated for consistency.Apply this change in both cleanup defer blocks:
defer func() { - log.Print("Cleaning up testnet") + logger.Println("Cleaning up testnet") benchTest.Cleanup(ctx) }()Also applies to: 122-125
cmd/celestia-appd/cmd/root.go (1)
142-146: LGTM! Consider documenting the testing-only nature more prominently.The timeout flag is properly implemented, but since it's only for testing purposes, consider making this more visible.
Add a "TESTING ONLY" prefix to the flag description:
- startCmd.Flags().Duration(TimeoutCommitFlag, 0, "Override the application configured timeout_commit. Note: only for testing purposes.") + startCmd.Flags().Duration(TimeoutCommitFlag, 0, "[TESTING ONLY] Override the application configured timeout_commit.")app/test/gas_estimation_test.go (2)
27-129: Consider improving test reliability and cleanup.While the test is comprehensive, consider these improvements:
- Add test cleanup to ensure resources are properly released
- Consider making the concurrent transaction count configurable
- Add retry mechanism for potential network delays
func TestEstimateGasPrice(t *testing.T) { + t.Cleanup(func() { + // Add cleanup code here + }) + const numAccounts = 150 // test setup: create a test chain, submit a few PFBs to it, keep track of their gas // price, then test the gas estimator API. - accountNames := testfactory.GenerateAccounts(150) // using 150 to have 2 pages of txs + accountNames := testfactory.GenerateAccounts(numAccounts) // using numAccounts to have 2 pages of txs
131-191: Consider adding more test cases for edge scenarios.The test covers basic scenarios well, but consider adding tests for:
- Failed transactions
- Maximum gas limit scenarios
- Complex transactions with multiple messages
Would you like me to help generate additional test cases for these scenarios?
app/default_overrides.go (1)
297-299: Consider moving themebibyteconstant to a shared constants package.The
mebibyteconstant could be useful in other parts of the codebase. Consider moving it to a shared constants package (e.g.,pkg/appconsts) to promote reusability and maintain consistency.app/test/upgrade_test.go (1)
260-260: Document the meaning of the new timeout parameter.The
0value for the timeout commit parameter suggests using a default timeout. Consider adding a comment explaining this, or use a named constant to make the intention clearer.- testApp := app.New(log.NewNopLogger(), db, nil, 0, encCfg, upgradeHeight, 0, util.EmptyAppOptions{}) + // Use default timeout commit + const defaultTimeoutCommit = 0 + testApp := app.New(log.NewNopLogger(), db, nil, 0, encCfg, upgradeHeight, defaultTimeoutCommit, util.EmptyAppOptions{})test/e2e/testnet/node.go (2)
206-212: Improve temporary directory cleanup.The deferred cleanup of the temporary directory is good practice, but consider adding error handling for the cleanup operation.
- defer os.RemoveAll(tmpDir) + defer func() { + if err := os.RemoveAll(tmpDir); err != nil { + n.logger.Printf("Failed to cleanup temporary directory %s: %v", tmpDir, err) + } + }()
66-66: Consider enhancing log messages with additional context.While the logging is functional, consider adding more contextual information to help with debugging.
- n.logger.Println("Pulling round state traces", "address", addr) + n.logger.Printf("Pulling round state traces from node %s at address %s", n.Name, addr) - n.logger.Println("Pulling block summary traces", "address", addr) + n.logger.Printf("Pulling block summary traces from node %s at address %s", n.Name, addr) - n.logger.Println("Creating validator's config and data directories", "name", n.Name, "directory", nodeDir) + n.logger.Printf("Creating validator %s config and data directories at %s", n.Name, nodeDir)Also applies to: 79-79, 212-212
app/errors/errors.go (1)
7-11: LGTM! Consider documenting error code selection.The error registration follows cosmos-sdk conventions with clear messaging. However, it would be helpful to document the rationale behind selecting error code 11142 for future maintainability.
app/extend_block.go (1)
28-34: Consider adding benchmark results for the deprecated notice.The deprecation notice recommends using IsEmptyBlockRef for better performance, but it would be helpful to document the actual performance benefits to justify the change.
proto/celestia/core/v1/gas_estimation/gas_estimator.proto (2)
62-64: Consider adding field validation for tx_bytes.The tx_bytes field in EstimateGasPriceAndUsageRequest should include validation constraints or documentation about size limits and expected format.
message EstimateGasPriceAndUsageRequest { TxPriority tx_priority = 1; - bytes tx_bytes = 2; + bytes tx_bytes = 2 [(cosmos_proto.scalar) = "TxBytes", (cosmos_proto.validate) = "required,tx"]; }
68-71: Consider adding field constraints for estimated values.The response fields should include documentation about their ranges and units (e.g., whether estimated_gas_price is in utia).
docs/architecture/adr-023-gas-used-and-gas-price-estimation.md (4)
43-46: Consider edge cases in gas price estimation.The gas price estimation for different priority levels relies on percentiles from recent blocks. Consider adding fallback mechanisms for scenarios with insufficient transaction history or extreme outliers.
64-73: Add input validation to mean calculation.The
meanfunction should validate that gas prices are non-negative.func mean(gasPrices []float64) float64 { if len(gasPrices) == 0 { return 0 } + for _, gasPrice := range gasPrices { + if gasPrice < 0 { + return 0 + } + } sum := 0.0 for _, gasPrice := range gasPrices { sum += gasPrice } return sum / float64(len(gasPrices)) }
76-87: Add input validation to standardDeviation calculation.The
standardDeviationfunction should validate that gas prices are non-negative.func standardDeviation(gasPrices []float64) float64 { if len(gasPrices) < 2 { return 0 } + for _, gasPrice := range gasPrices { + if gasPrice < 0 { + return 0 + } + } meanGasPrice := mean(gasPrices) var variance float64 for _, gasPrice := range gasPrices { variance += math.Pow(gasPrice-meanGasPrice, 2) } variance /= float64(len(gasPrices)) return math.Sqrt(variance) }
92-92: Fix markdown formatting.Remove trailing whitespace.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
92-92: Trailing spaces
Expected: 0 or 2; Actual: 1(MD009, no-trailing-spaces)
app/test/integration_test.go (1)
278-287: Consider removing deprecated test.This test uses a deprecated method. Consider removing it since
TestIsEmptyBlockRefcovers the same functionality with the new recommended method.docs/release-notes/release-notes.md (1)
57-57: Fix grammatical issues.Please apply the following improvements:
- Add a hyphen to "hard-coded"
- Replace "execution to" with "execution of"
Apply this diff to fix the grammatical issues:
-Once 5/6+ of the voting power have signalled, the upgrade will be ready. There is a hard coded delay between confirmation of the upgrade and execution to the new state machine. +Once 5/6+ of the voting power have signalled, the upgrade will be ready. There is a hard-coded delay between confirmation of the upgrade and execution of the new state machine.🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: This word is normally spelled with a hyphen.
Context: ..., the upgrade will be ready. There is a hard coded delay between confirmation of the upgra...(HARD_CODE_COMPOUND)
[uncategorized] ~57-~57: The preposition ‘of’ seems more likely in this position.
Context: ...nfirmation of the upgrade and execution to the new state machine. To view the upc...(AI_HYDRA_LEO_REPLACE_TO_OF)
🛑 Comments failed to post (3)
test/e2e/testnet/util.go (1)
14-24: 🛠️ Refactor suggestion
Evaluate the switch from structured to unstructured logging.
Replacingzerologwith the standardlog.LoggerinGetGrafanaInfoFromEnvVar(lines 14–24) simplifies logging but loses the structured logging context. If debugging data or structured logs are essential, consider a library that preserves structured fields.test/util/malicious/app.go (1)
59-59: 🛠️ Refactor suggestion
Reconsider hardcoded timeout value.
The timeout value is hardcoded to 0, which might not be appropriate. Consider:
- Using a configurable value from
appOpts- Using the default timeout from the consensus config
-goodApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, 0, 0, appOpts, baseAppOptions...) +defaultTimeout := config.DefaultConsensusConfig().TimeoutCommit +goodApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, 0, defaultTimeout, appOpts, baseAppOptions...)Committable suggestion skipped: line range outside the PR's diff.
test/util/blobfactory/payforblob_factory.go (1)
254-254: 💡 Codebase verification
Restore custom transaction options support
The change to use
DefaultTxOpts()exclusively impacts test scenarios that require specific gas values. Several test files explicitly useFeeTxOpts(1e9)for their test cases:
app/test/circuit_breaker_test.goapp/test/check_tx_test.goConsider keeping the flexibility to pass custom transaction options to support various test scenarios.
🔗 Analysis chain
Verify the impact of removing custom transaction options.
The function now uses
DefaultTxOpts()instead of accepting custom options. While this simplifies the API, it might limit test scenarios that require specific transaction options.Run this script to check for existing usages that might be affected:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to ManyMultiBlobTx that pass custom options rg -A 2 "ManyMultiBlobTx\("Length of output: 1136
Script:
#!/bin/bash # Get the implementation of ManyMultiBlobTx and DefaultTxOpts echo "=== ManyMultiBlobTx implementation ===" ast-grep --pattern 'func ManyMultiBlobTx($$$) { $$$ }' echo -e "\n=== DefaultTxOpts implementation ===" ast-grep --pattern 'func DefaultTxOpts($$$) { $$$ }' echo -e "\n=== Searching for any custom tx options in test files ===" rg "TxOpts" test/ app/test/Length of output: 2954
rach-id
changed the title
## Overview Implements the gas estimation endpoint following: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md Is there an issue for this or I create one? --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit 02f04c9)
rach-id
force-pushed
the
cherry-pick-gas-estimation
branch
from
585c6da to
92dab1e
Compare
Overview
cherry picks to gas estimation API to v3.x