address PR comments

OffchainLabs · Nov 7, 2024 · 87af748 · 87af748
1 parent 4383d52
commit 87af748
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 33 deletions.
diff --git a/execution/gethexec/blockmetadata.go b/execution/gethexec/blockmetadata.go
@@ -4,14 +4,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"sync"
 
 	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/common/lru"
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/rpc"
 	"github.com/offchainlabs/nitro/arbos/arbostypes"
 	"github.com/offchainlabs/nitro/arbutil"
-	"github.com/offchainlabs/nitro/util/containers"
 	"github.com/offchainlabs/nitro/util/stopwaiter"
 )
 
@@ -29,16 +28,15 @@ type BulkBlockMetadataFetcher struct {
 	bc            *core.BlockChain
 	fetcher       BlockMetadataFetcher
 	reorgDetector chan struct{}
-	blocksLimit   int
-	cacheMutex    sync.RWMutex
-	cache         *containers.LruCache[arbutil.MessageIndex, arbostypes.BlockMetadata]
+	blocksLimit   uint64
+	cache         *lru.SizeConstrainedCache[arbutil.MessageIndex, arbostypes.BlockMetadata]
 }
 
-func NewBulkBlockMetadataFetcher(bc *core.BlockChain, fetcher BlockMetadataFetcher, cacheSize, blocksLimit int) *BulkBlockMetadataFetcher {
-	var cache *containers.LruCache[arbutil.MessageIndex, arbostypes.BlockMetadata]
+func NewBulkBlockMetadataFetcher(bc *core.BlockChain, fetcher BlockMetadataFetcher, cacheSize, blocksLimit uint64) *BulkBlockMetadataFetcher {
+	var cache *lru.SizeConstrainedCache[arbutil.MessageIndex, arbostypes.BlockMetadata]
 	var reorgDetector chan struct{}
 	if cacheSize != 0 {
-		cache = containers.NewLruCache[arbutil.MessageIndex, arbostypes.BlockMetadata](cacheSize)
+		cache = lru.NewSizeConstrainedCache[arbutil.MessageIndex, arbostypes.BlockMetadata](cacheSize)
 		reorgDetector = make(chan struct{})
 		fetcher.SetReorgEventsReader(reorgDetector)
 	}
@@ -73,19 +71,15 @@ func (b *BulkBlockMetadataFetcher) Fetch(fromBlock, toBlock rpc.BlockNumber) ([]
 		var data arbostypes.BlockMetadata
 		var found bool
 		if b.cache != nil {
-			b.cacheMutex.RLock()
 			data, found = b.cache.Get(i)
-			b.cacheMutex.RUnlock()
 		}
 		if !found {
 			data, err = b.fetcher.BlockMetadataAtCount(i + 1)
 			if err != nil {
 				return nil, err
 			}
 			if data != nil && b.cache != nil {
-				b.cacheMutex.Lock()
 				b.cache.Add(i, data)
-				b.cacheMutex.Unlock()
 			}
 		}
 		if data != nil {
@@ -99,9 +93,7 @@ func (b *BulkBlockMetadataFetcher) Fetch(fromBlock, toBlock rpc.BlockNumber) ([]
 }
 
 func (b *BulkBlockMetadataFetcher) ClearCache(ctx context.Context, ignored struct{}) {
-	b.cacheMutex.Lock()
 	b.cache.Clear()
-	b.cacheMutex.Unlock()
 }
 
 func (b *BulkBlockMetadataFetcher) Start(ctx context.Context) {

diff --git a/execution/gethexec/executionengine.go b/execution/gethexec/executionengine.go
@@ -258,6 +258,13 @@ func (s *ExecutionEngine) Reorg(count arbutil.MessageIndex, newMessages []arbost
 		return nil, err
 	}
 
+	if s.reorgEventsReader != nil {
+		select {
+		case s.reorgEventsReader <- struct{}{}:
+		default:
+		}
+	}
+
 	newMessagesResults := make([]*execution.MessageResult, 0, len(oldMessages))
 	for i := range newMessages {
 		var msgForPrefetch *arbostypes.MessageWithMetadata
@@ -277,12 +284,6 @@ func (s *ExecutionEngine) Reorg(count arbutil.MessageIndex, newMessages []arbost
 		s.resequenceChan <- oldMessages
 		resequencing = true
 	}
-	if s.reorgEventsReader != nil {
-		select {
-		case s.reorgEventsReader <- struct{}{}:
-		default:
-		}
-	}
 	return newMessagesResults, nil
 }
 

diff --git a/execution/gethexec/node.go b/execution/gethexec/node.go
@@ -60,8 +60,8 @@ type Config struct {
 	EnablePrefetchBlock         bool                             `koanf:"enable-prefetch-block"`
 	SyncMonitor                 SyncMonitorConfig                `koanf:"sync-monitor"`
 	StylusTarget                StylusTargetConfig               `koanf:"stylus-target"`
-	BlockMetadataApiCacheSize   int                              `koanf:"block-metadata-api-cache-size"`
-	BlockMetadataApiBlocksLimit int                              `koanf:"block-metadata-api-blocks-limit"`
+	BlockMetadataApiCacheSize   uint64                           `koanf:"block-metadata-api-cache-size"`
+	BlockMetadataApiBlocksLimit uint64                           `koanf:"block-metadata-api-blocks-limit"`
 
 	forwardingTarget string
 }
@@ -84,12 +84,6 @@ func (c *Config) Validate() error {
 	if c.forwardingTarget != "" && c.Sequencer.Enable {
 		return errors.New("ForwardingTarget set and sequencer enabled")
 	}
-	if c.BlockMetadataApiCacheSize < 0 {
-		return errors.New("block-metadata-api-cache-size cannot be negative")
-	}
-	if c.BlockMetadataApiBlocksLimit < 0 {
-		return errors.New("block-metadata-api-blocks-limit cannot be negative")
-	}
 	return nil
 }
 
@@ -107,8 +101,8 @@ func ConfigAddOptions(prefix string, f *flag.FlagSet) {
 	f.Uint64(prefix+".tx-lookup-limit", ConfigDefault.TxLookupLimit, "retain the ability to lookup transactions by hash for the past N blocks (0 = all blocks)")
 	f.Bool(prefix+".enable-prefetch-block", ConfigDefault.EnablePrefetchBlock, "enable prefetching of blocks")
 	StylusTargetConfigAddOptions(prefix+".stylus-target", f)
-	f.Int(prefix+".block-metadata-api-cache-size", ConfigDefault.BlockMetadataApiCacheSize, "size of lru cache storing the blockMetadata to service arb_getRawBlockMetadata")
-	f.Int(prefix+".block-metadata-api-blocks-limit", ConfigDefault.BlockMetadataApiBlocksLimit, "maximum number of blocks allowed to be queried for blockMetadata per arb_getRawBlockMetadata query. Enabled by default, set 0 to disable")
+	f.Uint64(prefix+".block-metadata-api-cache-size", ConfigDefault.BlockMetadataApiCacheSize, "size (in bytes) of lru cache storing the blockMetadata to service arb_getRawBlockMetadata")
+	f.Uint64(prefix+".block-metadata-api-blocks-limit", ConfigDefault.BlockMetadataApiBlocksLimit, "maximum number of blocks allowed to be queried for blockMetadata per arb_getRawBlockMetadata query. Enabled by default, set 0 to disable the limit")
 }
 
 var ConfigDefault = Config{
@@ -124,7 +118,7 @@ var ConfigDefault = Config{
 	Forwarder:                   DefaultNodeForwarderConfig,
 	EnablePrefetchBlock:         true,
 	StylusTarget:                DefaultStylusTargetConfig,
-	BlockMetadataApiCacheSize:   10000,
+	BlockMetadataApiCacheSize:   100 * 1024 * 1024,
 	BlockMetadataApiBlocksLimit: 100,
 }
 

diff --git a/go-ethereum b/go-ethereum
diff --git a/system_tests/timeboost_test.go b/system_tests/timeboost_test.go
@@ -121,7 +121,7 @@ func TestTimeboostBulkBlockMetadataAPI(t *testing.T) {
 	}
 
 	// Test that LRU caching works
-	builder.execConfig.BlockMetadataApiCacheSize = 10
+	builder.execConfig.BlockMetadataApiCacheSize = 1000
 	builder.execConfig.BlockMetadataApiBlocksLimit = 25
 	builder.RestartL2Node(t)
 	l2rpc = builder.L2.Stack.Attach()