Add witness tracing #8336
Add witness tracing #8336
Conversation
|
maybe it will be nice if we can have a consensus regarding the format of the result with other clients |
| @@ -79,6 +84,7 @@ | |||
| "stack" : [ "0000000000000000000000000000000000000000000000000000000000000020", "0000000000000000000000000000000000000000000000000000000000000000", "0000000000000000000000000000000000000000000000000000000000000020" ], | |||
| "memory" : [ ], | |||
| "storage" : { }, | |||
| "statelessWitness" : [ ], | |||
There was a problem hiding this comment.
is it needed to add it if it's empty ? It seems that we are increasing the size of the response for nothing. It seems to be the same for memory and storage. I remember @ahamlat did some optmisation on tracing but not sure it was related to that
There was a problem hiding this comment.
Yes it removed empties and nulls - it took some time but I eventually found both. Changed in next PR
There was a problem hiding this comment.
What do you mean "Changed in next PR" ? I did a quick check and it seems to be already implemented in this PR.
There was a problem hiding this comment.
Yes, comment doesn't make much sense. I meant next update to the PR.
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
lu-pinto
force-pushed
the
add-witness-tracing
branch
2 times, most recently
from
f92e3fd to
45e26d8
Compare
Good idea. I think we can do that later and just add it to the conversation. I feel this will block this PR which is a good improvement for the next devnet. It's a new concept anyway, doesn't need to be perfect right now |
lu-pinto
force-pushed
the
add-witness-tracing
branch
from
45e26d8 to
9689171
Compare
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
lu-pinto
force-pushed
the
add-witness-tracing
branch
from
9689171 to
b13b4ad
Compare
| private Optional<List<AccessEvent<?>>> captureStatelessWitness(final MessageFrame frame) { | ||
| List<AccessEvent<?>> leafAccesses = frame.getAccessWitness().getLeafAccesses(); | ||
| if (!options.traceStatelessWitness() || leafAccesses.isEmpty()) { | ||
| return Optional.empty(); |
| } | ||
| }, | ||
| "statusCode": 200 | ||
| } |
There was a problem hiding this comment.
Is this a new test ? What is the goal of this test ?
There was a problem hiding this comment.
This is running with default values. Is there a case for that already?
EDIT: I see there _default.json which is essentially the same. Probably the result of a rename or something after rebasing the changes. I'll remove the _complete.json one
| } | ||
| }, | ||
| "statusCode": 200 | ||
| } |
There was a problem hiding this comment.
It would be great to add a test with
"disableStatelessWitness": false
There was a problem hiding this comment.
So I think that's what _complete.json/_default.json do which just uses default values and witness is enabled by default. But since we currently don't have a verkle blockchain, the result is empty, so it's not printed.
remove duplicate test fixture with default Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
PR description
This PR adds tracing of accesses to the witness with every executed frame. This can also be used along with RPC methods like
trace_*anddebug_*.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