Skip to content
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

Integrate CaptureArbitrumStorageGet/Set into the prestate tracer #2602

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Aug 22, 2024

This PR implements CaptureArbitrumStorage[Get]Set methods for prestateTracer capturing changes that ArbOS makes to storage outside EVM execution, these weren't picked up by the prestateTracer before.

Arbos Storage object calls RecordStorage[Get]Set functions to capture tracing of reads and writes of key-value pairs, meaning the SLOAD/SSTORE opcodes tracing in these functions during scenario TracingDuringEVM has no impact at all as the caller address (scope.Contract) and the key being passed don't associate with each other, instead the key corresponds to types.ArbosStateAddress.

To solve this issue, we currently call CaptureArbitrumStorage[Get]Set regardless of the tracing scenario and call opcode tracing additionally if the scenario is TracingDuringEVM to preserve functionality of other tracers that haven't yet implemented CaptureArbitrumStorage... methods. Notice that SLOAD/SSTORE opcode tracing has no effect on prestate tracer due to the above stated issue and calling CaptureArbitrumStorage... methods first yields correct results.

Pulls in geth- OffchainLabs/go-ethereum#352
Resolves NIT-2733

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Aug 22, 2024
@ganeshvanahalli ganeshvanahalli changed the base branch from master to update-gethpin-v1.14.0 August 22, 2024 14:39
@ganeshvanahalli ganeshvanahalli marked this pull request as ready for review August 22, 2024 14:56
@ganeshvanahalli ganeshvanahalli changed the title Integrate capturearbitrumstoragegetset prestatetracer Integrate CaptureArbitrumStorageGet/Set into the prestate tracer Aug 22, 2024
Base automatically changed from update-gethpin-v1.14.0 to master November 6, 2024 01:09
system_tests/debugapi_test.go Outdated Show resolved Hide resolved
system_tests/debugapi_test.go Outdated Show resolved Hide resolved
system_tests/debugapi_test.go Outdated Show resolved Hide resolved
system_tests/debugapi_test.go Outdated Show resolved Hide resolved
system_tests/debugapi_test.go Outdated Show resolved Hide resolved
diegoximenes
diegoximenes previously approved these changes Nov 19, 2024
system_tests/debugapi_test.go Outdated Show resolved Hide resolved
arbos/util/tracing.go Outdated Show resolved Hide resolved
arbos/util/tracing.go Outdated Show resolved Hide resolved
if !cond {
Fatal(t, msg...)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test should look at things that happen before EVM execution. I think easiest is an arbos upgrade. Might be other possibilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arbos upgrade through ScheduleArbOSUpgrade does make changes to arbitrum storage but the tracing scenario set by default when calling precompiles is tracingDuringEVM, so these changes don't get picked up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the test to check for the storage accesses

@ganeshvanahalli ganeshvanahalli marked this pull request as draft December 17, 2024 22:26
@ganeshvanahalli ganeshvanahalli marked this pull request as ready for review December 18, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants