From 419bd7bb9d5c4f0960034a1533fe1a65752d461b Mon Sep 17 00:00:00 2001 From: Joshua Colvin Date: Mon, 20 Nov 2023 17:56:46 -0700 Subject: [PATCH 1/2] don't re-request parent block if same block as previous block `logsToDeliveredMessages` was requesting parent block for every single parsedLog even if the previous parsedLog had the same block number, causing lots of duplicate requests. --- arbnode/delayed.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/arbnode/delayed.go b/arbnode/delayed.go index f2c3d6200..51b22c58b 100644 --- a/arbnode/delayed.go +++ b/arbnode/delayed.go @@ -184,6 +184,8 @@ func (b *DelayedBridge) logsToDeliveredMessages(ctx context.Context, logs []type } messages := make([]*DelayedInboxMessage, 0, len(logs)) + var lastParentChainBlockNumber uint64 + var lastL1BlockNumber uint64 for _, parsedLog := range parsedLogs { msgKey := common.BigToHash(parsedLog.MessageIndex) data, ok := messageData[msgKey] @@ -196,9 +198,17 @@ func (b *DelayedBridge) logsToDeliveredMessages(ctx context.Context, logs []type requestId := common.BigToHash(parsedLog.MessageIndex) parentChainBlockNumber := parsedLog.Raw.BlockNumber - l1BlockNumber, err := arbutil.CorrespondingL1BlockNumber(ctx, b.client, parentChainBlockNumber) - if err != nil { - return nil, err + var l1BlockNumber uint64 + if lastParentChainBlockNumber == parentChainBlockNumber && lastParentChainBlockNumber > 0 { + l1BlockNumber = lastL1BlockNumber + } else { + var err error + l1BlockNumber, err = arbutil.CorrespondingL1BlockNumber(ctx, b.client, parentChainBlockNumber) + if err != nil { + return nil, err + } + lastParentChainBlockNumber = parentChainBlockNumber + lastL1BlockNumber = l1BlockNumber } msg := &DelayedInboxMessage{ BlockHash: parsedLog.Raw.BlockHash, @@ -216,7 +226,7 @@ func (b *DelayedBridge) logsToDeliveredMessages(ctx context.Context, logs []type }, ParentChainBlockNumber: parsedLog.Raw.BlockNumber, } - err = msg.Message.FillInBatchGasCost(batchFetcher) + err := msg.Message.FillInBatchGasCost(batchFetcher) if err != nil { return nil, err } From 1d77a372fa05c55f9a39616765653d61e2c1e607 Mon Sep 17 00:00:00 2001 From: Rachel Bousfield Date: Wed, 6 Dec 2023 18:10:14 -0700 Subject: [PATCH 2/2] fix program stack --- arbitrator/Cargo.lock | 10 +-- .../wasm-libraries/user-host/src/host.rs | 2 +- .../wasm-libraries/user-host/src/ink.rs | 2 +- .../wasm-libraries/user-host/src/lib.rs | 49 +------------ .../wasm-libraries/user-host/src/link.rs | 6 +- .../wasm-libraries/user-host/src/program.rs | 70 +++++++++++++++++++ arbos/programs/native.go | 3 +- system_tests/program_test.go | 12 +++- 8 files changed, 94 insertions(+), 60 deletions(-) create mode 100644 arbitrator/wasm-libraries/user-host/src/program.rs diff --git a/arbitrator/Cargo.lock b/arbitrator/Cargo.lock index 2ec646d1a..5518dc36d 100644 --- a/arbitrator/Cargo.lock +++ b/arbitrator/Cargo.lock @@ -482,7 +482,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "978747c1d849a7d2ee5e8adc0159961c48fb7e5db2f06af6723b80123bb53856" dependencies = [ "cfg-if", - "hashbrown 0.14.2", + "hashbrown 0.14.3", "lock_api", "once_cell", "parking_lot_core", @@ -692,9 +692,9 @@ dependencies = [ [[package]] name = "hashbrown" -version = "0.14.2" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f93e7192158dbcda357bdec5fb5788eebf8bbac027f3f33e719d29135ae84156" +checksum = "290f1a1d9242c78d09ce40a5e87e7554ee637af1351968159f4952f028f75604" [[package]] name = "heck" @@ -852,9 +852,9 @@ checksum = "f92be4933c13fd498862a9e02a3055f8a8d9c039ce33db97306fd5a6caa7f29b" [[package]] name = "llvm-sys" -version = "150.1.2" +version = "150.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "417dbaef2fece3b186fe15704e010849279de5f7eea1caa8845558130867bdd2" +checksum = "bfd60e740af945d99c2446a52e3ab8cdba2f740a40a16c51f6871bdea2abc687" dependencies = [ "cc", "lazy_static", diff --git a/arbitrator/wasm-libraries/user-host/src/host.rs b/arbitrator/wasm-libraries/user-host/src/host.rs index 7faf28c70..63dfb4700 100644 --- a/arbitrator/wasm-libraries/user-host/src/host.rs +++ b/arbitrator/wasm-libraries/user-host/src/host.rs @@ -1,7 +1,7 @@ // Copyright 2022, Offchain Labs, Inc. // For license information, see https://github.com/nitro/blob/master/LICENSE -use crate::{evm_api::ApiCaller, Program}; +use crate::{evm_api::ApiCaller, program::Program}; use arbutil::{ crypto, evm::{self, api::EvmApi, js::JsEvmApi, user::UserOutcomeKind}, diff --git a/arbitrator/wasm-libraries/user-host/src/ink.rs b/arbitrator/wasm-libraries/user-host/src/ink.rs index 6f38c93fe..e01e616e0 100644 --- a/arbitrator/wasm-libraries/user-host/src/ink.rs +++ b/arbitrator/wasm-libraries/user-host/src/ink.rs @@ -1,7 +1,7 @@ // Copyright 2022-2023, Offchain Labs, Inc. // For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE -use crate::Program; +use crate::program::Program; use prover::programs::{ config::PricingParams, prelude::{GasMeteredMachine, MachineMeter, MeteredMachine}, diff --git a/arbitrator/wasm-libraries/user-host/src/lib.rs b/arbitrator/wasm-libraries/user-host/src/lib.rs index cdd21203c..a8920a07e 100644 --- a/arbitrator/wasm-libraries/user-host/src/lib.rs +++ b/arbitrator/wasm-libraries/user-host/src/lib.rs @@ -1,55 +1,8 @@ // Copyright 2022-2023, Offchain Labs, Inc. // For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE -use arbutil::{ - evm::{js::JsEvmApi, EvmData}, - pricing, -}; -use evm_api::ApiCaller; -use prover::programs::{meter::MeteredMachine, prelude::StylusConfig}; - mod evm_api; mod host; mod ink; mod link; - -pub(crate) static mut PROGRAMS: Vec = vec![]; - -pub(crate) struct Program { - args: Vec, - outs: Vec, - evm_api: JsEvmApi, - evm_data: EvmData, - config: StylusConfig, -} - -impl Program { - pub fn new( - args: Vec, - evm_api: JsEvmApi, - evm_data: EvmData, - config: StylusConfig, - ) -> Self { - Self { - args, - outs: vec![], - evm_api, - evm_data, - config, - } - } - - pub fn into_outs(self) -> Vec { - self.outs - } - - pub fn start(cost: u64) -> &'static mut Self { - let program = Self::start_free(); - program.buy_ink(pricing::HOSTIO_INK + cost).unwrap(); - program - } - - pub fn start_free() -> &'static mut Self { - unsafe { PROGRAMS.last_mut().expect("no program") } - } -} +mod program; diff --git a/arbitrator/wasm-libraries/user-host/src/link.rs b/arbitrator/wasm-libraries/user-host/src/link.rs index 320811b58..9ce459fff 100644 --- a/arbitrator/wasm-libraries/user-host/src/link.rs +++ b/arbitrator/wasm-libraries/user-host/src/link.rs @@ -1,7 +1,7 @@ // Copyright 2022-2023, Offchain Labs, Inc. // For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE -use crate::{evm_api::ApiCaller, Program, PROGRAMS}; +use crate::{evm_api::ApiCaller, program::Program}; use arbutil::{ evm::{js::JsEvmApi, user::UserOutcomeKind, EvmData}, format::DebugBytes, @@ -119,11 +119,11 @@ pub unsafe extern "C" fn go__github_com_offchainlabs_nitro_arbos_programs_callPr // provide arguments let args_len = calldata.len(); - PROGRAMS.push(Program::new(calldata, evm_api, evm_data, config)); + Program::push_new(calldata, evm_api, evm_data, config); // call the program let status = program_call_main(module, args_len); - let outs = PROGRAMS.pop().unwrap().into_outs(); + let outs = Program::pop(); sp.restore_stack(); // restore the stack pointer (corrupts during EVM API calls) /// cleans up and writes the output diff --git a/arbitrator/wasm-libraries/user-host/src/program.rs b/arbitrator/wasm-libraries/user-host/src/program.rs new file mode 100644 index 000000000..453b29b23 --- /dev/null +++ b/arbitrator/wasm-libraries/user-host/src/program.rs @@ -0,0 +1,70 @@ +// Copyright 2022-2023, Offchain Labs, Inc. +// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE + +use crate::evm_api::ApiCaller; +use arbutil::{ + evm::{js::JsEvmApi, EvmData}, + pricing, +}; +use prover::programs::{meter::MeteredMachine, prelude::StylusConfig}; + +/// The list of active programs. The current program is always the last. +/// +/// Note that this data-structure may re-alloc while references to [`Program`] are held. +/// This is sound due to [`Box`] providing a level of indirection. +/// +/// Normal Rust rules would suggest using a [`Vec`] of cells would be better. The issue is that, +/// should an error guard recover, this WASM will reset to an earlier state but with the current +/// memory. This means that stack unwinding won't happen, rendering these primitives unhelpful. +#[allow(clippy::vec_box)] +static mut PROGRAMS: Vec> = vec![]; + +/// An active user program. +pub(crate) struct Program { + /// Arguments passed via the VM. + pub args: Vec, + /// Output generated by the program. + pub outs: Vec, + /// Mechanism for calling back into Geth. + pub evm_api: JsEvmApi, + /// EVM Context info. + pub evm_data: EvmData, + /// Call configuration. + pub config: StylusConfig, +} + +impl Program { + /// Adds a new program, making it current. + pub fn push_new( + args: Vec, + evm_api: JsEvmApi, + evm_data: EvmData, + config: StylusConfig, + ) { + let program = Self { + args, + outs: vec![], + evm_api, + evm_data, + config, + }; + unsafe { PROGRAMS.push(Box::new(program)) } + } + + /// Removes the current program, returning its output. + pub fn pop() -> Vec { + unsafe { PROGRAMS.pop().expect("no program").outs } + } + + /// Provides a reference to the current program after paying some ink. + pub fn start(cost: u64) -> &'static mut Self { + let program = Self::start_free(); + program.buy_ink(pricing::HOSTIO_INK + cost).unwrap(); + program + } + + /// Provides a reference to the current program. + pub fn start_free() -> &'static mut Self { + unsafe { PROGRAMS.last_mut().expect("no program") } + } +} diff --git a/arbos/programs/native.go b/arbos/programs/native.go index f99ae8f4b..cb6cab0d0 100644 --- a/arbos/programs/native.go +++ b/arbos/programs/native.go @@ -122,10 +122,11 @@ func callProgram( (*u64)(&scope.Contract.Gas), )) + depth := interpreter.Depth() debug := stylusParams.debugMode != 0 data, msg, err := status.toResult(output.intoBytes(), debug) if status == userFailure && debug { - log.Warn("program failure", "err", err, "msg", msg, "program", address) + log.Warn("program failure", "err", err, "msg", msg, "program", address, "depth", depth) } return data, err } diff --git a/system_tests/program_test.go b/system_tests/program_test.go index 4489ca190..d2d7a3200 100644 --- a/system_tests/program_test.go +++ b/system_tests/program_test.go @@ -230,6 +230,7 @@ func errorTest(t *testing.T, jit bool) { defer cleanup() programAddress := deployWasm(t, ctx, auth, l2client, rustFile("fallible")) + multiAddr := deployWasm(t, ctx, auth, l2client, rustFile("multicall")) // ensure tx passes tx := l2info.PrepareTxTo("Owner", &programAddress, l2info.TransferGas, nil, []byte{0x01}) @@ -246,7 +247,16 @@ func errorTest(t *testing.T, jit bool) { Fatal(t, "call should have failed") } - validateBlocks(t, 6, jit, ctx, node, l2client) + // ensure tx recovery is correct after failing in a deeply nested call + args := []byte{} + for i := 0; i < 32; i++ { + args = argsForMulticall(vm.CALL, multiAddr, nil, args) + } + tx = l2info.PrepareTxTo("Owner", &multiAddr, 1e9, nil, args) + Require(t, l2client.SendTransaction(ctx, tx)) + EnsureTxFailed(t, ctx, l2client, tx) + + validateBlocks(t, 7, jit, ctx, node, l2client) } func TestProgramStorage(t *testing.T) {