Port `MessageTrace` and `VMTracer` machinery from Hardhat #531

Xanewok · 2024-06-24T20:24:08Z

Part of #246

While this is not immediately at the top of the module dependency chain, the logic was pretty self-contained (so easier to port and verify) and it was helpful for me to port the important trace data types as it helped me understand how the information flows and what's used and how.

The biggest change is that this exposes a single observe method that can be used in HH to digest the traces directly on the Rust side, rather than 3 separate methods, without marshalling the data over FFI when fetching the stack trace (and possibly in the future if we remove the minimal node/vm EthereumJS events).

The companion branch is here https://github.com/NomicFoundation/hardhat/tree/refactor/port-vm-tracer; I still need to figure out how to streamline the process and at least prove that the PR works in a backwards-compatible fashion with Hardhat.

It might be helpful to see these files used when porting, when reviewing:

changeset-bot · 2024-06-24T20:24:12Z

⚠️ No Changeset found

Latest commit: de86baa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Xanewok · 2024-06-24T20:28:12Z

crates/edr_napi/src/trace/exit.rs

+pub struct Exit(pub(crate) edr_solidity::exit::ExitCode);
+
+impl FromNapiValue for Exit {
+    unsafe fn from_napi_value(


I wasn't sure whether I should have #[napi] pub enum ExitCode in edr_napi since that's also directly used in the edr_solidity::message_trace module; I figured that it's okay for now to have the plain Rust enum and directly implement the N-API conversion on the JS Exit class (compatible with the existing Hardhat interface).

See my other comment, but I'd prefer using #[napi] pub enum ExitCode here to avoid the need for unsafe code.

Bugs in the N-API layer are particularly hard to debug, so if we can rely on auto-generated code, that's preferred.

I followed this and have a dedicated enum ExitCode on the edr_solidity side with the Hardhat-specific #[napi] pub enum ExitCode in the edr_napi side, which does the relevant conversion.

Wodann

Overall, I'm hesitant about committing these changes to main. Any commit in main will be released in the next NPM package that's cut.

Whereas most code is tested through the EDR and Hardhat test suites, this part of the code additionally requires testing of community plugins. This process is still manual and thus very time consuming.

If the intent of this PR is to be a stepping stone for the stack trace port, I'd propose merging this to a feature branch and continue the work there.

crates/edr_solidity/src/contracts_identifier.rs

Wodann · 2024-06-25T19:44:25Z

crates/edr_solidity/src/contracts_identifier.rs

@@ -19,14 +19,14 @@ pub enum BytecodeType {
 }

 /// Temporary stub
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Clone)]


Suggested change

#[derive(Debug, PartialEq, Clone)]

#[derive(Clone, Debug, PartialEq)]

crates/edr_solidity/src/contracts_identifier.rs

Wodann · 2024-06-25T19:48:37Z

crates/edr_solidity/src/exit.rs

+/// Represents the exit code of the EVM. Naive Rust port of the `ExitCode` from
+/// Hardhat.
+#[derive(Clone, Copy, Debug)]
+pub enum ExitCode {


This is specific to Hardhat, so why not use this is in edr_napi only, while returning the normal EVM's result from edr_solidity?

That way we can also avoid the unsafe code, as we can use #[napi] pub enum ExitCode instead of using a wrapper struct.

See #531 (comment); I split them up and now we do not use unsafe anymore.

crates/edr_solidity/src/message_trace.rs

Wodann · 2024-06-25T19:56:42Z

crates/edr_solidity/src/vm_tracer.rs

+
+impl VmTracer {
+    /// Creates a new [`VmTracer`].
+    pub fn new() -> Self {


I'd inverse these two and make the fn new const, such that it offers a benefit over the Default implementation (i.e. being const). Otherwise, I don't see value in having the fn new.

Sure, done.

Wodann · 2024-06-25T20:00:15Z

crates/edr_solidity/src/vm_tracer.rs

+            // TODO: Make this nicer and make sure the U160 logic/precompile comparison
+            // logic works


Todo. Are you planning to resolve this before merging?

If not, could you please create a GitHub issue for this and link to its URL here?

Verified the comparison but I'm not going to make this nicer for now, as I plan to make it more Rust idiomatic after we migrate everything and we can sever most of the logic from JS idiosyncracies later on

crates/edr_solidity/src/vm_tracer.rs

hardhat-tests/package.json

Wodann · 2024-06-25T20:07:16Z

crates/edr_napi/src/trace/exit.rs

+pub struct Exit(pub(crate) edr_solidity::exit::ExitCode);
+
+impl FromNapiValue for Exit {
+    unsafe fn from_napi_value(


See my other comment, but I'd prefer using #[napi] pub enum ExitCode here to avoid the need for unsafe code.

Bugs in the N-API layer are particularly hard to debug, so if we can rely on auto-generated code, that's preferred.

Xanewok · 2024-07-03T08:24:53Z

I'll move this to draft for now, as it seems to be impossible to port some of the logic without exposing it into our public API via napi-rs.

One approach might be to collect the changes on the Rust side, however there is substantial amount of logic that has to touch the N-API surface due to how JS classes (i.e. codebase model) interact with each other, so we wouldn't get far with this approach.

Another alternative might be to conditionally compile the internal bits for now, however my intuition is that it might require quite a bit of work and setup for it to work correctly with napi-rs (its build system often leaves a bit to be desired). My changes also require modifying Hardhat to use the new API from the EDR package rather than in-tree logic in hardhat-network/stack-traces module; this might need setting up some patching mechanism as well while doing the migration.

I'm continuing work on my branch for now; I'm open to investigating whether the approach outlined above makes sense to facilitate incremental work but I'm also okay with progressing as is, i.e. working solely on my branch.

@fvictorio do you have any preference? Are you fine with me continuing the work and only opening the PR once we migrate almost all of it or would you prefer that we devote some time into supporting the partial migration without exposing it publicly via N-API?

Xanewok · 2024-07-23T13:25:35Z

I have addressed the feedback and changed the base of the PR to target the feature branch of #545.

As done in the #545, this additionally patches the Hardhat dep with a commit from https://github.com/NomicFoundation/hardhat/tree/refactor/port-vm-tracer; once it's merged, I will update the main companion branch of https://github.com/NomicFoundation/hardhat/tree/refactor/port-solidity-stack-traces to include that as well.

In the meantime, I may change the patching mechanism to support general git checkout as discussed on Slack with @fvictorio but that is unrelated to the changes here for now.

See NomicFoundation/hardhat@3aeeb56

Wodann

It's hard to judge the JS parts for me, but as long as the tests are passing that should be alright. It would still be good if Franco could double-check.

There are some comments left for the Rust part. Otherwise looks good

crates/edr_napi/src/trace/exit.rs

crates/edr_napi/src/trace/message_trace.rs

crates/edr_solidity/src/vm_tracer.rs

Xanewok · 2024-07-24T11:30:07Z

Forgot to mark it as ready for review, sorry about that.

hardhat-tests/test/internal/hardhat-network/stack-traces/test.ts

fvictorio · 2024-07-26T07:07:00Z

With respect to the Hardhat code, you can do this:

export { ExitCode } from "@nomicfoundation/edr";

instead of

import { ExitCode } from "@nomicfoundation/edr";
export { ExitCode };

It makes it a bit clearer that it's just a re-export. Not super-important, just FYI.

fvictorio

LGTM. Thanks for the explanation in the description, it was very useful.

(Tentative approval because the comments I left are very small)

Xanewok · 2024-07-26T10:37:24Z

TIL:

export { ExitCode } from "@nomicfoundation/edr";

I'll adapt the code in subsequent patches if that's okay to use this shorter syntax, not to have to re-squash and re-apply the commits in the Hardhat patch 🙏

* refactor: Port the codebase model construction and EVM decoding from HH * chore: Add a script that regenerates the pnpm patch for Hardhat * patch(hardhat): Cherry-pick and adapt for the unreleased Rip7212 PR See NomicFoundation/hardhat@f944cd5 * patch(hardhat): Cherry-pick a stack trace cleanup PR See NomicFoundation/hardhat@5739893 * patch(hardhat): Use the rewritten compiler to model logic from EDR See NomicFoundation/hardhat@49c66b6 * Port `MessageTrace` and `VMTracer` machinery from Hardhat (#531) * Some small improvements * Port `MessageTrace` and `VMTracer` machinery from Hardhat * Move HH-specific ExitCode logic to edr_napi * Remove unused `ExitCode::STATIC_STATE_CHANGE` * patch(hardhat): Re-use MessageTrace and VmTracer from EDR now See NomicFoundation/hardhat@3aeeb56 * chore: Re-run pnpm build * Address PR feedback * Address PR feedback * fixup: Apply formatting * refactor: Port ContractsIdentifier to Rust (#562) * chore: Port the BytecodeTrie from ContractsIdentifier * Migrate some library utils * Migrate some Opcode helpers * Finish the ContractsIdentifier port * Remove unused get_library_address_positions * Simplify the ContractsIdentifier port * patch(hardhat): Re-use the ContractsIdentifier from EDR * fixup: Fix a simple test * refactor: Port the `VmTraceDecoder` from Hardhat (#568) * fix: Correctly deserialize BuildInfo with `_format` field * refactor: Port the VmTraceDecoder from Hardhat * fix(napi): Correctly encode and decode `MessageTrace` types * refactor: Remove now unneeded functions * patch(hardhat): Port the VmTraceDecoder from Hardhat * Reformat and remove unused imports * fixup: Use a more correct type for the XYZTrace::bytecode field * fixup: Adjust test to now optional `deployedContract` property * refactor: Port `ReturnData` and `StackTraceEntryType` (#589) * feat: Port solidity-stack-trace.ts * feat: Port return-data.ts * patch(hardhat): Port return-data.ts and solidity-stack-trace.ts * fixup: Reformat files * fixup: let's see how tests feel about a number instead of a const enum variant * Make the comment about Rust type alias more clear [skip ci] * fix: Fully transpile in Mocha tests to correctly inline const enums (#594) It seems transpile-only transpiles per module, however we need to perform full "compilation" in order to correctly see and inline values for const enums. We use const enums because that's how they are emitted by napi-rs and there's no option to change that. Rather than invest into supporting that or working around it, let's just fully compile the test harness once. See https://www.typescriptlang.org/tsconfig/#isolatedModules for more context. * refactor: Port ErrorInferrer and SolidityTracer from Hardhat (#593) * Start porting SolidityTracer * Start porting ErrorInferrer * WIP: Keep Reference in MessageTraces * Port 99% of the ErrorInferrer Except `ErrorInferrer.inferAfterTracing`. I discovered that the original code creates a shallow copy of the stack trace during select heuristics and returns the modified one or the *original* one if the heuristic does not hit. One could keep track of possible series of changes that would have to be subsequently applied if the heuristic hits and returns a modified the stack trace but instead, I want the code to do exactly what the original code did. Rather than wrap every entry in a shareable `Rc` (they are not modified), I decided to make them clonable in the next PR, which means we need to not keep the `ClassInstance` around to derive the Clone trait. * Port mapped-inlined-internal-functions-heuristics.ts * refactor: Prune StackTraceEntry.message to make it clonable See previous commit "Port 99% of the ErrorInferrer". * refactor: Drop unused inner funcs in the ErrorInferrer * feat: Finish porting `ErrorInferrer` * feat: Finish porting `SolidityTracer` * refactor: Remove now unused utils IntoEither/IntoOption * patch(hardhat): Port ErrorInferrer and SolidityTracer * fixup! refactor: Prune StackTraceEntry.message to make it clonable * fixup: Reformat * Document the unsafety surrounding napi-rs's References * refactor: Remove some now unused functions * Address review feedback * fix a missing period * fix alphabetical sorting * refactor: Port stack-traces/debug.ts (#596) * Port over debugging facilities from debug.ts * patch(hardhat): Port debug.ts * fix: Make the depth optional in printMessageTrace * fixup: Formatting * Merge `main` into the stack trace port feature branch (#618) * fix: use remote chain id for pre-fork simulation (#567) * fix: use remote chain id for pre-fork simulation * Fix error message * chore: Bump hardhat to 2.22.7 (#571) * chore: Bump hardhat to 2.22.7 * fixup: Don't enable RIP-7212 in our tests * Adapt for NomicFoundation/hardhat#5411 * fix: prevent crash when returning large JSON responses (#569) * ci: update collaborator check in the benchmarks workflow (#574) * edr-0.5.1 (#559) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix: add json alias property in provider response (#582) * fix: add json alias property in provider response * Create empty-bobcats-refuse.md * edr-0.5.2 (#583) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * breaking change: rename response.json to response.data (#584) * breaking change: rename response.json to response.data * Create sour-donkeys-draw.md * Update sour-donkeys-draw.md * fix: improve error message and option to skip unsupported transaction type in debug trace (#606) * fix: improve error message and option to skip unsupported tx type in debug trace * Address code review feedback * Handle unsupported transaction type requested * Fix test setup * ci: remove review-related slack notifications (#612) * chore: js tooling improvements (#609) * Fix pnpm warning * Add syncpakc * Add syncpack * Run syncpack format * Run syncpack on CI * Add setup-node action * Fixes related to upgrading prettier * Run prettier * Add setup-rust action * Run prettier in edr_napi * Add lint scripts to packages * Run prettier in crates/tools/js/benchmark * Port benchmark code to typescript * Add eslint to all packages * Upgrade @types/node to v20 * Fix broken build:edr script * Resolve benchmark output path * chore: upgrade hardhat and add patch (#613) * Upgrade Hardhat to v2.22.9 * Add patch for hardhat#5664 * chore: Bump to vanilla Hardhat 2.22.9 * chore: Regenerate the Hardhat patch to use the new EDR internals * chore: Bump @napi-rs/cli to fix duplicated napi typedefs See <napi-rs/napi-rs#2088> * fixup: Prettify test.ts * Fixes after testing in OZ (#625) * fix: add bounds checks * fix: check that steps is not empty before traversing it * fixup: formatting [skip ci] --------- Co-authored-by: Igor Matuszewski <xanewok@gmail.com> --------- Co-authored-by: Agost Biro <5764438+agostbiro@users.noreply.github.com> Co-authored-by: Wodann <Wodann@users.noreply.github.com> Co-authored-by: Piotr Galar <piotr.galar@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Franco Victorio <victorio.franco@gmail.com> * chore: Restore old test.ts and remove some `as any` conversions * WIP: Benchmark with patched Hardhat to use new EDR internals * Revert "WIP: Benchmark with patched Hardhat to use new EDR internals" This reverts commit ac2e012. * Remove the gen-hardhat-patches script --------- Co-authored-by: Agost Biro <5764438+agostbiro@users.noreply.github.com> Co-authored-by: Wodann <Wodann@users.noreply.github.com> Co-authored-by: Piotr Galar <piotr.galar@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Franco Victorio <victorio.franco@gmail.com>

Xanewok commented Jun 24, 2024

View reviewed changes

Xanewok mentioned this pull request Jun 25, 2024

Clean up the VMTracer and trace events interface in TS NomicFoundation/hardhat#5429

Merged

3 tasks

Wodann requested changes Jun 25, 2024

View reviewed changes

Xanewok marked this pull request as draft July 3, 2024 08:25

Some small improvements

ccce385

Xanewok force-pushed the refactor/port-vm-tracer branch from 86d6014 to 2c2af0c Compare July 23, 2024 13:17

Xanewok changed the base branch from main to refactor/port-solidity-stack-traces July 23, 2024 13:17

Xanewok added the no changeset needed This PR doesn't require a changeset label Jul 23, 2024

Xanewok requested review from Wodann and a team July 23, 2024 13:26

Xanewok added 4 commits July 23, 2024 15:31

Port MessageTrace and VMTracer machinery from Hardhat

4e1e799

Move HH-specific ExitCode logic to edr_napi

f473302

Remove unused ExitCode::STATIC_STATE_CHANGE

81412e6

patch(hardhat): Re-use MessageTrace and VmTracer from EDR now

b0bfc20

See NomicFoundation/hardhat@3aeeb56

Xanewok force-pushed the refactor/port-vm-tracer branch from 2c2af0c to b0bfc20 Compare July 23, 2024 13:32

Wodann requested changes Jul 23, 2024

View reviewed changes

Xanewok added 2 commits July 23, 2024 17:59

chore: Re-run pnpm build

d1de170

Address PR feedback

146e939

Xanewok marked this pull request as ready for review July 24, 2024 11:29

Xanewok mentioned this pull request Jul 25, 2024

refactor: Port ContractsIdentifier to Rust #562

Merged

Xanewok requested a review from fvictorio July 25, 2024 08:59

fvictorio reviewed Jul 26, 2024

View reviewed changes

hardhat-tests/test/internal/hardhat-network/stack-traces/test.ts Outdated Show resolved Hide resolved

fvictorio approved these changes Jul 26, 2024

View reviewed changes

Xanewok mentioned this pull request Jul 26, 2024

Robustly check for precompile numbers in the tracing logic #565

Open

Address PR feedback

b455234

fixup: Apply formatting

de86baa

Xanewok merged commit 51ce7f9 into refactor/port-solidity-stack-traces Jul 26, 2024
36 checks passed

Xanewok deleted the refactor/port-vm-tracer branch July 26, 2024 11:11

Xanewok added this to the Solidity tracing port milestone Jul 26, 2024

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port `MessageTrace` and `VMTracer` machinery from Hardhat #531

Port `MessageTrace` and `VMTracer` machinery from Hardhat #531

Xanewok commented Jun 24, 2024 •

edited

Loading

changeset-bot bot commented Jun 24, 2024 •

edited

Loading

Xanewok Jun 24, 2024

Wodann Jun 25, 2024

Xanewok Jul 23, 2024

Wodann left a comment

Wodann Jun 25, 2024

Wodann Jun 25, 2024

Xanewok Jul 23, 2024

Wodann Jun 25, 2024

Xanewok Jul 23, 2024

Wodann Jun 25, 2024

Xanewok Jul 23, 2024

Wodann Jun 25, 2024

Xanewok commented Jul 3, 2024

Xanewok commented Jul 23, 2024

Wodann left a comment

Xanewok commented Jul 24, 2024

fvictorio commented Jul 26, 2024 •

edited

Loading

fvictorio left a comment •

edited

Loading

Xanewok commented Jul 26, 2024

	#[derive(Debug, PartialEq, Clone)]
	#[derive(Clone, Debug, PartialEq)]

		// TODO: Make this nicer and make sure the U160 logic/precompile comparison
		// logic works

Port MessageTrace and VMTracer machinery from Hardhat #531

Port MessageTrace and VMTracer machinery from Hardhat #531

Conversation

Xanewok commented Jun 24, 2024 • edited Loading

changeset-bot bot commented Jun 24, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wodann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Jul 3, 2024

Xanewok commented Jul 23, 2024

Wodann left a comment

Choose a reason for hiding this comment

Xanewok commented Jul 24, 2024

fvictorio commented Jul 26, 2024 • edited Loading

fvictorio left a comment • edited Loading

Choose a reason for hiding this comment

Xanewok commented Jul 26, 2024

Port `MessageTrace` and `VMTracer` machinery from Hardhat #531

Port `MessageTrace` and `VMTracer` machinery from Hardhat #531

Xanewok commented Jun 24, 2024 •

edited

Loading

changeset-bot bot commented Jun 24, 2024 •

edited

Loading

fvictorio commented Jul 26, 2024 •

edited

Loading

fvictorio left a comment •

edited

Loading