-
Notifications
You must be signed in to change notification settings - Fork 446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add native prestate tracer #6907
Conversation
/// <param name="isPostMerge"></param> | ||
/// <remarks>Depends on <see cref="IsTracingInstructions"/></remarks> | ||
void StartOperation(int depth, long gas, Instruction opcode, int pc, bool isPostMerge = false); | ||
void StartOperation(int depth, long gas, Instruction opcode, int pc, Address executingAccount, bool isPostMerge = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent the past day and a half investigating deriving the executingAccount
in the tracer's ReportAction
/SetOperationStorage
methods without needing to modify the ITxTracer
API, however the approach started really blowing up in scope and had a bunch of edge cases that I was having difficulty debugging. Since this PR is meant to optimize performance for the tracers, I felt that having the executing account passed direclty from the virtual machine made the most sense after struggling to derive the executing account in the tracer itself.
If any reviewer feels strongly about not passing through the executing account and instead deriving it in the tracer, would you be able to pair with me on debugging? I feel pretty blocked moving forward with this update.
For some additional information about my investigation, I calculated the initial executing account with
private Address GetInitialExecutingAccount(Address from, Address? to, bool isContractCreation, bool isSystem)
{
UInt256 nonce = isContractCreation ? _worldState!.GetNonce(from) : 0;
return to ?? (isSystem
? from
: ContractAddress.From(from, nonce > 0 ? nonce - 1 : nonce));
}
Then, for each modifying operation the executing account was recalculated. Here's a gist link with my attempted changes as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's do it differently, pass ExecutionEnvironment
into ReportAction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added a commit to pass through the execution environment as a param in StartOperation
as discussed in our daily meeting
@@ -85,7 +85,7 @@ public class UserOperationTxTracer : TxTracer | |||
|
|||
public override void StartOperation(in ExecutionEnvironment env, long gas, Instruction opcode, int pc) | |||
{ | |||
int depth = env.CallDepth; | |||
int depth = env.CallDepth + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for reviewers - I kept going back and forth on leaving the depth
and isPostMerge
params in StartOperation
, even though they can now be derived from env
. Please let me know if you think I should add them back to the method signature instead of deriving them when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets keep it simple, if they are not needed, they are not needed.
Why do we need to do +1 here though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll derive them when needed then. Prior to my updates for the StartOperation params in this PR, the call depth was increased by one before being passed through to the tracers and I initially forgot to add that logic in with my updates.
_txTracer.StartOperation(vmState.Env.CallDepth + 1, gasAvailable, instruction, programCounter, vmState.Env.TxExecutionContext.BlockExecutionContext.Header.IsPostMerge);
Updates NativePrestateTracer to serialize uint256 dictionary keys/vals directly Co-authored-by: LukaszRozmej <LukaszRozmej@users.noreply.github.com> Co-authored-by: OlegJakushkin <OlegJakushkin@users.noreply.github.com>
Co-authored-by: LukaszRozmej <LukaszRozmej@users.noreply.github.com> Co-authored-by: OlegJakushkin <OlegJakushkin@users.noreply.github.com>
* Updates ITxTracer.StartOperation to have the entire execution environment as a param * Derive the pre-existing depth and isPostMerge params from the execution env
5fe3a93
to
7539103
Compare
@@ -85,7 +85,7 @@ public class UserOperationTxTracer : TxTracer | |||
|
|||
public override void StartOperation(in ExecutionEnvironment env, long gas, Instruction opcode, int pc) | |||
{ | |||
int depth = env.CallDepth; | |||
int depth = env.CallDepth + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets keep it simple, if they are not needed, they are not needed.
Why do we need to do +1 here though?
namespace Nethermind.Evm.Test.Tracing; | ||
|
||
[TestFixture] | ||
public class GethLike4byteTracerTests : GethLikeNativeTracerTestsBase | ||
{ | ||
[TestCaseSource(nameof(FourByteTracerTests))] | ||
public Dictionary<string, int>? four_byte_tracer_executes_correctly(byte[] code, byte[]? input) => | ||
(Dictionary<string, int>)ExecuteAndTrace(Native4ByteTracer.FourByteTracer, code, input).CustomTracerResult?.Value; | ||
(Dictionary<string, int>)ExecuteAndTrace(Native4ByteTracer.FourByteTracer, code, null, null, input).CustomTracerResult?.Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those nulls
don't look great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'll update this!
src/Nethermind/Nethermind.Evm.Test/Tracing/GethLikeNativeTracerFactoryTests.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/JavaScript/GethLikeJavaScriptTxTracer.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/GethLikeNativeTxTracer.cs
Outdated
Show resolved
Hide resolved
public Address From; | ||
public Address? To; | ||
public readonly Address? Beneficiary = beneficiary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't those be inferred from TxExecutionContext
and BlockExecutionContext
accesible from ExecutionEnvironment
? If not maybe add them there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, they can be inferred through TxExecutionContext
if I add them as properties in that struct and add them while building the execution environment. I'll then need to add the execution environment as a param for the ReportAction
method so that I can pull off the transaction from
address, to
address, and beneficiary
address on the first call. I think it would be worthwhile exposing the execution environment for ReportAction as well in case we want to add any new native tracers in the future that could take advantage of that so I'll go ahead and make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the spike for this approach on a commit in a separate branch since I realized that having this logic in the ReportAction
method would be kind of hacky since the nonce for the transaction sender is incremented prior to entering ReportAction and would need special handling for the initial case to decrement it by one. I thought about exposing another trace method to the ITxTracer API to mirror the geth OnTxStart
hook, but felt like that might be overkill for this use case since the information can just be passed from the constructor.
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/GethLikeTxTracer.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Serialization.Json/EthereumJsonSerializer.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm.Test/Tracing/GethLikePrestateTracerTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few minor touches
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires explanation in Release Notes
The
ITxTracer.StartOperation
method signature was updated to include the Execution Environment as a parameter and to remove thedepth
andisPostMerge
parameters since those can now be derived from the execution environment. We should relay to users that this method signature was changed in case anyone is relying on it for their own tracer implementations.