evmrpc: gate historical debug trace calls#3515
Conversation
PR SummaryMedium Risk Overview Guards resolve the block from tx hash, block number/hash, or Not gated: Reviewed by Cursor Bugbot for commit 28c8bbf. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3515 +/- ##
==========================================
- Coverage 59.04% 58.21% -0.83%
==========================================
Files 2199 2129 -70
Lines 182096 174001 -8095
==========================================
- Hits 107510 101295 -6215
+ Misses 64935 63708 -1227
+ Partials 9651 8998 -653
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
Thinking out loud: Do we want to apply the limit to sei_traceBlockByHashExcludeTraceFail and sei_traceBlockByNumberExcludeTraceFail too? I would just to have consistent behaviour even though I think we disabled those endpoints by default. The edge case I am thinking of is incase someone has them enabled. If we do decide to do this then I would drop the debug_ from config names and call it something like max_trace_lookback with docs explaining where it applies etc. etc.
|
|
||
| // HistoricalDebugTraceBlockAge defines how many blocks behind latest a debug_trace* | ||
| // target can be before it is considered historical. Set to -1 to disable classification. | ||
| HistoricalDebugTraceBlockAge int64 `mapstructure:"historical_debug_trace_block_age"` |
There was a problem hiding this comment.
Instead of "Age" would it be clearer to use the term "lookback"?
Because the former usually caries a notion of time, whereas the config here is simply a counter of relative offset from latest height.
|
|
||
| // DisableHistoricalDebugTrace rejects debug_trace* calls whose target block is older than | ||
| // HistoricalDebugTraceBlockAge blocks behind the latest block. | ||
| DisableHistoricalDebugTrace bool `mapstructure:"disable_historical_debug_trace"` |
There was a problem hiding this comment.
When we use the term "historical" do we refer to an explicit system component with the same name? I think not. I think that "historical" is the lingo we used internally to talk about this subject; right?
If that's right, then my suggestion is to remove the term "historical" entirely from config and variable names. What we really mean here is a lookback limit. We can then get rid of the dual flag of one boolean and one int "lookback amount", in favour of a single config, named something like max_debug_trace_lookback. Zero means ignored. Positive value means limit is applied to whatever is configured.
That simplifies things operationally speaking.
codchen
force-pushed
the
codex/historical-debug-trace-config
branch
from
22a4c8f to
28c8bbf
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 28c8bbf. Configure here.
| return false | ||
| } | ||
| return latestHeight-blockHeight > historicalBlockAge | ||
| } |
There was a problem hiding this comment.
Guards perform unnecessary lookups when feature is disabled
Medium Severity
The guardHistoricalDebugTraceByTxHash, guardHistoricalDebugTraceByNumber, guardHistoricalDebugTraceByHash, and guardHistoricalDebugTraceByNumberOrHash functions unconditionally perform expensive lookups (GetReceipt, BlockByHash, tmClient.Genesis()) before reaching guardHistoricalDebugTraceHeight, which is the first place disableHistoricalDebugTraceBlockAge is checked. Since the default config is 0 (feature disabled), every debug_trace* call incurs unnecessary store reads or RPCs. Worse, resolveDebugTraceBlockNumber with EarliestBlockNumber calls Genesis(), and if that RPC fails, the error propagates even though the guard is a no-op — a regression for debug_traceBlockByNumber("earliest"). Each intermediate guard function needs an early if api.disableHistoricalDebugTraceBlockAge <= 0 { return nil } check.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 28c8bbf. Configure here.
2 participants
Summary
disable_historical_debug_trace_block_age.0leaves historicaldebug_trace*calls enabled.>0disablesdebug_trace*calls whose target block is older than that many blocks behind latest.debug_trace*attempts with an endpoint/connection-labelled OTel counter when the configured age threshold is active.debug_trace*calls once the configured block-age threshold is exceeded.Tests
go test ./evmrpc/configgo test ./evmrpc -run 'Test(IsHistoricalDebugTraceBlock|GuardHistoricalDebugTraceHeight)$'go test ./evmrpc -run 'TestTrace(Transaction|Call|BlockByNumberLookbackLimit|BlockByNumberUnlimitedLookback)$'git diff --check