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

fix: prevent crash when returning large JSON responses #569

Merged
merged 1 commit into from
Aug 1, 2024
Merged

fix: prevent crash when returning large JSON responses #569

merged 1 commit into from
Aug 1, 2024

Conversation

Wodann
Copy link
Member

@Wodann Wodann commented Jul 29, 2024

Resolves #543

@Wodann Wodann self-assigned this Jul 29, 2024
@changeset-bot changeset-bot
Copy link

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

🦋 Changeset detected

Latest commit: cc5f4c6

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 Patch

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

@Wodann Wodann had a problem deploying to github-action-benchmark July 29, 2024 22:36 — with GitHub Actions Failure
@Wodann Wodann had a problem deploying to github-action-benchmark July 30, 2024 06:02 — with GitHub Actions Failure
@agostbiro
Copy link
Member

agostbiro commented Jul 30, 2024 •
edited
Loading

Thanks for this fix, the code looks good, but unfortunately it looks like there is an 115% slowdown over all and 120% slowdown on the synthetix benchmark. I reran the benchmarks just in case, but we probably have to come up with a different approach. The simplest that comes to mind is instead of making a JSON-RPC call for debug trace transaction, expose a method through NAPI that returns the DebugTraceResult that is still in the repo.

Edit: it failed again with similar numbers.

@fvictorio fvictorio mentioned this pull request Jul 30, 2024
Closed
@Xanewok Xanewok mentioned this pull request Jul 31, 2024
Merged
@Wodann Wodann had a problem deploying to github-action-benchmark July 31, 2024 17:13 — with GitHub Actions Error
@Wodann Wodann had a problem deploying to github-action-benchmark July 31, 2024 17:22 — with GitHub Actions Failure
@Wodann Wodann temporarily deployed to github-action-benchmark July 31, 2024 17:46 — with GitHub Actions Inactive
Wodann
Wodann commented Jul 31, 2024
View reviewed changes
crates/tools/js/benchmark/index.js
@@ -380,6 +379,7 @@ function preprocessConfig(config) {
}

config.providerConfig.minGasPrice = BigInt(config.providerConfig.minGasPrice);
config.providerConfig.enableRip7212 = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

All other changes apart from this line were a result of the formatter. I needed to make this change to allow old snapshots to continue working with the newly added configuration field.

cc: @agostbiro

Wodann
Wodann commented Jul 31, 2024
View reviewed changes
crates/edr_napi/package.json
@@ -55,8 +56,8 @@
"universal": "napi universal",
"version": "napi version",
"pretest": "pnpm build",
"test": "pnpm tsc && mocha --recursive \"test/**/*.ts\"",
"testNoBuild": "pnpm tsc && mocha --recursive \"test/**/*.ts\"",
"test": "pnpm tsc && node --max-old-space-size=8192 node_modules/mocha/bin/_mocha --recursive \"test/**/*.ts\"",
Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to introduce a larger heap size to prevent the newly added test from crashing in V8 with an OOM error

agostbiro
agostbiro approved these changes Aug 1, 2024
View reviewed changes
Copy link
Member

@agostbiro agostbiro left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM with one question

@Wodann Wodann temporarily deployed to github-action-benchmark August 1, 2024 13:27 — with GitHub Actions Inactive
@Wodann Wodann added this pull request to the merge queue Aug 1, 2024
Merged via the queue into main with commit 8ae31b9 Aug 1, 2024
17 checks passed
@Wodann Wodann deleted the fix/issue-543 branch August 1, 2024 13:27
fvictorio
fvictorio reviewed Aug 2, 2024
View reviewed changes
crates/edr_napi/index.d.ts
@@ -418,7 +418,8 @@ export class Provider {
setVerboseTracing(verboseTracing: boolean): void
}
export class Response {
get json(): string
/** Returns the response data as a JSON string or a JSON object. */
get data(): string | any
Copy link
Member

Choose a reason for hiding this comment

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

In TypeScript, any type in a union with any becomes any (like multiplying by 0 or, more precisely, doing a set union with the universal set). So this is just any and not ideal.

I'm not 100% what the right type would be here. I think string | object would work. But if that's not easily doable with N-API, then it's fine for this to be any for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can look into it but the serde_json::Value translates to any

@fvictorio fvictorio mentioned this pull request Aug 5, 2024
Merged
fvictorio added a commit that referenced this pull request Aug 22, 2024
@Wodann @github-actions
@fvictorio @Xanewok @agostbiro @galargh
chore: merge main into feat/multichain (#615)
226295c 
* edr-0.4.1

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* chore: run prettier (#535)

* test: avoid unconditional reverts in stack traces tests (#537)

* fix: allow missing nonce in remote blocks (#539)

* fix: allow missing nonce in remote blocks

* misc: add changeset

* ci: upgrade pnpm/action-setup (#541)

* ci: enable some debug logs for hardhat tests (#538)

* Sync with recent Hardhat changes and deduplicate the Hardhat dep (#544)

* chore: Explicitly depend on semver and fs-extra in hardhat-tests

Appeases ESLint. These are de facto pulled by other dependencies but
it's implicit.

* refactor: Unify ts-node and use the one currently used by Hardhat

Deduping the hardhat package makes it easier to patch it while working
on the stack trace porting feature branch.

* chore: Use the newest Hardhat 2.22.6

This will make patching the changes easier to review as we will sync
with the upstream as it has some changes already related to the stack
traces that we port.

* feat: upgrade revm dependencies (#546)

* feat: upgrade revm dependencies

* Create wild-phones-drum.md

* edr-0.4.2 (#540)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* ci: remove path checks for benchmarks (#550)

* ci: remove path restrictions for EDR jobs (#551)

* ci: check that index.d.ts is up to date (#553)

* feat: add support for RIP-7212 (#552)

* feat: add support for RIP-7212

* misc: add changelog

* fix: updated index.d.ts

* fix: revert rename of InvalidFEOpcode

* fix: set enableRip7212 in ProviderConfig

* test: validate that disabling RIP-7212 works

* refactor: use runtime variable instead of const

* Path Hardhat dev dep to work with latest EDR changes

EDR uses Hardhat as a dev dependency to run some javascript tests. The
way this works is that the tests are run using Hardhat, but we use pnpm
to override the EDR dependency in Hardhat with the local one. This works
fine as long as there are no breaking changes in EDR. When there are, we
have a circular dependency problem: we can't publish a new version of
EDR until the tests pass, but for the tests to pass we need a version of
Hardhat that works with the new version of EDR.

A temporary workaround for this is to use `pnpm patch` to temporarily
modify the Hardhat code in a way that works with the breaking change.
In this case, this just means adding the new field when constructing
the provider.

---------

Co-authored-by: Franco Victorio <[email protected]>

* edr-0.5.0 (#555)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* build: upgrade revm to v12 (#557)

* build: upgrade revm to v12

* test: remove invalid test

* misc: add changeset

* 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

---------

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]>
Co-authored-by: Igor Matuszewski <[email protected]>
Co-authored-by: Agost Biro <[email protected]>
Co-authored-by: Piotr Galar <[email protected]>
Xanewok added a commit that referenced this pull request Aug 26, 2024
@Xanewok @agostbiro
@Wodann @galargh @github-actions @fvictorio
Merge main into the stack trace port feature branch (#618)
1be6718 
* 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]>
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 left review comments

@agostbiro agostbiro agostbiro approved these changes

Assignees

@Wodann Wodann

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Error: Failed to convert rust String into napi string" on debug_traceTransaction
3 participants
@Wodann @agostbiro @fvictorio