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.

Sign up for GitHub

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

Jump to bottom

refactor: Port the VmTraceDecoder from Hardhat #568

Merged
merged 8 commits into from
Jul 31, 2024
Merged

refactor: Port the VmTraceDecoder from Hardhat #568

merged 8 commits into from
Jul 31, 2024

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jul 26, 2024

This ports of VmTraceDecoder from Hardhat; what's left now is the error inferrer and finally solidity stack trace logic 🥳

What's more interesting, however, is the fix for the MessageTrace and others:

  1. There's a napi-rs bug that incorrectly validates and converts an object that has a required property but can be explicitly undefined, e.g. deployedContract: Bytecode | undefined. That's the case for CreateMessageTrace and so this is worked around using an optional property, however...
  2. With this, the CallMessageTrace is a strict superset of CreateMessageTrace. We need to be careful with the type order when using the N-API type Either to always attempt converting and validating the most specific type first, otherwise we may incorrectly convert into a less specific type, skipping some additional properties it may have.

Link to the companion commit: NomicFoundation/hardhat@137cfb3

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 99c5bae

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 Xanewok added the no changeset needed This PR doesn't require a changeset label Jul 26, 2024
@Xanewok Xanewok requested a review from a team July 26, 2024 18:17
@Xanewok Xanewok added this to the Solidity tracing port milestone Jul 26, 2024
Wodann
Wodann reviewed Jul 26, 2024
View reviewed changes
Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Some questions and comments

crates/edr_napi/src/trace/message_trace.rs
// NOTE: we can't use Option<T> as this is converted to an optional property,
// which is not backwards compatible with the current JS interface
pub deployed_contract: Either<Uint8Array, Undefined>,
// HACK: It seems that `Either<Uint8Array, Undefined>` means exactly what we
Copy link
Member

Choose a reason for hiding this comment

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

FYI, backwards compatibility for Hardhat's internals is not a requirement, so if it's easier, you can just adopt Option<Uint8Array> and change the Hardhat code.

crates/edr_napi/src/trace/message_trace.rs
// This is explicitly `any` on the JS side to side-step the type-checking until we port
pub bytecode: Option<Value>,
pub steps: Vec<Either4<EvmStep, PrecompileMessageTrace, CallMessageTrace, CreateMessageTrace>>,
#[napi(ts_type = "Bytecode")]
Copy link
Member

Choose a reason for hiding this comment

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

If this is now filled on the Rust side, is there a reason we can't define an N-API Bytecode type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do define and use Bytecode elsewhere already but there's some subtle nuance here:

  1. We can't use ClassInstance here because it internally holds a &'static mut T so that's UB because we probably will often have traces that point to the same bytecode
  2. I introduced the ClassInstanceRef that guards the access with a RefCell, however it's not possible to directly use it here as it'd require ToNapiValue and FromNapiValue implementation...
  3. and while it might be feasible to implement it using ToNapiValue (it can pass the underlying object), however...
  4. implementing FromNapiValue would be very tricky:
    a. the instances are shared, so on the Rust side are wrapped in Rc and internally uses napi::Ref for napi-side ref-counting of the underlying object, so...
    b. converting at the FFI boundary without a side-table would have to create new Rc'd objects; ignoring the potential perf drawbacks, it would also be incorrect because it would lead to multiple RefCells guarding the same memory access and...
    c. introducing the side-table means introducing a static map per JS class on the Rust side, hooking into the napi lifecycle to manage the objects on the Rust side once they're freed on the JS side, modifying the ClassInstanceRef helper to do the same on the Rust side...
    d. ...at which point we'd be reimplementing quite a bit of higher-level convenience code that arguably should be on the napi-rs side itself; hopefully, we won't need as much JS glue anymore once the port is complete.

Let me know if the context makes sense; I'm more than happy if we simplified this somehow but I didn't manage to find a way to make it both safe and convenient 😢

Copy link
Member

Choose a reason for hiding this comment

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

It sounds very complicated. I'll trust your judgement!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I'll try to embed a bit more context and/or more explain these things more concisely next time 🙏

fvictorio
fvictorio approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait for @Wodann's approval

@Xanewok Xanewok merged commit fc79cdb into refactor/port-solidity-stack-traces Jul 31, 2024
40 checks passed
@Xanewok Xanewok deleted the refactor/port-vm-trace-decoder branch July 31, 2024 17:43
github-merge-queue bot pushed a commit that referenced this pull request Aug 27, 2024
@Xanewok @agostbiro
@Wodann @galargh @github-actions @fvictorio
feat: Port existing stack trace refinement 1:1 from Hardhat (#545)
4a39a8d 
* 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 <[email protected]>

---------

Co-authored-by: Agost Biro <[email protected]>
Co-authored-by: Wodann <[email protected]>
Co-authored-by: Piotr Galar <[email protected]>
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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Wodann <[email protected]>
Co-authored-by: Piotr Galar <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fvictorio fvictorio fvictorio approved these changes

@Wodann Wodann Wodann approved these changes

Assignees
No one assigned
Labels
no changeset needed This PR doesn't require a changeset
Projects
None yet
Milestone
Solidity tracing port
Development

Successfully merging this pull request may close these issues.
3 participants
@Xanewok @fvictorio @Wodann