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 173985 -8111
==========================================
- Hits 107510 101282 -6228
+ Misses 64935 63706 -1229
+ Partials 9651 8997 -654
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.
codchen
force-pushed
the
codex/historical-debug-trace-config
branch
from
28c8bbf to
e9f7b71
Compare
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
Much nicer thanks Tony 🙌
Left couple of comments on error handing.
| } | ||
| receipt, err := api.keeper.GetReceipt(api.ctxProvider(LatestCtxHeight), hash) | ||
| if err != nil || receipt == nil { | ||
| return nil |
There was a problem hiding this comment.
- Did we mean to
return errif error is non-nil? - if not, log the error in DEBUG level or something similar?
| } | ||
| block, _, err := api.backend.BlockByHash(ctx, hash) | ||
| if err != nil || block == nil { | ||
| return nil |
3 participants
Summary
max_trace_lookback_blocksconfig for alldebug_trace*lookback checks.-1keeps the existing unlimited-lookback behavior.0means only the latest block is within lookback.>0allows that many blocks behind latest.debug_trace*attempts that exceed the configured lookback with an endpoint/connection-labelled OTel counter.Tests
go test ./evmrpc/configgo test ./evmrpc -run 'Test(IsHistoricalDebugTraceBlock|GuardHistoricalDebugTraceHeight)$'go test ./evmrpc -run 'TestTrace(Transaction|Call|BlockByNumberLookbackLimit|BlockByNumberUnlimitedLookback)$'git diff --check origin/main...HEAD