From de3b6a9305b4cacca9a915e48ea0b8c4b874ef93 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Tue, 29 Aug 2023 15:06:24 -0600 Subject: [PATCH 01/10] Fix clippy warning about an Arc not being Send+Sync --- arbitrator/prover/src/utils.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arbitrator/prover/src/utils.rs b/arbitrator/prover/src/utils.rs index 6c11e9af05..efd94dcd7c 100644 --- a/arbitrator/prover/src/utils.rs +++ b/arbitrator/prover/src/utils.rs @@ -158,6 +158,10 @@ impl From<&[u8]> for CBytes { } } +// There's no thread safety concerns for CBytes +unsafe impl Send for CBytes {} +unsafe impl Sync for CBytes {} + #[derive(Serialize, Deserialize)] #[serde(remote = "Type")] enum RemoteType { From 9227df788929352184e0bd792898f15beabac0b9 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Tue, 29 Aug 2023 15:32:04 -0600 Subject: [PATCH 02/10] Fix formatting --- arbitrator/jit/src/syscall.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arbitrator/jit/src/syscall.rs b/arbitrator/jit/src/syscall.rs index 4cd0363b49..c81641a7f8 100644 --- a/arbitrator/jit/src/syscall.rs +++ b/arbitrator/jit/src/syscall.rs @@ -306,10 +306,10 @@ pub fn js_value_index(mut env: WasmEnvMut, sp: u32) { pub fn js_value_call(mut env: WasmEnvMut, sp: u32) -> MaybeEscape { let Some(resume) = env.data().exports.resume.clone() else { - return Escape::failure(format!("wasmer failed to bind {}", "resume".red())) + return Escape::failure(format!("wasmer failed to bind {}", "resume".red())); }; let Some(get_stack_pointer) = env.data().exports.get_stack_pointer.clone() else { - return Escape::failure(format!("wasmer failed to bind {}", "getsp".red())) + return Escape::failure(format!("wasmer failed to bind {}", "getsp".red())); }; let sp = GoStack::simple(sp, &env); let data = env.data_mut(); From ad64d967f68b84b1e8ed29a3d6907bd639b8cb16 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Wed, 30 Aug 2023 09:37:29 -0600 Subject: [PATCH 03/10] Make PreimageResolver Send+Sync --- arbitrator/prover/src/machine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arbitrator/prover/src/machine.rs b/arbitrator/prover/src/machine.rs index fff9c0f3d8..d5a9c52d92 100644 --- a/arbitrator/prover/src/machine.rs +++ b/arbitrator/prover/src/machine.rs @@ -651,7 +651,7 @@ pub struct MachineState<'a> { initial_hash: Bytes32, } -pub type PreimageResolver = Arc Option>; +pub type PreimageResolver = Arc Option + Send + Sync>; /// Wraps a preimage resolver to provide an easier API /// and cache the last preimage retrieved. From 283b5d71a1d8eff096e878d8f028cf33356585e9 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Wed, 30 Aug 2023 09:53:11 -0600 Subject: [PATCH 04/10] Extend comment explaining why CBytes is Send+Sync --- arbitrator/prover/src/utils.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arbitrator/prover/src/utils.rs b/arbitrator/prover/src/utils.rs index efd94dcd7c..e86ea96768 100644 --- a/arbitrator/prover/src/utils.rs +++ b/arbitrator/prover/src/utils.rs @@ -158,7 +158,10 @@ impl From<&[u8]> for CBytes { } } -// There's no thread safety concerns for CBytes +// There's no thread safety concerns for CBytes. +// This type is basically a Box<[u8]> (which is Send + Sync) with libc as an allocator. +// Any data races between threads are prevented by Rust borrowing rules, +// and the data isn't thread-local so there's no concern moving it between threads. unsafe impl Send for CBytes {} unsafe impl Sync for CBytes {} From 06930b67bf24630f002abbe523c2ae9e3fd71362 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Wed, 30 Aug 2023 12:56:15 -0600 Subject: [PATCH 05/10] Fix pollForReverts channel reading --- arbnode/batch_poster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 429701be7e..7efa2b3f73 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -309,8 +309,8 @@ func (b *BatchPoster) pollForReverts(ctx context.Context) { // - polling is through context, or // - we see a transaction in the block from dataposter that was reverted. select { - case h, closed := <-headerCh: - if closed { + case h, ok := <-headerCh: + if !ok { log.Info("L1 headers channel has been closed") return } From 370ac231089a56649372e7bbe21ce3c26cefda80 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Wed, 30 Aug 2023 20:43:12 -0600 Subject: [PATCH 06/10] Fix off by 1 in validator logging --- staker/block_validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staker/block_validator.go b/staker/block_validator.go index f04b852041..94bc2a0806 100644 --- a/staker/block_validator.go +++ b/staker/block_validator.go @@ -597,7 +597,7 @@ func (v *BlockValidator) iterativeValidationPrint(ctx context.Context) time.Dura var batchMsgs arbutil.MessageIndex var printedCount int64 if validated.GlobalState.Batch > 0 { - batchMsgs, err = v.inboxTracker.GetBatchMessageCount(validated.GlobalState.Batch) + batchMsgs, err = v.inboxTracker.GetBatchMessageCount(validated.GlobalState.Batch - 1) } if err != nil { printedCount = -1 From 8cb2606040a993d7fd1f6cf08e5af842ff1b2d1d Mon Sep 17 00:00:00 2001 From: ganeshvanahalli Date: Thu, 31 Aug 2023 10:49:42 -0500 Subject: [PATCH 07/10] add new type Uint64OrHex --- go-ethereum | 2 +- system_tests/conditionaltx_test.go | 29 ++++++++++++++--------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/go-ethereum b/go-ethereum index c905292f8a..f8363dc42d 160000 --- a/go-ethereum +++ b/go-ethereum @@ -1 +1 @@ -Subproject commit c905292f8af601f7fca261e65a7d4bc144261e29 +Subproject commit f8363dc42d6cf8dbd76aed2550207a90919e88ee diff --git a/system_tests/conditionaltx_test.go b/system_tests/conditionaltx_test.go index c65103694a..b758ba1807 100644 --- a/system_tests/conditionaltx_test.go +++ b/system_tests/conditionaltx_test.go @@ -16,7 +16,6 @@ import ( "github.com/ethereum/go-ethereum/arbitrum" "github.com/ethereum/go-ethereum/arbitrum_types" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/rpc" @@ -103,23 +102,23 @@ func getOptions(address common.Address, rootHash common.Hash, slotValueMap map[c } func getFulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := hexutil.Uint64(timestamp + 30) - past := hexutil.Uint64(timestamp - 1) - futureBlockNumber := hexutil.Uint64(blockNumber + 1000) - currentBlockNumber := hexutil.Uint64(blockNumber) + future := common.Uint64OrHex(timestamp + 30) + past := common.Uint64OrHex(timestamp - 1) + futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) + currentBlockNumber := common.Uint64OrHex(blockNumber) return getBlockTimeLimits(t, currentBlockNumber, futureBlockNumber, past, future) } func getUnfulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := hexutil.Uint64(timestamp + 30) - past := hexutil.Uint64(timestamp - 1) - futureBlockNumber := hexutil.Uint64(blockNumber + 1000) - previousBlockNumber := hexutil.Uint64(blockNumber - 1) + future := common.Uint64OrHex(timestamp + 30) + past := common.Uint64OrHex(timestamp - 1) + futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) + previousBlockNumber := common.Uint64OrHex(blockNumber - 1) // skip first empty options return getBlockTimeLimits(t, futureBlockNumber, previousBlockNumber, future, past)[1:] } -func getBlockTimeLimits(t *testing.T, blockMin, blockMax hexutil.Uint64, timeMin, timeMax hexutil.Uint64) []*arbitrum_types.ConditionalOptions { +func getBlockTimeLimits(t *testing.T, blockMin, blockMax common.Uint64OrHex, timeMin, timeMax common.Uint64OrHex) []*arbitrum_types.ConditionalOptions { basic := []*arbitrum_types.ConditionalOptions{ {}, {TimestampMin: &timeMin}, @@ -157,9 +156,9 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* c.KnownAccounts[k] = v } limitTriples := []struct { - a *hexutil.Uint64 - b *hexutil.Uint64 - c **hexutil.Uint64 + a *common.Uint64OrHex + b *common.Uint64OrHex + c **common.Uint64OrHex }{ {a.BlockNumberMin, b.BlockNumberMin, &c.BlockNumberMin}, {a.BlockNumberMax, b.BlockNumberMax, &c.BlockNumberMax}, @@ -168,10 +167,10 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* } for _, tripple := range limitTriples { if tripple.b != nil { - value := hexutil.Uint64(*tripple.b) + value := common.Uint64OrHex(*tripple.b) *tripple.c = &value } else if tripple.a != nil { - value := hexutil.Uint64(*tripple.a) + value := common.Uint64OrHex(*tripple.a) *tripple.c = &value } else { *tripple.c = nil From 8f3f8162b88f22e3ef56e3ebbc6ba627b10b8ecb Mon Sep 17 00:00:00 2001 From: ganeshvanahalli Date: Thu, 31 Aug 2023 12:59:35 -0500 Subject: [PATCH 08/10] reuse type math.HexOrDecimal64 --- go-ethereum | 2 +- system_tests/conditionaltx_test.go | 29 +++++++++++++++-------------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/go-ethereum b/go-ethereum index f8363dc42d..b4bd0da114 160000 --- a/go-ethereum +++ b/go-ethereum @@ -1 +1 @@ -Subproject commit f8363dc42d6cf8dbd76aed2550207a90919e88ee +Subproject commit b4bd0da1142fe6bb81cac7e0794ebb4746b9885a diff --git a/system_tests/conditionaltx_test.go b/system_tests/conditionaltx_test.go index b758ba1807..14aa000313 100644 --- a/system_tests/conditionaltx_test.go +++ b/system_tests/conditionaltx_test.go @@ -16,6 +16,7 @@ import ( "github.com/ethereum/go-ethereum/arbitrum" "github.com/ethereum/go-ethereum/arbitrum_types" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/math" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/rpc" @@ -102,23 +103,23 @@ func getOptions(address common.Address, rootHash common.Hash, slotValueMap map[c } func getFulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := common.Uint64OrHex(timestamp + 30) - past := common.Uint64OrHex(timestamp - 1) - futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) - currentBlockNumber := common.Uint64OrHex(blockNumber) + future := math.HexOrDecimal64(timestamp + 30) + past := math.HexOrDecimal64(timestamp - 1) + futureBlockNumber := math.HexOrDecimal64(blockNumber + 1000) + currentBlockNumber := math.HexOrDecimal64(blockNumber) return getBlockTimeLimits(t, currentBlockNumber, futureBlockNumber, past, future) } func getUnfulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := common.Uint64OrHex(timestamp + 30) - past := common.Uint64OrHex(timestamp - 1) - futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) - previousBlockNumber := common.Uint64OrHex(blockNumber - 1) + future := math.HexOrDecimal64(timestamp + 30) + past := math.HexOrDecimal64(timestamp - 1) + futureBlockNumber := math.HexOrDecimal64(blockNumber + 1000) + previousBlockNumber := math.HexOrDecimal64(blockNumber - 1) // skip first empty options return getBlockTimeLimits(t, futureBlockNumber, previousBlockNumber, future, past)[1:] } -func getBlockTimeLimits(t *testing.T, blockMin, blockMax common.Uint64OrHex, timeMin, timeMax common.Uint64OrHex) []*arbitrum_types.ConditionalOptions { +func getBlockTimeLimits(t *testing.T, blockMin, blockMax math.HexOrDecimal64, timeMin, timeMax math.HexOrDecimal64) []*arbitrum_types.ConditionalOptions { basic := []*arbitrum_types.ConditionalOptions{ {}, {TimestampMin: &timeMin}, @@ -156,9 +157,9 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* c.KnownAccounts[k] = v } limitTriples := []struct { - a *common.Uint64OrHex - b *common.Uint64OrHex - c **common.Uint64OrHex + a *math.HexOrDecimal64 + b *math.HexOrDecimal64 + c **math.HexOrDecimal64 }{ {a.BlockNumberMin, b.BlockNumberMin, &c.BlockNumberMin}, {a.BlockNumberMax, b.BlockNumberMax, &c.BlockNumberMax}, @@ -167,10 +168,10 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* } for _, tripple := range limitTriples { if tripple.b != nil { - value := common.Uint64OrHex(*tripple.b) + value := math.HexOrDecimal64(*tripple.b) *tripple.c = &value } else if tripple.a != nil { - value := common.Uint64OrHex(*tripple.a) + value := math.HexOrDecimal64(*tripple.a) *tripple.c = &value } else { *tripple.c = nil From d23602ffc8215367d7705c0e4867cb357949ba84 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Thu, 31 Aug 2023 19:42:15 -0600 Subject: [PATCH 09/10] Fix batch poster fixAccErr not resetting when the error stops --- arbnode/batch_poster.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 429701be7e..53cd5d5594 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -970,10 +970,14 @@ func (b *BatchPoster) Start(ctxIn context.Context) { return b.config().PollInterval } posted, err := b.maybePostSequencerBatch(ctx) + ephemeralError := errors.Is(err, AccumulatorNotFoundErr) || errors.Is(err, storage.ErrStorageRace) + if !ephemeralError { + b.firstAccErr = time.Time{} + } if err != nil { b.building = nil logLevel := log.Error - if errors.Is(err, AccumulatorNotFoundErr) || errors.Is(err, storage.ErrStorageRace) { + if ephemeralError { // Likely the inbox tracker just isn't caught up. // Let's see if this error disappears naturally. if b.firstAccErr == (time.Time{}) { @@ -982,8 +986,6 @@ func (b *BatchPoster) Start(ctxIn context.Context) { } else if time.Since(b.firstAccErr) < time.Minute { logLevel = log.Debug } - } else { - b.firstAccErr = time.Time{} } logLevel("error posting batch", "err", err) return b.config().ErrorDelay From 053e13a4d81da7e417be72c70f023bf86aa11189 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Thu, 31 Aug 2023 19:47:50 -0600 Subject: [PATCH 10/10] Rename firstAccErr to firstEphemeralError --- arbnode/batch_poster.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 53cd5d5594..43cf97f2ae 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -56,20 +56,20 @@ type batchPosterPosition struct { type BatchPoster struct { stopwaiter.StopWaiter - l1Reader *headerreader.HeaderReader - inbox *InboxTracker - streamer *TransactionStreamer - config BatchPosterConfigFetcher - seqInbox *bridgegen.SequencerInbox - bridge *bridgegen.Bridge - syncMonitor *SyncMonitor - seqInboxABI *abi.ABI - seqInboxAddr common.Address - building *buildingBatch - daWriter das.DataAvailabilityServiceWriter - dataPoster *dataposter.DataPoster - redisLock *redislock.Simple - firstAccErr time.Time // first time a continuous missing accumulator occurred + l1Reader *headerreader.HeaderReader + inbox *InboxTracker + streamer *TransactionStreamer + config BatchPosterConfigFetcher + seqInbox *bridgegen.SequencerInbox + bridge *bridgegen.Bridge + syncMonitor *SyncMonitor + seqInboxABI *abi.ABI + seqInboxAddr common.Address + building *buildingBatch + daWriter das.DataAvailabilityServiceWriter + dataPoster *dataposter.DataPoster + redisLock *redislock.Simple + firstEphemeralError time.Time // first time a continuous error suspected to be ephemeral occurred // An estimate of the number of batches we want to post but haven't yet. // This doesn't include batches which we don't want to post yet due to the L1 bounds. backlog uint64 @@ -972,7 +972,7 @@ func (b *BatchPoster) Start(ctxIn context.Context) { posted, err := b.maybePostSequencerBatch(ctx) ephemeralError := errors.Is(err, AccumulatorNotFoundErr) || errors.Is(err, storage.ErrStorageRace) if !ephemeralError { - b.firstAccErr = time.Time{} + b.firstEphemeralError = time.Time{} } if err != nil { b.building = nil @@ -980,10 +980,10 @@ func (b *BatchPoster) Start(ctxIn context.Context) { if ephemeralError { // Likely the inbox tracker just isn't caught up. // Let's see if this error disappears naturally. - if b.firstAccErr == (time.Time{}) { - b.firstAccErr = time.Now() + if b.firstEphemeralError == (time.Time{}) { + b.firstEphemeralError = time.Now() logLevel = log.Debug - } else if time.Since(b.firstAccErr) < time.Minute { + } else if time.Since(b.firstEphemeralError) < time.Minute { logLevel = log.Debug } }