Change `edr_napi::provider::Response::solidity_trace` to return a stack trace #734

agostbiro · 2024-12-03T14:55:59Z

The goal of this PR is to port Solidity stack trace generation to pure Rust and without NAPI dependencies. Doing this also necessitated porting hardhat_addCompilationResult to EDR from Hardhat.

Most of the additions edr_solidity come from moving edr_napi code there and stripping the NAPI dependencies. As the stack trace error inference involves complex heuristics, this code was originally ported verbatim from JS which lead to non-idiomatic Rust code. I originally wanted to clean it up as part of moving inference code from edr_napi to edr_solidity, but I only managed to do it partially due to running out of time, so much of it remains unidiomatic Rust code. I'd ask to exclude this aspect from review.

I think these are the important changes in edr_solidity that need review:

I renamed types whose names came from Hardhat + EthereumJS and are no longer exposed to JS to names that I feel make more sense in EDR. E.g. Bytecode is renamed to ContractMetadata, VmTrace is renamed to NestedTrace (to differentiate from flat traces from edr_evm) and VmTraceDecoder is renamed to ContractDecoder.
Introduced an internal nested trace type in NestedTracer that has interior mutability, but it's converted to a nested trace type without interior mutability when returned from NestedTracer. This way the risk of panics from interior mutability is confined to the nested_tracer module.
Using Arc<RwLock> in build model types instead of Rc<RefCell>. This was necessary, because the EDR provider holds on to those types now to support hardhat_addCompilationResult, so they need to be Send. We don't really need to write to the types held by Arc<RwLock> outside the edr_solidity crate, but it took too long to remove that so I gave up on it as part of this PR.

Besides edr_solidity, on the Rust side changes to the provider in edr_provider and edr_napi would be important to review I think.

Hardhat companion PR: NomicFoundation/hardhat#6085

changeset-bot · 2024-12-03T14:56:03Z

🦋 Changeset detected

Latest commit: ee22667

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@nomicfoundation/edr	Minor

Not sure what this means? Click here to learn what changesets are.

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

fvictorio · 2024-12-04T13:48:21Z

@agostbiro I pushed the changes to run the stack traces tests using the new stackTrace getter. You can check that tests are not passing by running this in hardhat-tests:

pnpm mocha test/internal/hardhat-network/stack-traces/test.ts --bail

This will stop at the first failure.

The test that is failing runs this bytecode:

0x608060405234801561001057600080fd5b506004361061002b5760003560e01c8063f8a8fd6d14610030575b600080fd5b61003861003a565b005b6000604051610048906100f0565b604051809103906000f080158015610064573d6000803e3d6000fd5b5090508073ffffffffffffffffffffffffffffffffffffffff1663a9cc47186040518163ffffffff1660e01b8152600401600060405180830381600087803b1580156100af57600080fd5b505af11580156100c3573d6000803e3d6000fd5b505050506040513d6000823e3d601f19601f820116820180604052506100ec91908101906101e1565b5050565b6103078061029983390190565b600082601f83011261010e57600080fd5b815161012161011c8261024f565b610222565b9150818183526020840193506020810190508385604084028201111561014657600080fd5b60005b83811015610176578161015c8882610180565b845260208401935060408301925050600181019050610149565b5050505092915050565b60006040828403121561019257600080fd5b61019c6040610222565b905060006101ac848285016101cc565b60008301525060206101c0848285016101cc565b60208301525092915050565b6000815190506101db81610281565b92915050565b6000602082840312156101f357600080fd5b600082015167ffffffffffffffff81111561020d57600080fd5b610219848285016100fd565b91505092915050565b6000604051905081810181811067ffffffffffffffff8211171561024557600080fd5b8060405250919050565b600067ffffffffffffffff82111561026657600080fd5b602082029050602081019050919050565b6000819050919050565b61028a81610277565b811461029557600080fd5b5056fe608060405234801561001057600080fd5b506102e7806100206000396000f3fe608060405234801561001057600080fd5b506004361061002b5760003560e01c8063a9cc471814610030575b600080fd5b61003861004e565b604051610045919061021b565b60405180910390f35b60606040517f08c379a00000000000000000000000000000000000000000000000000000000081526004016100829061023d565b60405180910390fd5b61009361010d565b81526020019060019003908161008b5790505090506040518060400160405280600181526020016002815250816000815181106100cc57fe5b60200260200101819052506040518060400160405280600281526020016003815250816001815181106100fb57fe5b60200260200101819052508091505090565b604051806040016040528060008152602001600081525090565b600061013383836101dd565b60408301905092915050565b600061014a8261026d565b6101548185610285565b935061015f8361025d565b8060005b838110156101905781516101778882610127565b975061018283610278565b925050600181019050610163565b5085935050505092915050565b60006101aa600883610296565b91507f44206661696c65640000000000000000000000000000000000000000000000006000830152602082019050919050565b6040820160008201516101f3600085018261020c565b506020820151610206602085018261020c565b50505050565b610215816102a7565b82525050565b60006020820190508181036000830152610235818461013f565b905092915050565b600060208201905081810360008301526102568161019d565b9050919050565b6000819050602082019050919050565b600081519050919050565b6000602082019050919050565b600082825260208201905092915050565b600082825260208201905092915050565b600081905091905056fea2646970667358221220fbf4de57de6e06c6bac2a7ec2392518b6eb777da534fdd38b8bd290988583c6764736f6c63430006030033a264697066735822122070cadf9cc927971990ad70f87b31a21294cb7ba5f9498d6595ebde6fab6c9a9b64736f6c63430006030033

with calldata 0xf8a8fd6d (which is just test()). I double-checked that the bytecode is the correct one, and that it indeed fails when that function is called.

* wip: make tracing config a provider argument * Patch Hardhat to pass tracing config to provider --------- Co-authored-by: Franco Victorio <[email protected]>

This function has to match the latest tested solc version in the stack traces tests.

Wodann · 2024-12-31T14:57:52Z

We don't really need to write to the types held by Arc outside the edr_solidity crate, but it took too long to remove that so I gave up on it as part of this PR.

Is there an issue to track follow-up work?

agostbiro · 2024-12-31T16:24:18Z

Is there an issue to track follow-up work?

Added in 52b4958

Wodann

Overall LGTM!

There are some small things that I pointed out; which might need to be fixed before merging.

Wodann · 2024-12-31T18:04:51Z

crates/edr_provider/src/data.rs

@@ -330,6 +337,9 @@ impl<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch> ProviderData<LoggerErr
            self.subscriber_callback.clone(),
            self.call_override.clone(),
            config,
+            // `hardhat_reset` doesn't discard contract metadata added with


If we're doing an overhaul of hardhat_reset for HH3, should we reconsider the design of this too?

cc: @fvictorio

This is a good question. I think we might want to remove hardhat_reset in HH3.

Wodann · 2024-12-31T18:25:27Z

crates/edr_solidity/src/error_inferrer.rs

+    let retained_indices: HashSet<_> = stacktrace
+        .iter()
+        .enumerate()
+        .filter(|(idx, frame)| {


Can't you use filter_map instead of splitting it into two iterators?

Not sure tbh. I moved this code without changes from edr_napi and I'd rather not change it now as @fvictorio has been doing manual testing.

Wodann · 2024-12-31T18:27:58Z

crates/edr_solidity/src/source_map.rs

 ) -> Vec<Instruction> {
    let source_maps = uncompress_sourcemaps(compressed_sourcemaps);
+    if print_debug {
+        println!("num source_maps: {:?}", source_maps.len());


Are these prints intended to remain?

Good catch thanks! I fixed it by force pushing as we probably want to merge this while retaining the commit history as it's a big PR and it'd help with debugging later if needed.

Wodann · 2024-12-31T18:34:24Z

crates/edr_solidity/src/source_map.rs

@@ -198,6 +204,10 @@ pub fn decode_instructions(
            location,
        };

+        if bytes_index == 22249 {
+            println!("instruction {:?}", instruction);


Seems like a remnant from debugging

Wodann · 2024-12-31T18:34:35Z

crates/edr_solidity/src/source_map.rs

@@ -207,6 +217,11 @@ pub fn decode_instructions(
        add_unmapped_instructions(&mut instructions, bytecode);
    }

+    if print_debug {
+        println!("instructions length: {:?}", instructions.len());


Is this leftover from debugging?

Wodann · 2024-12-31T18:35:09Z

crates/edr_solidity/Cargo.toml

 serde = { version = "1.0.158", default-features = false, features = ["std"] }
 serde_json = { version = "1.0.89", features = ["preserve_order"] }
 strum = { version = "0.26.0", features = ["derive"] }
+thiserror = "1.0.58"
+semver = "1.0.23"


Nit: alphabetical (cargo add) ordering

Fixed in ee22667

Wodann · 2024-12-31T18:39:09Z

For the record, I didn't do an in-depth review of all code changes that were moved from edr_napi to edr_solidity, as I assumed that most of the code remained the same structure/flow

Duplicate stack trace logic in Rust

83b57b0

agostbiro linked an issue Dec 3, 2024 that may be closed by this pull request

Change edr_napi::provider::Response::solidity_trace to return a stack trace #733

Closed

agostbiro temporarily deployed to github-action-benchmark December 3, 2024 14:56 — with GitHub Actions Inactive

agostbiro marked this pull request as draft December 3, 2024 14:56

wip

3f87c6f

fvictorio temporarily deployed to github-action-benchmark December 4, 2024 13:46 — with GitHub Actions Inactive

Set throwOnTransactionFailures true and get tx hash from error

fad1358

agostbiro temporarily deployed to github-action-benchmark December 4, 2024 17:30 — with GitHub Actions Inactive

refactor: pass tracing config to provider (#738)

70ee8de

* wip: make tracing config a provider argument * Patch Hardhat to pass tracing config to provider --------- Co-authored-by: Franco Victorio <[email protected]>

agostbiro temporarily deployed to github-action-benchmark December 5, 2024 08:20 — with GitHub Actions Inactive

Remove solidity_trace getter from Response

36059bb

agostbiro temporarily deployed to github-action-benchmark December 5, 2024 09:58 — with GitHub Actions Inactive

Remove duplicated NAPI logic

781b57e

agostbiro had a problem deploying to github-action-benchmark December 5, 2024 10:30 — with GitHub Actions Failure

remove getContractAndFunctionNameCallback

748b678

agostbiro had a problem deploying to github-action-benchmark December 6, 2024 10:01 — with GitHub Actions Failure

fvictorio added 3 commits December 6, 2024 11:05

Add link_hex_string_bytecode back to edr_napi

5277a66

Adapt hardhat patch to latest changes

8e59550

Remove unused stuff in hardhat-tests

433ad4d

fvictorio temporarily deployed to github-action-benchmark December 6, 2024 10:05 — with GitHub Actions Inactive

Refactor for type safety

4cef55a

agostbiro temporarily deployed to github-action-benchmark December 10, 2024 13:36 — with GitHub Actions Inactive

agostbiro mentioned this pull request Dec 18, 2024

Remove bail_on_(call|transaction)_failure from EDR #743

Open

agostbiro added 2 commits December 19, 2024 18:45

Make build model Send + Sync

8de6f2e

Add hardhat_addCompilationResult

fad2851

agostbiro temporarily deployed to github-action-benchmark December 19, 2024 22:46 — with GitHub Actions Inactive

fvictorio added 2 commits December 20, 2024 09:32

Remove hardhat_getStackTraceFailuresCount logic

19eed93

Fix missing argument in test

5f34c04

fvictorio mentioned this pull request Dec 27, 2024

Get stack traces from EDR's response NomicFoundation/hardhat#6085

Merged

3 tasks

fvictorio added 2 commits December 27, 2024 13:58

Upgrade Hardhat dependency

a05caeb

Export a get_latest_supported_solc_version function

e82fcac

This function has to match the latest tested solc version in the stack traces tests.

fvictorio temporarily deployed to github-action-benchmark December 27, 2024 13:10 — with GitHub Actions Inactive

Make stackTrace throw on error

6e19b02

agostbiro temporarily deployed to github-action-benchmark December 30, 2024 16:01 — with GitHub Actions Inactive

Adapt tests and patch to new .stackTrace() semantics

03b8e36

fvictorio temporarily deployed to github-action-benchmark December 30, 2024 17:43 — with GitHub Actions Inactive

agostbiro requested a review from Wodann December 31, 2024 12:50

agostbiro marked this pull request as ready for review December 31, 2024 12:50

agostbiro assigned agostbiro and fvictorio Dec 31, 2024

agostbiro mentioned this pull request Dec 31, 2024

Remove Arc<RwLock> from build model types in edr_solidity #759

Open

agostbiro temporarily deployed to github-action-benchmark December 31, 2024 16:24 — with GitHub Actions Inactive

Wodann approved these changes Dec 31, 2024

View reviewed changes

Add TODO comments

4adac6b

agostbiro force-pushed the refactor/stack-trace-to-rs branch from f1f3d59 to 4adac6b Compare January 2, 2025 09:19

agostbiro had a problem deploying to github-action-benchmark January 2, 2025 09:19 — with GitHub Actions Error

Fix imports

ee22667

agostbiro temporarily deployed to github-action-benchmark January 2, 2025 09:24 — with GitHub Actions Inactive

fvictorio approved these changes Jan 2, 2025

View reviewed changes

agostbiro added this pull request to the merge queue Jan 2, 2025

Merged via the queue into main with commit 215b180 Jan 2, 2025
37 checks passed

agostbiro deleted the refactor/stack-trace-to-rs branch January 2, 2025 11:27

This was referenced Jan 6, 2025

Incorporate stack traces refactor in feat/multichain #766

Open

feat: Solidity stack traces NomicFoundation/hardhat#6122

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `edr_napi::provider::Response::solidity_trace` to return a stack trace #734

Change `edr_napi::provider::Response::solidity_trace` to return a stack trace #734

agostbiro commented Dec 3, 2024 •

edited

Loading

changeset-bot bot commented Dec 3, 2024 •

edited

Loading

fvictorio commented Dec 4, 2024

Wodann commented Dec 31, 2024

agostbiro commented Dec 31, 2024

Wodann left a comment

Wodann Dec 31, 2024

fvictorio Jan 2, 2025

Wodann Dec 31, 2024

agostbiro Jan 2, 2025

Wodann Dec 31, 2024

agostbiro Jan 2, 2025

Wodann Dec 31, 2024

agostbiro Jan 2, 2025

Wodann Dec 31, 2024

agostbiro Jan 2, 2025

Wodann Dec 31, 2024

agostbiro Jan 2, 2025

Wodann commented Dec 31, 2024

Change edr_napi::provider::Response::solidity_trace to return a stack trace #734

Change edr_napi::provider::Response::solidity_trace to return a stack trace #734

Conversation

agostbiro commented Dec 3, 2024 • edited Loading

changeset-bot bot commented Dec 3, 2024 • edited Loading

🦋 Changeset detected

fvictorio commented Dec 4, 2024

Wodann commented Dec 31, 2024

agostbiro commented Dec 31, 2024

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

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

Wodann commented Dec 31, 2024

Change `edr_napi::provider::Response::solidity_trace` to return a stack trace #734

Change `edr_napi::provider::Response::solidity_trace` to return a stack trace #734

agostbiro commented Dec 3, 2024 •

edited

Loading

changeset-bot bot commented Dec 3, 2024 •

edited

Loading