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

add support for wasm compile targets #339

Merged
merged 27 commits into from
Aug 17, 2024
Merged

Conversation

magicxyyz
Copy link
Contributor

@magicxyyz magicxyyz commented Jul 11, 2024

part of NIT-2631

This PR:

  • changes wasm store schema to support storing assembly for multiple targets
  • adds support for multiple targets in StateDB, including recording UserWasm for specific targets

@cla-bot cla-bot bot added the s label Jul 11, 2024
@@ -7,16 +7,11 @@ import (
"github.com/ethereum/go-ethereum/core/rawdb"
)

func (db *cachingDB) ActivatedAsm(moduleHash common.Hash) ([]byte, error) {
func (db *cachingDB) ActivatedAsm(targetName string, moduleHash common.Hash) ([]byte, error) {
if asm, _ := db.activatedAsmCache.Get(moduleHash); len(asm) > 0 {

This comment was marked as outdated.

@magicxyyz magicxyyz marked this pull request as ready for review August 6, 2024 15:49
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

very good.
tiny suggestions

core/state/statedb_arbitrum.go Outdated Show resolved Hide resolved
const (
TargetWavm = "wavm"
TargetArm = "arm"
TargetX86 = "x86"
Copy link
Contributor

Choose a reason for hiding this comment

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

not 100% certain - but I think maybe TargetArm and TargetAsm should be identical to the runtime.GOARCH on arm64/x86 architectures, which will be compatible with some exiting code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used GOARCH strings + I renamed the constants to TargetArm64 and TargetAmd64 to make it more consistent

the new version of schema is sort of compatible with previous version - opening old schema will be detected as empty database, so we don't need to increase the version number

note: previously the version key didn't exist, for that case version 0 is returned when trying to read the version
@magicxyyz magicxyyz requested a review from tsahee August 9, 2024 16:51
@gligneul gligneul self-requested a review August 9, 2024 19:07
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looks good, just minor comments.

core/rawdb/accessors_state_arbitrum.go Outdated Show resolved Hide resolved
core/rawdb/accessors_state_arbitrum.go Outdated Show resolved Hide resolved
core/rawdb/accessors_state_arbitrum.go Outdated Show resolved Hide resolved
core/state/statedb_arbitrum.go Outdated Show resolved Hide resolved
magicxyyz and others added 5 commits August 12, 2024 12:33
Co-authored-by: Gabriel de Quadros Ligneul <8294320+gligneul@users.noreply.github.com>
Co-authored-by: Gabriel de Quadros Ligneul <8294320+gligneul@users.noreply.github.com>
tsahee
tsahee previously approved these changes Aug 14, 2024
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

LGTM

core/rawdb/schema_arbitrum.go Outdated Show resolved Hide resolved
core/rawdb/schema_arbitrum.go Outdated Show resolved Hide resolved
if dbErr == nil {
asmMap[target] = asm
} else {
err = errors.Join(fmt.Errorf("failed to read activated asm from database for target %v and module %v: %w", target, moduleHash, dbErr), err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Side note, but I had no idea errors.Join existed, that's really helpful!

Copy link
Contributor Author

@magicxyyz magicxyyz Aug 16, 2024

Choose a reason for hiding this comment

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

I hadn't either, saw that during some other PR review :) It solves the multiple errors issue quite ok: not sure if that is formatted nicely when logged, but full errors information is preserved

gligneul
gligneul previously approved these changes Aug 15, 2024
@magicxyyz magicxyyz dismissed stale reviews from gligneul and tsahee via aa44bd4 August 15, 2024 23:43
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM (pending nitro PR approval)

@PlasmaPower PlasmaPower merged commit 85dc1b7 into master Aug 17, 2024
7 checks passed
@PlasmaPower PlasmaPower deleted the wasm-store-compile-target branch August 17, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants