-
Notifications
You must be signed in to change notification settings - Fork 73
feat: gas manager for loadtest; add tx-gas-chart #755
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a gas limiting mechanism for the loadtest command to control transaction flow through configurable gas budgets and oscillation curves. The feature aims to enable running loadtests indefinitely while managing gas consumption patterns.
Key changes:
- Adds a new
gaslimiterpackage implementing a gas vault system and oscillating gas provider - Refactors loadtest functions to return transaction objects instead of transaction hashes
- Implements gas budget tracking with configurable oscillation curves for dynamic gas limit control
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/loadtest/gaslimiter/curve.go | Defines base curve interface and configuration for gas oscillation patterns |
| cmd/loadtest/gaslimiter/curve_sine.go | Implements sinusoidal curve for oscillating gas budgets |
| cmd/loadtest/gaslimiter/curve_sine_test.go | Tests sine curve implementation with expected values |
| cmd/loadtest/gaslimiter/gas_vault.go | Implements thread-safe gas budget storage with spend/wait mechanism |
| cmd/loadtest/gaslimiter/gas_provider.go | Base gas provider that watches for new block headers |
| cmd/loadtest/gaslimiter/gas_provider_oscillating.go | Oscillating gas provider that adds gas based on curve values per block |
| cmd/loadtest/gaslimiter/README.md | Documentation for gas limiter components and workflow |
| cmd/loadtest/loadtest.go | Integrates gas limiter into main loadtest loop and refactors transaction handling |
| cmd/loadtest/uniswapv3.go | Updates UniswapV3 loadtest to return transaction object instead of hash |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Fix panic caused by signing a nil transaction in loadTestContractCall. The transaction was being built and assigned to `tx`, but the code was incorrectly trying to sign `rtx` which was declared but never assigned. This caused a segmentation fault when using the `contract-call` mode: panic: runtime error: invalid memory address or nil pointer dereference at cmd/loadtest/loadtest.go:1987 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 33 out of 39 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func GetSenderFromTx(ctx context.Context, tx rpctypes.PolyTransaction) (common.Address, error) { | ||
| // Get transaction type | ||
| txType := tx.Type() | ||
|
|
||
| // For non-standard transaction types, we assume the sender is already set | ||
| if txType > 2 { | ||
| return tx.From(), nil | ||
| } | ||
|
|
||
| // Get transaction fields | ||
| chainID := tx.ChainID() | ||
| nonce := tx.Nonce() | ||
| value := tx.Value() | ||
| gas := tx.Gas() | ||
| to := tx.To() | ||
| data := tx.Data() | ||
| v := tx.V() | ||
| r := tx.R() | ||
| s := tx.S() | ||
|
|
||
| // Calculate the signing hash based on transaction type | ||
| var sigHash []byte | ||
| var err error | ||
|
|
||
| switch txType { | ||
| case 0: // Legacy transaction | ||
| sigHash, err = calculateLegacySigningHash(chainID, nonce, tx.GasPrice(), gas, to, value, data) | ||
| case 1: // EIP-2930 (Access List) | ||
| // For now, we can try with empty access list | ||
| // If you need full support, you'll need to add AccessList to PolyTransaction interface | ||
| sigHash, err = calculateEIP2930SigningHash(chainID, nonce, tx.GasPrice(), gas, to, value, data, []interface{}{}) | ||
| case 2: // EIP-1559 | ||
| maxPriorityFee := new(big.Int).SetUint64(tx.MaxPriorityFeePerGas()) | ||
| maxFee := new(big.Int).SetUint64(tx.MaxFeePerGas()) | ||
| sigHash, err = calculateEIP1559SigningHash(chainID, nonce, maxPriorityFee, maxFee, gas, to, value, data) | ||
| default: | ||
| return common.Address{}, fmt.Errorf("unsupported transaction type: %d (0x%x)", txType, txType) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return common.Address{}, fmt.Errorf("failed to calculate signing hash: %w", err) | ||
| } | ||
|
|
||
| // Normalize v value for recovery | ||
| var recoveryID byte | ||
| if txType == 0 { | ||
| // Legacy transaction with EIP-155 | ||
| if chainID > 0 { | ||
| // EIP-155: v = chainId * 2 + 35 + {0,1} | ||
| // Extract recovery id: recoveryID = v - (chainId * 2 + 35) | ||
| vBig := new(big.Int).Set(v) | ||
| vBig.Sub(vBig, big.NewInt(35)) | ||
| vBig.Sub(vBig, new(big.Int).Mul(new(big.Int).SetUint64(chainID), big.NewInt(2))) | ||
| recoveryID = byte(vBig.Uint64()) | ||
| } else { | ||
| // Pre-EIP-155: v is 27 or 28 | ||
| recoveryID = byte(v.Uint64() - 27) | ||
| } | ||
| } else { | ||
| // EIP-2930 and EIP-1559: v is 0 or 1 (or 27/28) | ||
| vVal := v.Uint64() | ||
| if vVal >= 27 { | ||
| recoveryID = byte(vVal - 27) | ||
| } else { | ||
| recoveryID = byte(vVal) | ||
| } | ||
| } | ||
|
|
||
| // Validate recoveryID | ||
| if recoveryID > 1 { | ||
| return common.Address{}, fmt.Errorf("invalid recovery id: %d (v=%s, chainID=%d, type=%d)", recoveryID, v.String(), chainID, txType) | ||
| } | ||
|
|
||
| // Build signature in the [R || S || V] format (65 bytes) | ||
| sig := make([]byte, 65) | ||
| // Use FillBytes to ensure proper padding with leading zeros | ||
| r.FillBytes(sig[0:32]) | ||
| s.FillBytes(sig[32:64]) | ||
| sig[64] = recoveryID | ||
|
|
||
| // Recover public key from signature using go-ethereum's crypto package | ||
| pubKey, err := crypto.Ecrecover(sigHash, sig) | ||
| if err != nil { | ||
| return common.Address{}, fmt.Errorf("failed to recover public key: %w", err) | ||
| } | ||
|
|
||
| // Derive address from public key | ||
| // The public key returned by Ecrecover is 65 bytes: [0x04 || X || Y] | ||
| // We hash the X and Y coordinates (skip first byte) and take last 20 bytes | ||
| hash := crypto.Keccak256(pubKey[1:]) | ||
| address := common.BytesToAddress(hash[12:]) | ||
|
|
||
| return address, nil | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function lacks test coverage for the transaction sender recovery logic. Given the complexity of handling multiple transaction types (Legacy, EIP-2930, EIP-1559) and the critical nature of signature recovery, comprehensive tests should be added to verify correct sender address recovery for all transaction types.
| ```bash | ||
| $ polycli tx-gas-chart --rpc-url http://localhost:8545 | ||
|
|
||
| This will create a file named tx_gasprice_chart.png in the current directory. |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The markdown formatting is broken. The code block starting at line 9 is not properly closed. Line 11 should have triple backticks to close the code block before continuing with the next section heading. The same issue appears to occur throughout this file where bash code examples are not properly formatted.
| ```bash | ||
| # Configure the oscillation wave pattern | ||
| --gas-manager-oscillation-wave string # Wave type: flat, sine, square, triangle, sawtooth (default: "flat") | ||
| --gas-manager-target uint64 # Target gas limit baseline (default: 30000000) | ||
| --gas-manager-period uint64 # Period in blocks for wave oscillation (default: 1) | ||
| --gas-manager-amplitude uint64 # Amplitude of oscillation (default: 0) | ||
|
|
||
| Gas Price Control (Pricing Strategy) | ||
|
|
||
| # Select and configure pricing strategy | ||
| --gas-manager-price-strategy string # Strategy: estimated, fixed, dynamic (default: "estimated") | ||
| --gas-manager-fixed-gas-price-wei uint64 # Fixed price in wei (default: 300000000) | ||
| --gas-manager-dynamic-gas-prices-wei string # Comma-separated prices for dynamic strategy | ||
| # Use 0 for network-suggested price | ||
| # (default: "0,1000000,0,10000000,0,100000000") | ||
| --gas-manager-dynamic-gas-prices-variation float64 # Variation ±percentage for dynamic prices (default: 0.3) | ||
|
|
||
| Examples |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The markdown code block is not properly closed. Line 86 ends the bash code block but line 87 "Gas Price Control (Pricing Strategy)" should be a markdown heading (starting with ###) instead of plain text. The subsequent bash code block starting at line 89 is also not properly opened with triple backticks.
| } | ||
| wg.Wait() | ||
|
|
||
| blocks.avgBlockGasUsed = big.NewInt(0).Div(totalGasUsed, big.NewInt(int64(len(blocks.blocks)))).Uint64() |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using uint64 value for division with big.Int can cause precision loss. When dividing totalGasUsed (a *big.Int) by the number of blocks (converted to int64), you should ensure that the conversion doesn't lose data. Consider checking if len(blocks.blocks) can safely be converted to int64, or use big.Int arithmetic throughout to avoid potential overflow issues.
| blocks.avgBlockGasUsed = big.NewInt(0).Div(totalGasUsed, big.NewInt(int64(len(blocks.blocks)))).Uint64() | |
| if len(blocks.blocks) > 0 { | |
| divisor := new(big.Int).SetUint64(uint64(len(blocks.blocks))) | |
| blocks.avgBlockGasUsed = new(big.Int).Div(totalGasUsed, divisor).Uint64() | |
| } |
|
|
||
| // Validate recoveryID | ||
| if recoveryID > 1 { | ||
| return common.Address{}, fmt.Errorf("invalid recovery id: %d (v=%s, chainID=%d, type=%d)", recoveryID, v.String(), chainID, txType) |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages include chainID in the format string but only when the transaction type is legacy with EIP-155. For other transaction types (EIP-2930, EIP-1559), chainID is still included in the error message even though it may not be relevant to the specific recovery failure. Consider making the error message more specific to the transaction type being processed.
| package gasmanager | ||
|
|
||
| import ( | ||
| "math" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
| ) | ||
|
|
||
| // GasVault manages a budget of gas that can be added to and spent from. | ||
| type GasVault struct { | ||
| mu *sync.Mutex | ||
| gasBudgetAvailable uint64 | ||
| } | ||
|
|
||
| // NewGasVault creates a new GasVault instance. | ||
| func NewGasVault() *GasVault { | ||
| return &GasVault{ | ||
| mu: &sync.Mutex{}, | ||
| gasBudgetAvailable: 0, | ||
| } | ||
| } | ||
|
|
||
| // AddGas adds the specified amount of gas to the vault's available budget. | ||
| func (o *GasVault) AddGas(gas uint64) { | ||
| o.mu.Lock() | ||
| defer o.mu.Unlock() | ||
| if o.gasBudgetAvailable+gas < o.gasBudgetAvailable { | ||
| o.gasBudgetAvailable = math.MaxUint64 | ||
| log.Trace().Uint64("available_budget_after", o.gasBudgetAvailable).Msg("gas budget in vault capped to max uint64") | ||
| } else { | ||
| o.gasBudgetAvailable += gas | ||
| log.Trace().Uint64("available_budget_after", o.gasBudgetAvailable).Msg("new gas budget available in vault") | ||
| } | ||
| } | ||
|
|
||
| // SpendOrWaitAvailableBudget attempts to spend the specified amount of gas from the vault's available budget. | ||
| func (o *GasVault) SpendOrWaitAvailableBudget(gas uint64) { | ||
| const intervalToCheckBudgetAvailability = 100 * time.Millisecond | ||
| for { | ||
| if spent := o.trySpendBudget(gas); spent { | ||
| break | ||
| } | ||
| time.Sleep(intervalToCheckBudgetAvailability) | ||
| } | ||
| } | ||
|
|
||
| // trySpendBudget tries to spend the specified amount of gas from the vault's available budget. | ||
| // It returns true if the gas was successfully spent, or false if there was insufficient budget. | ||
| func (o *GasVault) trySpendBudget(gas uint64) bool { | ||
| o.mu.Lock() | ||
| defer o.mu.Unlock() | ||
| if gas <= o.gasBudgetAvailable { | ||
| o.gasBudgetAvailable -= gas | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // GetAvailableBudget returns the current available gas budget in the vault. | ||
| func (o *GasVault) GetAvailableBudget() uint64 { | ||
| o.mu.Lock() | ||
| defer o.mu.Unlock() | ||
| return o.gasBudgetAvailable | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gas manager components (GasVault, GasProvider, GasPricer, and pricing strategies) lack test coverage. While wave patterns are tested, the core functionality of gas budget management, provider behavior, and pricing strategies should also have comprehensive unit tests to ensure correct behavior under various conditions.
| Visualization with tx-gas-chart | ||
|
|
||
| You can visualize the gas patterns generated by your loadtest using the tx-gas-chart command: | ||
|
|
||
| # Run loadtest with sine wave pattern | ||
| polycli loadtest \ | ||
| --rpc-url http://localhost:8545 \ | ||
| --gas-manager-oscillation-wave sine \ | ||
| --gas-manager-period 100 \ | ||
| --gas-manager-amplitude 10000000 \ | ||
| --target-address 0xYourAddress | ||
|
|
||
| # Generate chart to visualize results | ||
| polycli tx-gas-chart \ | ||
| --rpc-url http://localhost:8545 \ | ||
| --target-address 0xYourAddress \ | ||
| --output sine_wave_result.png | ||
|
|
||
| How It Works |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The markdown formatting is inconsistent. "Visualization with tx-gas-chart" (line 169), "How It Works" (line 187), and other section headings should use markdown heading syntax (e.g., "## Visualization with tx-gas-chart") instead of plain text. Additionally, the bash code examples starting at line 173 are not properly enclosed in triple backtick code blocks.
|
|
||
| variationMin := float64(1) - s.config.Variation | ||
| variationMax := float64(1) + s.config.Variation | ||
| factor := variationMin + rand.Float64()*variationMax |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The factor calculation is incorrect. When calculating random variation, you're computing variationMin + rand.Float64() * variationMax, which adds the maximum variation to the minimum. This should be multiplying by the range, not the max. The correct formula should be: factor := variationMin + rand.Float64() * (variationMax - variationMin). Currently, with a variation of 0.3, you'll get factors ranging from 0.7 to approximately 1.7-2.0 instead of the intended 0.7 to 1.3.
| factor := variationMin + rand.Float64()*variationMax | |
| factor := variationMin + rand.Float64()*(variationMax-variationMin) |
| // loadTestModeERC721, | ||
| // loadTestModeDeploy, | ||
| // loadTestModeIncrement, | ||
| // loadTestModeStore, |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The random mode selection has been commented out, leaving only loadTestModeERC20 and loadTestModeTransaction active. If this is intentional for the gas manager feature, it should be explained in a comment. If this is temporary debugging code, it should be reverted before merging.
| // Ensure gasPrice is >= 1 for logarithmic scale | ||
| gasPrice := t.gasPrice | ||
| if gasPrice <= 0 { | ||
| gasPrice = 1 | ||
| } | ||
|
|
||
| // Use the local gasPrice variable (protected) in all appends |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional check for negative or zero gas price values is performed, but the modified gasPrice variable is reassigned after the check. This means the protection is applied, but then you're working with the protected value. However, there's a subtle issue: when t.gasPrice is 0 and you set gasPrice = 1 for protection, you then append gasPrice (the protected value) to the txGroups, which is correct. But the log scale protection should be mentioned in a comment to explain why gas prices can be modified from their original values.
| // Ensure gasPrice is >= 1 for logarithmic scale | |
| gasPrice := t.gasPrice | |
| if gasPrice <= 0 { | |
| gasPrice = 1 | |
| } | |
| // Use the local gasPrice variable (protected) in all appends | |
| // For plotting on a logarithmic Y scale we must avoid zero/negative values. | |
| // Clamp gasPrice to at least 1 for visualization purposes; this means the | |
| // plotted gas price may differ from the original t.gasPrice when it is <= 0. | |
| gasPrice := t.gasPrice | |
| if gasPrice <= 0 { | |
| gasPrice = 1 | |
| } | |
| // Always use the protected gasPrice value for all plotted points. |
closes https://github.com/0xPolygon/devtools/issues/444
This PR adds a Gas Manager to the loadtest command and introduces a new tx-gas-chart visualization tool.
Gas Manager features:
New tx-gas-chart command:
Use cases: