Feat-11 : Add limit param to stop opcode trace capture after N steps#10173
Conversation
|
@macfarla Kindly take a look at once. |
|
This PR doesn't seem to handle negative limit values if someone passes limit = -1, then stepCount >= limit would be true from step 0 , so no frames would be captured. Better to reject negative values . Also, limit = 0 is treated as "unlimited" based on the options.limit() > 0 check, but there's no test confirming that behavior. Should add one. |
@parthdagia05 Good Catch! will fix. |
83f9f4d to
e8a899b
Compare
|
@sagarkhandagre998 taking a look, however there is a compile error. please ensure you have built locally and run relevant tests before requesting a review. This will massively speed up the feedback loop.
|
Really sorry for that @macfarla ,I had built that locally and tested but had resolved the conflict from github web and something wrong happend . I will fix it to the correct changes . |
|
thats why you got the compilation error , I will fix that . |
8e93982 to
812abc4
Compare
macfarla
left a comment
There was a problem hiding this comment.
couple of comments. I'm not sure about modifying the traceOpcode boolean, there may be a cleaner way
f8f91d4 to
91d3bca
Compare
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
…emainingPostExecution Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
…ostExecution backfill Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Verifies that limit(0) captures all EVM steps without capping, per the spec contract that 0 means unlimited. Without this test a regression flipping the guard from to would pass all other limit tests undetected. Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
…s positive. Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Introduce a dedicated limitReached flag in DebugOperationTracer for step-limit handling. Stop mutating traceOpcode for limit enforcement; keep it only for opcode-filter semantics. Preserve existing behavior: no new frames after limit, while backfill path still runs correctly. Improve readability and reduce surprise/maintenance risk called out in review. Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
9b7afe8 to
b82efa1
Compare
|
@macfarla please take a look. |
macfarla
left a comment
There was a problem hiding this comment.
looking pretty good, can you add a changelog entry and we can run final checks
| @Override | ||
| protected void capturePreExecutionState(final MessageFrame frame) { | ||
| if (options.limit() > 0 && stepCount >= options.limit()) { | ||
| limitReached = true; |
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
fc3a249 to
96843cb
Compare
|
@macfarla We can run the final checks . |
besu-eth#10173) * feat(debug): add limit param to stop trace capture after N steps Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com> --------- Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: abhay-dev2901 <abhaytp1998@gmail.com>
PR description
Add
limitparam to stop opcode trace capture after N stepsPart of aligning Besu's debug RPC endpoints with the execution-apis spec.
What changed:
OpCodeTracerConfigBuilder— newlimit(int)config option (0= unlimited, default)TransactionTraceParams— new optionallimitJSON param wired intotraceOptions()DebugOperationTracer— dedicatedstepCountfield drives the limit check; backfill ofgasRemainingPostExecutionon the last captured frame is preserved even after the limit is hitDebugOperationTracerTest— two new unit tests covering frame count cap and backfill correctnessBehaviour:
limitis absent or0— no change to existing behaviourlimit=N— exactly NStructLogentries are recorded; EVM execution continues to completion per specTwo bugs fixed over the naive implementation:
traceFrames.size()as step counter — replaced with a dedicatedstepCountfield that is explicit and reset correctly per transactiongasRemainingPostExecutionbackfilled — moved the early-return guard intracePostExecutionto after the backfill block so it always runsFixed Issue(s)
Refs #10115
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests