Skip to content

Commit aea1001

Browse files
author
WorldDogs
committed
refactor: improve batch validation and caching logic
- Enhanced the Get method in BatchCache to validate that batches have signatures before caching. - Updated the rollup method to log when a batch is not found in the cache. - Added tests to ensure proper caching behavior for valid and invalid batches, including scenarios with and without signatures. - Introduced a new test file for batch cache validation to cover various edge cases.
1 parent 8d56264 commit aea1001

File tree

5 files changed

+204
-12
lines changed

5 files changed

+204
-12
lines changed

‎tx-submitter/services/batch_fetcher.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ func (bf *BatchFetcher) GetRollupBatchByIndex(index uint64) (*eth.RPCRollupBatch
2727
lastErr = err
2828
continue
2929
}
30-
if batch != nil {
30+
// Validate that batch exists and has signatures before returning
31+
if batch != nil && len(batch.Signatures) > 0 {
3132
return batch, nil
3233
}
3334
}

‎tx-submitter/services/rollup.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,11 +1042,9 @@ func (r *Rollup) rollup() error {
10421042
}
10431043

10441044
batch, ok := r.batchCache.Get(batchIndex)
1045-
if !ok || len(batch.Signatures) == 0 {
1046-
log.Info("Batch validation failed",
1047-
"batch_index", batchIndex,
1048-
"found", ok,
1049-
"has_signatures", ok && len(batch.Signatures) > 0)
1045+
if !ok {
1046+
log.Info("Batch not found in cache",
1047+
"batch_index", batchIndex)
10501048
return nil
10511049
}
10521050

‎tx-submitter/types/batch_cache.go‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,20 +45,37 @@ func (b *BatchCache) Get(batchIndex uint64) (*eth.RPCRollupBatch, bool) {
4545
return nil, false
4646
}
4747

48-
if fetchedBatch != nil {
49-
// Store in cache for future use
48+
// Validate batch before caching - batch must exist and have signatures
49+
if fetchedBatch != nil && len(fetchedBatch.Signatures) > 0 {
50+
// Store valid batch in cache for future use
5051
b.m.Lock()
5152
b.batchCache[batchIndex] = fetchedBatch
5253
b.m.Unlock()
5354

5455
return fetchedBatch, true
56+
} else if fetchedBatch != nil {
57+
// Batch exists but doesn't have signatures, don't cache it
58+
log.Debug("Batch validation failed - no signatures",
59+
"batch_index", batchIndex,
60+
"found", fetchedBatch != nil,
61+
"has_signatures", len(fetchedBatch.Signatures) > 0)
62+
return fetchedBatch, true
5563
}
5664
}
5765

5866
return nil, false
5967
}
6068

6169
func (b *BatchCache) Set(batchIndex uint64, batch *eth.RPCRollupBatch) {
70+
// Validate batch before caching - batch must exist and have signatures
71+
if batch == nil || len(batch.Signatures) == 0 {
72+
log.Debug("Refusing to cache invalid batch",
73+
"batch_index", batchIndex,
74+
"exists", batch != nil,
75+
"has_signatures", batch != nil && len(batch.Signatures) > 0)
76+
return
77+
}
78+
6279
b.m.Lock()
6380
defer b.m.Unlock()
6481

‎tx-submitter/types/batch_cache_test.go‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,13 @@ func TestBatchCache(t *testing.T) {
3030

3131
expectedBatch := &eth.RPCRollupBatch{
3232
Version: 1,
33+
Signatures: []eth.RPCBatchSignature{
34+
{
35+
Signature: []byte("signature"),
36+
},
37+
},
3338
}
34-
mockFetcher.On("GetRollupBatchByIndex", uint64(1)).Return(expectedBatch, nil).Once()
39+
mockFetcher.On("GetRollupBatchByIndex", uint64(1)).Return(expectedBatch, nil)
3540

3641
batch, ok := cache.Get(1)
3742
assert.True(t, ok)
@@ -64,13 +69,23 @@ func TestBatchCache(t *testing.T) {
6469

6570
batch := &eth.RPCRollupBatch{
6671
Version: 1,
72+
Signatures: []eth.RPCBatchSignature{
73+
{
74+
Signature: []byte("signature"),
75+
},
76+
},
6777
}
6878

79+
// Add this line to set up the mock expectation
80+
mockFetcher.On("GetRollupBatchByIndex", uint64(3)).Return(batch, nil).Maybe()
81+
6982
cache.Set(3, batch)
7083

7184
gotBatch, ok := cache.Get(3)
7285
assert.True(t, ok)
7386
assert.Equal(t, batch, gotBatch)
87+
88+
mockFetcher.AssertExpectations(t)
7489
})
7590

7691
t.Run("Delete batch", func(t *testing.T) {
@@ -79,6 +94,11 @@ func TestBatchCache(t *testing.T) {
7994

8095
batch := &eth.RPCRollupBatch{
8196
Version: 1,
97+
Signatures: []eth.RPCBatchSignature{
98+
{
99+
Signature: []byte("signature"),
100+
},
101+
},
82102
}
83103

84104
cache.Set(4, batch)
@@ -102,8 +122,22 @@ func TestBatchCache(t *testing.T) {
102122
mockFetcher := new(MockBatchFetcher)
103123
cache := NewBatchCache(mockFetcher)
104124

105-
batch1 := &eth.RPCRollupBatch{Version: 1}
106-
batch2 := &eth.RPCRollupBatch{Version: 2}
125+
batch1 := &eth.RPCRollupBatch{
126+
Version: 1,
127+
Signatures: []eth.RPCBatchSignature{
128+
{
129+
Signature: []byte("signature1"),
130+
},
131+
},
132+
}
133+
batch2 := &eth.RPCRollupBatch{
134+
Version: 2,
135+
Signatures: []eth.RPCBatchSignature{
136+
{
137+
Signature: []byte("signature2"),
138+
},
139+
},
140+
}
107141

108142
cache.Set(5, batch1)
109143
cache.Set(6, batch2)
@@ -130,7 +164,14 @@ func TestBatchCache(t *testing.T) {
130164
cache := NewBatchCache(mockFetcher)
131165

132166
// Pre-set a batch to avoid nil pointer in concurrent access
133-
testBatch := &eth.RPCRollupBatch{Version: 7}
167+
testBatch := &eth.RPCRollupBatch{
168+
Version: 7,
169+
Signatures: []eth.RPCBatchSignature{
170+
{
171+
Signature: []byte("signature"),
172+
},
173+
},
174+
}
134175
cache.Set(7, testBatch)
135176

136177
// Setup mock expectation to allow any number of calls

‎tx-submitter/types/batch_cache_validation_test.go‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
package types
2+
3+
import (
4+
"testing"
5+
6+
"github.com/morph-l2/go-ethereum/common"
7+
"github.com/morph-l2/go-ethereum/eth"
8+
"github.com/stretchr/testify/assert"
9+
)
10+
11+
func TestBatchValidation(t *testing.T) {
12+
t.Run("Get - Valid batch with signatures is cached", func(t *testing.T) {
13+
mockFetcher := new(MockBatchFetcher)
14+
cache := NewBatchCache(mockFetcher)
15+
16+
// Create valid batch with signatures
17+
validBatch := &eth.RPCRollupBatch{
18+
Version: 1,
19+
Signatures: []eth.RPCBatchSignature{
20+
{
21+
Signer: common.HexToAddress("0x1234567890123456789012345678901234567890"),
22+
Signature: []byte("test-signature"),
23+
},
24+
},
25+
}
26+
27+
mockFetcher.On("GetRollupBatchByIndex", uint64(1)).Return(validBatch, nil).Once()
28+
29+
// Get should return the batch and cache it
30+
batch, ok := cache.Get(1)
31+
assert.True(t, ok)
32+
assert.Equal(t, validBatch, batch)
33+
assert.Equal(t, 1, len(batch.Signatures))
34+
35+
// Second get should use cache without calling fetcher
36+
batch, ok = cache.Get(1)
37+
assert.True(t, ok)
38+
assert.Equal(t, validBatch, batch)
39+
40+
mockFetcher.AssertExpectations(t)
41+
})
42+
43+
t.Run("Get - Invalid batch without signatures is not cached", func(t *testing.T) {
44+
mockFetcher := new(MockBatchFetcher)
45+
cache := NewBatchCache(mockFetcher)
46+
47+
// Create invalid batch without signatures
48+
invalidBatch := &eth.RPCRollupBatch{
49+
Version: 1,
50+
Signatures: []eth.RPCBatchSignature{}, // Empty signatures
51+
}
52+
53+
mockFetcher.On("GetRollupBatchByIndex", uint64(2)).Return(invalidBatch, nil).Once()
54+
mockFetcher.On("GetRollupBatchByIndex", uint64(2)).Return(invalidBatch, nil).Once() // Second call because not cached
55+
56+
// Get should return the batch but not cache it
57+
batch, ok := cache.Get(2)
58+
assert.True(t, ok) // Still returns true because batch was found, just not cached
59+
assert.Equal(t, invalidBatch, batch)
60+
assert.Equal(t, 0, len(batch.Signatures))
61+
62+
// Second get should call fetcher again since it wasn't cached
63+
batch, ok = cache.Get(2)
64+
assert.True(t, ok)
65+
assert.Equal(t, invalidBatch, batch)
66+
67+
mockFetcher.AssertExpectations(t)
68+
})
69+
70+
t.Run("Set - Valid batch with signatures is stored", func(t *testing.T) {
71+
mockFetcher := new(MockBatchFetcher)
72+
cache := NewBatchCache(mockFetcher)
73+
74+
// Create valid batch with signatures
75+
validBatch := &eth.RPCRollupBatch{
76+
Version: 1,
77+
Signatures: []eth.RPCBatchSignature{
78+
{
79+
Signer: common.HexToAddress("0x1234567890123456789012345678901234567890"),
80+
Signature: []byte("test-signature"),
81+
},
82+
},
83+
}
84+
85+
// Set should store the batch
86+
cache.Set(3, validBatch)
87+
88+
// Get should retrieve from cache
89+
batch, ok := cache.Get(3)
90+
assert.True(t, ok)
91+
assert.Equal(t, validBatch, batch)
92+
})
93+
94+
t.Run("Set - Invalid batch without signatures is not stored", func(t *testing.T) {
95+
mockFetcher := new(MockBatchFetcher)
96+
cache := NewBatchCache(mockFetcher)
97+
98+
// Create invalid batch without signatures
99+
invalidBatch := &eth.RPCRollupBatch{
100+
Version: 1,
101+
Signatures: []eth.RPCBatchSignature{}, // Empty signatures
102+
}
103+
104+
// Set should not store the batch
105+
cache.Set(4, invalidBatch)
106+
107+
// Setup mock for fetching since batch shouldn't be in cache
108+
mockFetcher.On("GetRollupBatchByIndex", uint64(4)).Return(nil, assert.AnError).Once()
109+
110+
// Get should try to fetch from node and fail
111+
batch, ok := cache.Get(4)
112+
assert.False(t, ok)
113+
assert.Nil(t, batch)
114+
115+
mockFetcher.AssertExpectations(t)
116+
})
117+
118+
t.Run("Set - Nil batch is not stored", func(t *testing.T) {
119+
mockFetcher := new(MockBatchFetcher)
120+
cache := NewBatchCache(mockFetcher)
121+
122+
// Set with nil batch should not store anything
123+
cache.Set(5, nil)
124+
125+
// Setup mock for fetching since nothing should be in cache
126+
mockFetcher.On("GetRollupBatchByIndex", uint64(5)).Return(nil, assert.AnError).Once()
127+
128+
// Get should try to fetch from node and fail
129+
batch, ok := cache.Get(5)
130+
assert.False(t, ok)
131+
assert.Nil(t, batch)
132+
133+
mockFetcher.AssertExpectations(t)
134+
})
135+
}

0 commit comments

Comments
 (0)