From 1bb666593d668fd93f38ffa39f71712be06d36b6 Mon Sep 17 00:00:00 2001 From: Maciej Kulawik Date: Tue, 15 Oct 2024 22:19:57 +0200 Subject: [PATCH 1/4] compile module only when activating or in targets list --- arbos/programs/native.go | 165 +++++++++++++++++++++--------- arbos/programs/programs.go | 4 +- arbos/programs/wasmstorehelper.go | 3 +- execution/gethexec/node.go | 3 - 4 files changed, 123 insertions(+), 52 deletions(-) diff --git a/arbos/programs/native.go b/arbos/programs/native.go index 725b302ac0..7fc3500e2e 100644 --- a/arbos/programs/native.go +++ b/arbos/programs/native.go @@ -70,7 +70,9 @@ func activateProgram( debug bool, burner burn.Burner, ) (*activationInfo, error) { - info, asmMap, err := activateProgramInternal(db, program, codehash, wasm, page_limit, stylusVersion, arbosVersionForGas, debug, burner.GasLeft()) + targets := db.Database().WasmTargets() + moduleActivationMandatory := true + info, asmMap, err := activateProgramInternal(program, codehash, wasm, page_limit, stylusVersion, arbosVersionForGas, debug, burner.GasLeft(), targets, moduleActivationMandatory) if err != nil { return nil, err } @@ -78,8 +80,7 @@ func activateProgram( return info, nil } -func activateProgramInternal( - db vm.StateDB, +func activateModule( addressForLogging common.Address, codehash common.Hash, wasm []byte, @@ -88,7 +89,7 @@ func activateProgramInternal( arbosVersionForGas uint64, debug bool, gasLeft *uint64, -) (*activationInfo, map[ethdb.WasmTarget][]byte, error) { +) (*activationInfo, []byte, error) { output := &rustBytes{} moduleHash := &bytes32{} stylusData := &C.StylusData{} @@ -106,7 +107,6 @@ func activateProgramInternal( stylusData, (*u64)(gasLeft), )) - module, msg, err := status_mod.toResult(output.intoBytes(), debug) if err != nil { if debug { @@ -114,72 +114,143 @@ func activateProgramInternal( } if errors.Is(err, vm.ErrExecutionReverted) { return nil, nil, fmt.Errorf("%w: %s", ErrProgramActivation, msg) + } else { + return nil, nil, err + } + } + info := &activationInfo{ + moduleHash: moduleHash.toHash(), + initGas: uint16(stylusData.init_cost), + cachedInitGas: uint16(stylusData.cached_init_cost), + asmEstimate: uint32(stylusData.asm_estimate), + footprint: uint16(stylusData.footprint), + } + return info, module, nil +} + +func compileNative( + wasm []byte, + stylusVersion uint16, + debug bool, + target ethdb.WasmTarget, +) ([]byte, error) { + output := &rustBytes{} + status_asm := C.stylus_compile( + goSlice(wasm), + u16(stylusVersion), + cbool(debug), + goSlice([]byte(target)), + output, + ) + asm := output.intoBytes() + if status_asm != 0 { + return nil, fmt.Errorf("%w: %s", ErrProgramActivation, string(asm)) + } + return asm, nil +} + +func activateProgramInternal( + addressForLogging common.Address, + codehash common.Hash, + wasm []byte, + page_limit uint16, + stylusVersion uint16, + arbosVersionForGas uint64, + debug bool, + gasLeft *uint64, + targets []ethdb.WasmTarget, + moduleActivationMandatory bool, +) (*activationInfo, map[ethdb.WasmTarget][]byte, error) { + var wavmFound bool + for _, target := range targets { + if target == rawdb.TargetWavm { + wavmFound = true + break } - return nil, nil, err } - hash := moduleHash.toHash() - targets := db.Database().WasmTargets() type result struct { target ethdb.WasmTarget asm []byte err error } + asmMap := make(map[ethdb.WasmTarget][]byte, len(targets)) + + // info can be set in separate thread, make sure to wait before reading + var info *activationInfo + var moduleActivationStarted bool + if moduleActivationMandatory { + moduleActivationStarted = true + var err error + var module []byte + info, module, err = activateModule(addressForLogging, codehash, wasm, page_limit, stylusVersion, arbosVersionForGas, debug, gasLeft) + if err != nil { + return nil, nil, err + } + if wavmFound { + asmMap[rawdb.TargetWavm] = module + } + } + results := make(chan result, len(targets)) for _, target := range targets { target := target if target == rawdb.TargetWavm { - results <- result{target, module, nil} + if moduleActivationStarted { + // skip if already started or activated because of moduleActivationMandatory + results <- result{target, nil, nil} + continue + } + go func() { + var err error + var module []byte + info, module, err = activateModule(addressForLogging, codehash, wasm, page_limit, stylusVersion, arbosVersionForGas, debug, gasLeft) + results <- result{target, module, err} + }() + moduleActivationStarted = true } else { go func() { - output := &rustBytes{} - status_asm := C.stylus_compile( - goSlice(wasm), - u16(stylusVersion), - cbool(debug), - goSlice([]byte(target)), - output, - ) - asm := output.intoBytes() - if status_asm != 0 { - results <- result{target, nil, fmt.Errorf("%w: %s", ErrProgramActivation, string(asm))} - return - } - results <- result{target, asm, nil} + asm, err := compileNative(wasm, stylusVersion, debug, target) + results <- result{target, asm, err} }() } } - asmMap := make(map[ethdb.WasmTarget][]byte, len(targets)) + var err error for range targets { res := <-results - if res.err != nil { - err = errors.Join(res.err, err) + if res.asm == nil { + continue + } else if res.err != nil { + err = errors.Join(res.err, fmt.Errorf("%s:%w", res.target, err)) } else { asmMap[res.target] = res.asm } } - if err != nil { - log.Error( - "Compilation failed for one or more targets despite activation succeeding", - "address", addressForLogging, - "codeHash", codeHash, - "moduleHash", hash, - "targets", targets, - "err", err, - ) + if err != nil && moduleActivationMandatory { + if info != nil { + log.Error( + "Compilation failed for one or more targets despite activation succeeding", + "address", addressForLogging, + "codehash", codehash, + "moduleHash", info.moduleHash, + "targets", targets, + "err", err, + ) + } else { + log.Error( + "Compilation failed for one or more targets despite activation succeeding", + "address", addressForLogging, + "codehash", codehash, + "targets", targets, + "err", err, + ) + } panic(fmt.Sprintf("Compilation of %v failed for one or more targets despite activation succeeding: %v", addressForLogging, err)) } - info := &activationInfo{ - moduleHash: hash, - initGas: uint16(stylusData.init_cost), - cachedInitGas: uint16(stylusData.cached_init_cost), - asmEstimate: uint32(stylusData.asm_estimate), - footprint: uint16(stylusData.footprint), - } return info, asmMap, err } -func getLocalAsm(statedb vm.StateDB, moduleHash common.Hash, addressForLogging common.Address, code []byte, codeHash common.Hash, pagelimit uint16, time uint64, debugMode bool, program Program) ([]byte, error) { +func getLocalAsm(statedb vm.StateDB, moduleHash common.Hash, addressForLogging common.Address, code []byte, codehash common.Hash, pagelimit uint16, time uint64, debugMode bool, program Program) ([]byte, error) { localTarget := rawdb.LocalTarget() localAsm, err := statedb.TryGetActivatedAsm(localTarget, moduleHash) if err == nil && len(localAsm) > 0 { @@ -197,8 +268,10 @@ func getLocalAsm(statedb vm.StateDB, moduleHash common.Hash, addressForLogging c zeroArbosVersion := uint64(0) zeroGas := uint64(0) + targets := statedb.Database().WasmTargets() // we know program is activated, so it must be in correct version and not use too much memory - info, asmMap, err := activateProgramInternal(statedb, addressForLogging, codeHash, wasm, pagelimit, program.version, zeroArbosVersion, debugMode, &zeroGas) + moduleActivationMandatory := false + info, asmMap, err := activateProgramInternal(addressForLogging, codehash, wasm, pagelimit, program.version, zeroArbosVersion, debugMode, &zeroGas, targets, moduleActivationMandatory) if err != nil { log.Error("failed to reactivate program", "address", addressForLogging, "expected moduleHash", moduleHash, "err", err) return nil, fmt.Errorf("failed to reactivate program address: %v err: %w", addressForLogging, err) @@ -300,10 +373,10 @@ func handleReqImpl(apiId usize, req_type u32, data *rustSlice, costPtr *u64, out // Caches a program in Rust. We write a record so that we can undo on revert. // For gas estimation and eth_call, we ignore permanent updates and rely on Rust's LRU. -func cacheProgram(db vm.StateDB, module common.Hash, program Program, addressForLogging common.Address, code []byte, codeHash common.Hash, params *StylusParams, debug bool, time uint64, runMode core.MessageRunMode) { +func cacheProgram(db vm.StateDB, module common.Hash, program Program, addressForLogging common.Address, code []byte, codehash common.Hash, params *StylusParams, debug bool, time uint64, runMode core.MessageRunMode) { if runMode == core.MessageCommitMode { // address is only used for logging - asm, err := getLocalAsm(db, module, addressForLogging, code, codeHash, params.PageLimit, time, debug, program) + asm, err := getLocalAsm(db, module, addressForLogging, code, codehash, params.PageLimit, time, debug, program) if err != nil { panic("unable to recreate wasm") } diff --git a/arbos/programs/programs.go b/arbos/programs/programs.go index 06ff4137da..5861181bff 100644 --- a/arbos/programs/programs.go +++ b/arbos/programs/programs.go @@ -170,7 +170,7 @@ func (p Programs) CallProgram( tracingInfo *util.TracingInfo, calldata []byte, reentrant bool, - runmode core.MessageRunMode, + runMode core.MessageRunMode, ) ([]byte, error) { evm := interpreter.Evm() contract := scope.Contract @@ -246,7 +246,7 @@ func (p Programs) CallProgram( address = *contract.CodeAddr } var arbos_tag uint32 - if runmode == core.MessageCommitMode { + if runMode == core.MessageCommitMode { arbos_tag = statedb.Database().WasmCacheTag() } ret, err := callProgram(address, moduleHash, localAsm, scope, interpreter, tracingInfo, calldata, evmData, goParams, model, arbos_tag) diff --git a/arbos/programs/wasmstorehelper.go b/arbos/programs/wasmstorehelper.go index c2d1aa65b0..1393752b72 100644 --- a/arbos/programs/wasmstorehelper.go +++ b/arbos/programs/wasmstorehelper.go @@ -62,7 +62,8 @@ func (p Programs) SaveActiveProgramToWasmStore(statedb *state.StateDB, codeHash // We know program is activated, so it must be in correct version and not use too much memory // Empty program address is supplied because we dont have access to this during rebuilding of wasm store - info, asmMap, err := activateProgramInternal(statedb, common.Address{}, codeHash, wasm, progParams.PageLimit, program.version, zeroArbosVersion, debugMode, &zeroGas) + moduleActivationMandatory := false + info, asmMap, err := activateProgramInternal(common.Address{}, codeHash, wasm, progParams.PageLimit, program.version, zeroArbosVersion, debugMode, &zeroGas, targets, moduleActivationMandatory) if err != nil { log.Error("failed to reactivate program while rebuilding wasm store", "expected moduleHash", moduleHash, "err", err) return fmt.Errorf("failed to reactivate program while rebuilding wasm store: %w", err) diff --git a/execution/gethexec/node.go b/execution/gethexec/node.go index 499a13164e..79c5c8896e 100644 --- a/execution/gethexec/node.go +++ b/execution/gethexec/node.go @@ -53,9 +53,6 @@ func (c *StylusTargetConfig) Validate() error { } targetsSet[target] = true } - if !targetsSet[rawdb.TargetWavm] { - return fmt.Errorf("%s target not found in archs list, archs: %v", rawdb.TargetWavm, c.ExtraArchs) - } targetsSet[rawdb.LocalTarget()] = true targets := make([]ethdb.WasmTarget, 0, len(c.ExtraArchs)+1) for target := range targetsSet { From 81155d85bee23c6a0783a7f24bff5bbca67c058a Mon Sep 17 00:00:00 2001 From: Maciej Kulawik Date: Wed, 23 Oct 2024 16:44:58 +0200 Subject: [PATCH 2/4] system_tests: fix checkWasmStoreContent helper --- system_tests/common_test.go | 4 +++- system_tests/program_test.go | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/system_tests/common_test.go b/system_tests/common_test.go index 027a41d875..21eaf89cd7 100644 --- a/system_tests/common_test.go +++ b/system_tests/common_test.go @@ -1322,6 +1322,7 @@ func createNonL1BlockChainWithStackConfig( if execConfig == nil { execConfig = ExecConfigDefaultTest(t) } + Require(t, execConfig.Validate()) stack, err := node.New(stackConfig) Require(t, err) @@ -1409,6 +1410,8 @@ func Create2ndNodeWithConfig( if execConfig == nil { execConfig = ExecConfigDefaultNonSequencerTest(t) } + Require(t, execConfig.Validate()) + feedErrChan := make(chan error, 10) parentChainRpcClient := parentChainStack.Attach() parentChainClient := ethclient.NewClient(parentChainRpcClient) @@ -1442,7 +1445,6 @@ func Create2ndNodeWithConfig( AddValNodeIfNeeded(t, ctx, nodeConfig, true, "", valnodeConfig.Wasm.RootPath) - Require(t, execConfig.Validate()) Require(t, nodeConfig.Validate()) configFetcher := func() *gethexec.Config { return execConfig } currentExec, err := gethexec.CreateExecutionNode(ctx, chainStack, chainDb, blockchain, parentChainClient, configFetcher) diff --git a/system_tests/program_test.go b/system_tests/program_test.go index 4c896d1791..156d9d3638 100644 --- a/system_tests/program_test.go +++ b/system_tests/program_test.go @@ -1991,6 +1991,7 @@ func readModuleHashes(t *testing.T, wasmDb ethdb.KeyValueStore) []common.Hash { } func checkWasmStoreContent(t *testing.T, wasmDb ethdb.KeyValueStore, targets []string, numModules int) { + t.Helper() modules := readModuleHashes(t, wasmDb) if len(modules) != numModules { t.Fatalf("Unexpected number of module hashes found in wasm store, want: %d, have: %d", numModules, len(modules)) @@ -2002,12 +2003,16 @@ func checkWasmStoreContent(t *testing.T, wasmDb ethdb.KeyValueStore, targets []s t.Fatalf("internal test error - unsupported target passed to checkWasmStoreContent: %v", target) } func() { + t.Helper() defer func() { if r := recover(); r != nil { t.Fatalf("Failed to read activated asm for target: %v, module: %v", target, module) } }() - _ = rawdb.ReadActivatedAsm(wasmDb, wasmTarget, module) + asm := rawdb.ReadActivatedAsm(wasmDb, wasmTarget, module) + if len(asm) == 0 { + t.Fatalf("Missing activated asm for target: %v, module: %v", target, module) + } }() } } From 8914c571031897357d203632cab0b0f558b65901 Mon Sep 17 00:00:00 2001 From: Maciej Kulawik Date: Wed, 23 Oct 2024 19:32:40 +0200 Subject: [PATCH 3/4] run some tests with single wasm target --- system_tests/program_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/system_tests/program_test.go b/system_tests/program_test.go index 156d9d3638..342807cbe2 100644 --- a/system_tests/program_test.go +++ b/system_tests/program_test.go @@ -58,6 +58,12 @@ func TestProgramKeccak(t *testing.T) { builder.WithExtraArchs(allWasmTargets) }) }) + + t.Run("WithOnlyLocalTarget", func(t *testing.T) { + keccakTest(t, true, func(builder *NodeBuilder) { + builder.WithExtraArchs([]string{string(rawdb.LocalTarget())}) + }) + }) } func keccakTest(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder)) { @@ -163,6 +169,11 @@ func TestProgramActivateTwice(t *testing.T) { builder.WithExtraArchs(allWasmTargets) }) }) + t.Run("WithOnlyLocalTarget", func(t *testing.T) { + testActivateTwice(t, true, func(builder *NodeBuilder) { + builder.WithExtraArchs([]string{string(rawdb.LocalTarget())}) + }) + }) } func testActivateTwice(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder)) { From 1abc820181760f4b0ceddc4b16deaf27d6744756 Mon Sep 17 00:00:00 2001 From: Maciej Kulawik Date: Wed, 23 Oct 2024 21:54:35 +0200 Subject: [PATCH 4/4] system_tests: check for unexpected extra asm in checkWasmStoreContent --- system_tests/program_test.go | 63 ++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/system_tests/program_test.go b/system_tests/program_test.go index 342807cbe2..65710a86a8 100644 --- a/system_tests/program_test.go +++ b/system_tests/program_test.go @@ -74,7 +74,7 @@ func keccakTest(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder)) { programAddress := deployWasm(t, ctx, auth, l2client, rustFile("keccak")) wasmDb := builder.L2.ExecNode.Backend.ArbInterface().BlockChain().StateCache().WasmStore() - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 1) wasm, _ := readWasmFile(t, rustFile("keccak")) otherAddressSameCode := deployContract(t, ctx, auth, l2client, wasm) @@ -87,7 +87,7 @@ func keccakTest(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder)) { Fatal(t, "activate should have failed with ProgramUpToDate", err) } }) - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 1) if programAddress == otherAddressSameCode { Fatal(t, "expected to deploy at two separate program addresses") @@ -205,7 +205,7 @@ func testActivateTwice(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder) multiAddr := deployWasm(t, ctx, auth, l2client, rustFile("multicall")) wasmDb := builder.L2.ExecNode.Backend.ArbInterface().BlockChain().StateCache().WasmStore() - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 1) preimage := []byte("it's time to du-du-du-du d-d-d-d-d-d-d de-duplicate") @@ -230,7 +230,7 @@ func testActivateTwice(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder) // Calling the contract pre-activation should fail. checkReverts() - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 1) // mechanisms for creating calldata activateProgram, _ := util.NewCallParser(pgen.ArbWasmABI, "activateProgram") @@ -253,7 +253,7 @@ func testActivateTwice(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder) // Ensure the revert also reverted keccak's activation checkReverts() - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 1) // Activate keccak program A, then call into B, which should succeed due to being the same codehash args = argsForMulticall(vm.CALL, types.ArbWasmAddress, oneEth, pack(activateProgram(keccakA))) @@ -261,7 +261,7 @@ func testActivateTwice(t *testing.T, jit bool, builderOpts ...func(*NodeBuilder) tx = l2info.PrepareTxTo("Owner", &multiAddr, 1e9, oneEth, args) ensure(tx, l2client.SendTransaction(ctx, tx)) - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 2) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 2) validateBlocks(t, 7, jit, builder) } @@ -1917,7 +1917,7 @@ func TestWasmStoreRebuilding(t *testing.T) { storeMap, err := createMapFromDb(wasmDb) Require(t, err) - checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDb, builder.execConfig.StylusTarget.WasmTargets(), 1) // close nodeB cleanupB() @@ -1974,7 +1974,7 @@ func TestWasmStoreRebuilding(t *testing.T) { } } - checkWasmStoreContent(t, wasmDbAfterRebuild, builder.execConfig.StylusTarget.ExtraArchs, 1) + checkWasmStoreContent(t, wasmDbAfterRebuild, builder.execConfig.StylusTarget.WasmTargets(), 1) cleanupB() } @@ -2001,30 +2001,43 @@ func readModuleHashes(t *testing.T, wasmDb ethdb.KeyValueStore) []common.Hash { return modules } -func checkWasmStoreContent(t *testing.T, wasmDb ethdb.KeyValueStore, targets []string, numModules int) { +func checkWasmStoreContent(t *testing.T, wasmDb ethdb.KeyValueStore, expectedTargets []ethdb.WasmTarget, numModules int) { t.Helper() modules := readModuleHashes(t, wasmDb) if len(modules) != numModules { t.Fatalf("Unexpected number of module hashes found in wasm store, want: %d, have: %d", numModules, len(modules)) } - for _, module := range modules { - for _, target := range targets { - wasmTarget := ethdb.WasmTarget(target) - if !rawdb.IsSupportedWasmTarget(wasmTarget) { - t.Fatalf("internal test error - unsupported target passed to checkWasmStoreContent: %v", target) - } - func() { - t.Helper() - defer func() { - if r := recover(); r != nil { - t.Fatalf("Failed to read activated asm for target: %v, module: %v", target, module) - } - }() - asm := rawdb.ReadActivatedAsm(wasmDb, wasmTarget, module) - if len(asm) == 0 { - t.Fatalf("Missing activated asm for target: %v, module: %v", target, module) + readAsm := func(module common.Hash, target string) []byte { + wasmTarget := ethdb.WasmTarget(target) + if !rawdb.IsSupportedWasmTarget(wasmTarget) { + t.Fatalf("internal test error - unsupported target passed to checkWasmStoreContent: %v", target) + } + return func() []byte { + t.Helper() + defer func() { + if r := recover(); r != nil { + t.Fatalf("Failed to read activated asm for target: %v, module: %v", target, module) } }() + return rawdb.ReadActivatedAsm(wasmDb, wasmTarget, module) + }() + } + for _, module := range modules { + for _, target := range allWasmTargets { + var expected bool + for _, expectedTarget := range expectedTargets { + if ethdb.WasmTarget(target) == expectedTarget { + expected = true + break + } + } + asm := readAsm(module, target) + if expected && len(asm) == 0 { + t.Fatalf("Missing asm for target: %v, module: %v", target, module) + } + if !expected && len(asm) > 0 { + t.Fatalf("Found asm for target: %v, module: %v, expected targets: %v", target, module, expectedTargets) + } } } }