[DX-2129] seth hardening #2208
[DX-2129] seth hardening #2208
Conversation
…information, various smaller fixes
… is only one block, fix for fee equalizer (+ tests)
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error messages throughout the Seth library to make them more user-friendly, actionable, and easier to troubleshoot. The main change is replacing the github.com/pkg/errors package with standard Go errors and fmt.Errorf while significantly improving error message clarity.
Key Changes:
- Replaced
github.com/pkg/errorswith standard libraryerrorspackage - Enhanced all error messages with detailed context, troubleshooting steps, and actionable solutions
- Added comprehensive test coverage for gas adjuster functionality
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| seth/util.go | Enhanced error messages for funding calculations and duration validation with detailed troubleshooting steps |
| seth/tracing.go | Improved tracing error messages, added proper error handling for event decoding |
| seth/retry.go | Enhanced gas bumping error messages, fixed gas calculation to avoid overflow issues |
| seth/nonce.go | Improved nonce manager error messages with configuration guidance |
| seth/keyfile.go | Enhanced key management error messages |
| seth/header_cache.go | Improved cache error messages |
| seth/go.mod | Moved pkg/errors to indirect dependency |
| seth/gas_adjuster_test.go | Added comprehensive test coverage for gas adjustment logic |
| seth/gas_adjuster.go | Enhanced gas estimation error messages, improved block fetching logic |
| seth/gas.go | Improved gas estimation error messages |
| seth/decode.go | Enhanced decoding error messages, improved event decoding error tracking |
| seth/contract_store_test.go | Updated tests to handle new error message format |
| seth/contract_store.go | Enhanced contract store error messages |
| seth/config_test.go | Updated tests for new error messages |
| seth/config.go | Improved configuration validation error messages |
| seth/cmd/seth.go | Updated CLI error handling |
| seth/client_*.go | Enhanced client initialization and operation error messages |
| seth/block_stats.go | Improved block stats error messages |
| seth/abi_finder.go | Enhanced ABI finder error messages |
| lib/utils/seth/seth.go | Updated error handling in utils package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| currentGasTip = big.NewInt(int64(baseFee64)) | ||
| } | ||
| } else if baseFeeTipMagnitudeDiff < 3 { | ||
| } else if baseFeeTipMagnitudeDiff < -3 { |
There was a problem hiding this comment.
This condition was changed from < 3 to < -3, which reverses the logic. The comment on line 267 says 'Base fee is MUCH SMALLER than tip', which should be when the difference is negative and large in absolute value. However, the original code at line 265 in the old version had < 3 which was likely incorrect. This fix appears intentional and correct, but verify this aligns with the Fee Equalizer logic tested in gas_adjuster_test.go.
| value, overflow = uint256.FromBig(tx.Value()) | ||
| if overflow { | ||
| return nil, fmt.Errorf("blob transaction value %s overflows uint256", tx.Value().String()) | ||
| } |
There was a problem hiding this comment.
If tx.Value() is nil, the value variable remains nil, which will cause a panic when used in the BlobTx struct at line 278. The BlobTx.Value field is not a pointer type, so nil cannot be assigned. Add a default value when tx.Value() is nil: value = uint256.NewInt(0)
| } | |
| } | |
| } else { | |
| value = uint256.NewInt(0) |
| TxnTimeout: MustMakeDuration(5 * time.Minute), | ||
| DialTimeout: MustMakeDuration(DefaultDialTimeout), | ||
| TransferGasFee: DefaultTransferGasFee, | ||
| GasPriceEstimationEnabled: true, |
There was a problem hiding this comment.
[nitpick] Changed from 200 to 20 blocks for gas price estimation. This is a significant change (10x reduction) that could affect gas price accuracy. While this might be intentional to reduce RPC load, it should be documented in the changeset or commit message why this default was changed, as it could impact users relying on the previous default.
| GasPriceEstimationEnabled: true, | |
| GasPriceEstimationEnabled: true, | |
| // [NOTE] Changed default from 200 to 20 blocks for gas price estimation to reduce RPC load. | |
| // This may impact gas price accuracy, but was deemed an acceptable tradeoff for performance. |
| if ok { | ||
| return cachedHeader, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Changed from blocksNumber / 100 to blocksNumber / 10, making the timeout 10x longer. While this might improve reliability, it's a significant behavioral change. Document why this change was needed - likely related to RPC timeouts when fetching many blocks.
| // The timeout was increased from blocksNumber / 100 to blocksNumber / 10 to improve reliability. | |
| // This change was necessary because fetching many blocks via RPC often resulted in timeouts with the previous value. | |
| // The longer timeout helps prevent RPC failures when retrieving block headers for congestion calculation. |
seth/gas_adjuster.go
Outdated
|
|
||
| var wg sync.WaitGroup | ||
| dataCh := make(chan *types.Header) | ||
| defer close(dataCh) |
There was a problem hiding this comment.
[nitpick] The channel is closed with defer before the goroutine reading from it has finished. The goroutine at line 103-109 ranges over dataCh, and the defer at line 101 will close it after the function returns. However, the wg.Wait() at line 132 ensures all writers finish before the function returns, so closing happens after all sends are complete. This is correct, but the placement is unusual - consider moving the close to after wg.Wait() for clarity.
| } | ||
| }() | ||
|
|
||
| limit := ratelimit.New(4) // 4 concurrent requests |
There was a problem hiding this comment.
[nitpick] Added rate limiting to 4 concurrent requests per second for block header fetching. This is a good improvement to prevent overwhelming RPC nodes, but the magic number '4' should be configurable or at least documented why this specific value was chosen.
| } | ||
|
|
||
| for name, storedABI := range m.ContractStore.ABIs { | ||
| if reflect.DeepEqual(storedABI, *targetABI) { |
There was a problem hiding this comment.
Using reflect.DeepEqual to compare ABIs in a loop (iterating through all stored ABIs) is expensive. Consider using a hash-based lookup or caching ABI names in a map for O(1) lookup instead of O(n) with expensive deep comparisons. This could significantly impact performance when many ABIs are loaded.
| if reflect.DeepEqual(storedABI, *targetABI) { | |
| if storedABI == targetABI { |
1 participant
This pull request introduces improvements to error handling, event decoding, and configuration defaults throughout the Seth client and related modules. The main focus is on providing more detailed diagnostics for ABI and event decoding failures, refining error types, and enhancing the clarity and robustness of gas estimation logic.
Error handling and diagnostics improvements:
ErrNoABIMethod,EventDecodingError,ABIDecodingError), and updated code to wrap and propagate these errors with more context, especially inabi_finder.goand event decoding logic. [1] [2] [3] [4] [5] [6] [7]Gas estimation and congestion logic:
ErrGasEstimation,ErrBlockFetching) and usingerrors.Isfor error checks instead of string matching. Also improved the fallback logic for block counts to avoid edge cases. [1] [2] [3] [4] [5] [6]Configuration and test improvements:
GasPriceEstimationBlocksfrom 200 to 20 for faster and more relevant gas price estimation during client initialization.TestGasAdjuster, ensuring coverage for new functionality.Miscellaneous enhancements:
NewClientRawby wrapping errors with address context.findABINameto map ABI instances to their names for better logging and diagnostics during event decoding.These changes collectively improve the reliability, maintainability, and observability of the Seth client, especially in scenarios involving ABI mismatches, event decoding failures, and gas estimation issues.