Skip to content

Commit 0679230

Browse files
authored
fix(block/syncing): verify header data hash vs actual data hash (#2724)
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview Verify header data hash vs actual data hash in syncer. <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. Ex: Closes #<issue number> -->
1 parent 1917bd7 commit 0679230

File tree

5 files changed

+115
-66
lines changed

5 files changed

+115
-66
lines changed

block/internal/syncing/da_retriever.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,11 +163,25 @@ func (r *DARetriever) processBlobs(ctx context.Context, blobs [][]byte, daHeight
163163
}
164164

165165
if header := r.tryDecodeHeader(bz, daHeight); header != nil {
166+
if _, ok := r.pendingHeaders[header.Height()]; ok {
167+
// a (malicious) node may have re-published valid header to another da height (should never happen)
168+
// we can already discard it, only the first one is valid
169+
r.logger.Debug().Uint64("height", header.Height()).Uint64("da_height", daHeight).Msg("header blob already exists for height, discarding")
170+
continue
171+
}
172+
166173
r.pendingHeaders[header.Height()] = header
167174
continue
168175
}
169176

170177
if data := r.tryDecodeData(bz, daHeight); data != nil {
178+
if _, ok := r.pendingData[data.Height()]; ok {
179+
// a (malicious) node may have re-published valid data to another da height (should never happen)
180+
// we can already discard it, only the first one is valid
181+
r.logger.Debug().Uint64("height", data.Height()).Uint64("da_height", daHeight).Msg("data blob already exists for height, discarding")
182+
continue
183+
}
184+
171185
r.pendingData[data.Height()] = data
172186
}
173187
}

block/internal/syncing/da_retriever_test.go

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -27,27 +27,6 @@ import (
2727
"github.com/evstack/ev-node/types"
2828
)
2929

30-
// makeSignedHeaderBytes builds a valid SignedHeader and returns its binary encoding and the object
31-
func makeSignedHeaderBytes(tb testing.TB, chainID string, height uint64, proposer []byte, pub crypto.PubKey, signer signerpkg.Signer, appHash []byte, dataHash []byte) ([]byte, *types.SignedHeader) {
32-
hdr := &types.SignedHeader{
33-
Header: types.Header{
34-
BaseHeader: types.BaseHeader{ChainID: chainID, Height: height, Time: uint64(time.Now().Add(time.Duration(height) * time.Second).UnixNano())},
35-
AppHash: appHash,
36-
DataHash: dataHash,
37-
ProposerAddress: proposer,
38-
},
39-
Signer: types.Signer{PubKey: pub, Address: proposer},
40-
}
41-
bz, err := types.DefaultAggregatorNodeSignatureBytesProvider(&hdr.Header)
42-
require.NoError(tb, err)
43-
sig, err := signer.Sign(bz)
44-
require.NoError(tb, err)
45-
hdr.Signature = sig
46-
bin, err := hdr.MarshalBinary()
47-
require.NoError(tb, err)
48-
return bin, hdr
49-
}
50-
5130
// makeSignedDataBytes builds SignedData containing the provided Data and returns its binary encoding
5231
func makeSignedDataBytes(t *testing.T, chainID string, height uint64, proposer []byte, pub crypto.PubKey, signer signerpkg.Signer, txs int) ([]byte, *types.SignedData) {
5332
d := &types.Data{Metadata: &types.Metadata{ChainID: chainID, Height: height, Time: uint64(time.Now().UnixNano())}}
@@ -189,7 +168,7 @@ func TestDARetriever_ProcessBlobs_HeaderAndData_Success(t *testing.T) {
189168
r := NewDARetriever(nil, cm, config.DefaultConfig(), gen, common.DefaultBlockOptions(), zerolog.Nop())
190169

191170
dataBin, data := makeSignedDataBytes(t, gen.ChainID, 2, addr, pub, signer, 2)
192-
hdrBin, _ := makeSignedHeaderBytes(t, gen.ChainID, 2, addr, pub, signer, nil, data.Hash())
171+
hdrBin, _ := makeSignedHeaderBytes(t, gen.ChainID, 2, addr, pub, signer, nil, &data.Data)
193172

194173
events := r.processBlobs(context.Background(), [][]byte{hdrBin, dataBin}, 77)
195174
require.Len(t, events, 1)
@@ -310,7 +289,7 @@ func TestDARetriever_RetrieveFromDA_TwoNamespaces_Success(t *testing.T) {
310289

311290
// Prepare header/data blobs
312291
dataBin, data := makeSignedDataBytes(t, gen.ChainID, 9, addr, pub, signer, 1)
313-
hdrBin, _ := makeSignedHeaderBytes(t, gen.ChainID, 9, addr, pub, signer, nil, data.Hash())
292+
hdrBin, _ := makeSignedHeaderBytes(t, gen.ChainID, 9, addr, pub, signer, nil, &data.Data)
314293

315294
cfg := config.DefaultConfig()
316295
cfg.DA.Namespace = "nsHdr"
@@ -353,7 +332,7 @@ func TestDARetriever_ProcessBlobs_CrossDAHeightMatching(t *testing.T) {
353332

354333
// Create header and data for the same block height but from different DA heights
355334
dataBin, data := makeSignedDataBytes(t, gen.ChainID, 5, addr, pub, signer, 2)
356-
hdrBin, _ := makeSignedHeaderBytes(t, gen.ChainID, 5, addr, pub, signer, nil, data.Hash())
335+
hdrBin, _ := makeSignedHeaderBytes(t, gen.ChainID, 5, addr, pub, signer, nil, &data.Data)
357336

358337
// Process header from DA height 100 first
359338
events1 := r.processBlobs(context.Background(), [][]byte{hdrBin}, 100)
@@ -392,9 +371,9 @@ func TestDARetriever_ProcessBlobs_MultipleHeadersCrossDAHeightMatching(t *testin
392371
data4Bin, data4 := makeSignedDataBytes(t, gen.ChainID, 4, addr, pub, signer, 2)
393372
data5Bin, data5 := makeSignedDataBytes(t, gen.ChainID, 5, addr, pub, signer, 1)
394373

395-
hdr3Bin, _ := makeSignedHeaderBytes(t, gen.ChainID, 3, addr, pub, signer, nil, data3.Hash())
396-
hdr4Bin, _ := makeSignedHeaderBytes(t, gen.ChainID, 4, addr, pub, signer, nil, data4.Hash())
397-
hdr5Bin, _ := makeSignedHeaderBytes(t, gen.ChainID, 5, addr, pub, signer, nil, data5.Hash())
374+
hdr3Bin, _ := makeSignedHeaderBytes(t, gen.ChainID, 3, addr, pub, signer, nil, &data3.Data)
375+
hdr4Bin, _ := makeSignedHeaderBytes(t, gen.ChainID, 4, addr, pub, signer, nil, &data4.Data)
376+
hdr5Bin, _ := makeSignedHeaderBytes(t, gen.ChainID, 5, addr, pub, signer, nil, &data5.Data)
398377

399378
// Process multiple headers from DA height 200 - should be stored as pending
400379
events1 := r.processBlobs(context.Background(), [][]byte{hdr3Bin, hdr4Bin, hdr5Bin}, 200)

block/internal/syncing/p2p_handler_test.go

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -51,19 +51,6 @@ func p2pMakeSignedHeader(t *testing.T, chainID string, height uint64, proposer [
5151
return hdr
5252
}
5353

54-
// p2pMakeData creates Data with the given number of txs
55-
func p2pMakeData(t *testing.T, chainID string, height uint64, txs int) *types.Data {
56-
t.Helper()
57-
d := &types.Data{Metadata: &types.Metadata{ChainID: chainID, Height: height, Time: uint64(time.Now().UnixNano())}}
58-
if txs > 0 {
59-
d.Txs = make(types.Txs, txs)
60-
for i := 0; i < txs; i++ {
61-
d.Txs[i] = []byte{byte(height), byte(i)}
62-
}
63-
}
64-
return d
65-
}
66-
6754
// P2PTestData aggregates all dependencies used by P2P handler tests.
6855
type P2PTestData struct {
6956
Handler *P2PHandler
@@ -104,7 +91,7 @@ func TestP2PHandler_ProcessHeaderRange_HeaderAndDataHappyPath(t *testing.T) {
10491
// Signed header at height 5 with non-empty data
10592
require.Equal(t, string(p2pData.Genesis.ProposerAddress), string(p2pData.ProposerAddr), "test signer must match genesis proposer for P2P validation")
10693
signedHeader := p2pMakeSignedHeader(t, p2pData.Genesis.ChainID, 5, p2pData.ProposerAddr, p2pData.ProposerPub, p2pData.Signer)
107-
blockData := p2pMakeData(t, p2pData.Genesis.ChainID, 5, 1)
94+
blockData := makeData(p2pData.Genesis.ChainID, 5, 1)
10895
signedHeader.DataHash = blockData.DACommitment()
10996

11097
// Re-sign after setting DataHash so signature matches header bytes
@@ -135,7 +122,7 @@ func TestP2PHandler_ProcessHeaderRange_MissingData_NonEmptyHash(t *testing.T) {
135122
signedHeader := p2pMakeSignedHeader(t, p2pData.Genesis.ChainID, 7, p2pData.ProposerAddr, p2pData.ProposerPub, p2pData.Signer)
136123

137124
// Non-empty data: set header.DataHash to a commitment; expect data store lookup to fail and event skipped
138-
blockData := p2pMakeData(t, p2pData.Genesis.ChainID, 7, 1)
125+
blockData := makeData(p2pData.Genesis.ChainID, 7, 1)
139126
signedHeader.DataHash = blockData.DACommitment()
140127

141128
p2pData.HeaderStore.EXPECT().GetByHeight(ctx, uint64(7)).Return(signedHeader, nil).Once()
@@ -149,7 +136,7 @@ func TestP2PHandler_ProcessDataRange_HeaderMissing(t *testing.T) {
149136
p2pData := setupP2P(t)
150137
ctx := context.Background()
151138

152-
blockData := p2pMakeData(t, p2pData.Genesis.ChainID, 9, 1)
139+
blockData := makeData(p2pData.Genesis.ChainID, 9, 1)
153140
p2pData.DataStore.EXPECT().GetByHeight(ctx, uint64(9)).Return(blockData, nil).Once()
154141
p2pData.HeaderStore.EXPECT().GetByHeight(ctx, uint64(9)).Return(nil, errors.New("no header")).Once()
155142

@@ -182,7 +169,7 @@ func TestP2PHandler_CreateEmptyDataForHeader_UsesPreviousDataHash(t *testing.T)
182169
signedHeader.DataHash = common.DataHashForEmptyTxs
183170

184171
// Mock previous data at height 9 so handler can propagate its hash
185-
previousData := p2pMakeData(t, p2pData.Genesis.ChainID, 9, 1)
172+
previousData := makeData(p2pData.Genesis.ChainID, 9, 1)
186173
p2pData.DataStore.EXPECT().GetByHeight(ctx, uint64(9)).Return(previousData, nil).Once()
187174

188175
emptyData := p2pData.Handler.createEmptyDataForHeader(ctx, signedHeader)
@@ -220,7 +207,7 @@ func TestP2PHandler_ProcessHeaderRange_MultipleHeightsHappyPath(t *testing.T) {
220207
// Build two consecutive heights with valid headers and data
221208
// Height 5
222209
header5 := p2pMakeSignedHeader(t, p2pData.Genesis.ChainID, 5, p2pData.ProposerAddr, p2pData.ProposerPub, p2pData.Signer)
223-
data5 := p2pMakeData(t, p2pData.Genesis.ChainID, 5, 1)
210+
data5 := makeData(p2pData.Genesis.ChainID, 5, 1)
224211
header5.DataHash = data5.DACommitment()
225212
// Re-sign after setting DataHash to keep signature valid
226213
bz5, err := types.DefaultAggregatorNodeSignatureBytesProvider(&header5.Header)
@@ -232,7 +219,7 @@ func TestP2PHandler_ProcessHeaderRange_MultipleHeightsHappyPath(t *testing.T) {
232219

233220
// Height 6
234221
header6 := p2pMakeSignedHeader(t, p2pData.Genesis.ChainID, 6, p2pData.ProposerAddr, p2pData.ProposerPub, p2pData.Signer)
235-
data6 := p2pMakeData(t, p2pData.Genesis.ChainID, 6, 2)
222+
data6 := makeData(p2pData.Genesis.ChainID, 6, 2)
236223
header6.DataHash = data6.DACommitment()
237224
bz6, err := types.DefaultAggregatorNodeSignatureBytesProvider(&header6.Header)
238225
require.NoError(t, err, "failed to get signature bytes for height 6")
@@ -260,7 +247,7 @@ func TestP2PHandler_ProcessDataRange_HeaderValidateHeaderFails(t *testing.T) {
260247
ctx := context.Background()
261248

262249
// Data exists at height 3
263-
blockData := p2pMakeData(t, p2pData.Genesis.ChainID, 3, 1)
250+
blockData := makeData(p2pData.Genesis.ChainID, 3, 1)
264251
p2pData.DataStore.EXPECT().GetByHeight(ctx, uint64(3)).Return(blockData, nil).Once()
265252

266253
// Header proposer does not match genesis -> validateHeader should fail

block/internal/syncing/syncer.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ func (s *Syncer) trySyncNextBlock(event *common.DAHeightEvent) error {
431431
// Compared to the executor logic where the current block needs to be applied first,
432432
// here only the previous block needs to be applied to proceed to the verification.
433433
// The header validation must be done before applying the block to avoid executing gibberish
434-
if err := s.validateBlock(currentState, header, data); err != nil {
434+
if err := s.validateBlock(header, data); err != nil {
435435
// remove header as da included (not per se needed, but keep cache clean)
436436
s.cache.RemoveHeaderDAIncluded(header.Hash().String())
437437
return errors.Join(errInvalidBlock, fmt.Errorf("failed to validate block: %w", err))
@@ -525,8 +525,10 @@ func (s *Syncer) executeTxsWithRetry(ctx context.Context, rawTxs [][]byte, heade
525525
}
526526

527527
// validateBlock validates a synced block
528+
// NOTE: if the header was gibberish and somehow passed all validation prior but the data was correct
529+
// or if the data was gibberish and somehow passed all validation prior but the header was correct
530+
// we are still losing both in the pending event. This should never happen.
528531
func (s *Syncer) validateBlock(
529-
lastState types.State,
530532
header *types.SignedHeader,
531533
data *types.Data,
532534
) error {
@@ -535,6 +537,11 @@ func (s *Syncer) validateBlock(
535537

536538
// Validate header with data
537539
if err := header.ValidateBasicWithData(data); err != nil {
540+
return fmt.Errorf("invalid header: %w", err)
541+
}
542+
543+
// Validate header against data
544+
if err := types.Validate(header, data); err != nil {
538545
return fmt.Errorf("header-data validation failed: %w", err)
539546
}
540547

block/internal/syncing/syncer_test.go

Lines changed: 79 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,26 +42,41 @@ func buildSyncTestSigner(tb testing.TB) (addr []byte, pub crypto.PubKey, signer
4242
return a, p, n
4343
}
4444

45-
func makeSignedHeader(t *testing.T, chainID string, height uint64, proposer []byte, pub crypto.PubKey, signer signerpkg.Signer, appHash []byte) *types.SignedHeader {
45+
// makeSignedHeaderBytes builds a valid SignedHeader and returns its binary encoding and the object
46+
func makeSignedHeaderBytes(tb testing.TB, chainID string, height uint64, proposer []byte, pub crypto.PubKey, signer signerpkg.Signer, appHash []byte, data *types.Data) ([]byte, *types.SignedHeader) {
47+
time := uint64(time.Now().UnixNano())
48+
dataHash := common.DataHashForEmptyTxs
49+
if data != nil {
50+
time = uint64(data.Time().UnixNano())
51+
dataHash = data.DACommitment()
52+
}
53+
4654
hdr := &types.SignedHeader{
4755
Header: types.Header{
48-
BaseHeader: types.BaseHeader{ChainID: chainID, Height: height, Time: uint64(time.Now().Add(time.Duration(height) * time.Second).UnixNano())},
56+
BaseHeader: types.BaseHeader{ChainID: chainID, Height: height, Time: time},
4957
AppHash: appHash,
58+
DataHash: dataHash,
5059
ProposerAddress: proposer,
5160
},
5261
Signer: types.Signer{PubKey: pub, Address: proposer},
5362
}
54-
// sign using aggregator provider (sync node will re-verify using sync provider, which defaults to same header bytes)
5563
bz, err := types.DefaultAggregatorNodeSignatureBytesProvider(&hdr.Header)
56-
require.NoError(t, err)
64+
require.NoError(tb, err)
5765
sig, err := signer.Sign(bz)
58-
require.NoError(t, err)
66+
require.NoError(tb, err)
5967
hdr.Signature = sig
60-
return hdr
68+
bin, err := hdr.MarshalBinary()
69+
require.NoError(tb, err)
70+
return bin, hdr
6171
}
6272

6373
func makeData(chainID string, height uint64, txs int) *types.Data {
64-
d := &types.Data{Metadata: &types.Metadata{ChainID: chainID, Height: height, Time: uint64(time.Now().UnixNano())}}
74+
d := &types.Data{
75+
Metadata: &types.Metadata{
76+
ChainID: chainID,
77+
Height: height,
78+
Time: uint64(time.Now().Add(time.Duration(height) * time.Second).UnixNano())},
79+
}
6580
if txs > 0 {
6681
d.Txs = make(types.Txs, txs)
6782
for i := 0; i < txs; i++ {
@@ -71,6 +86,54 @@ func makeData(chainID string, height uint64, txs int) *types.Data {
7186
return d
7287
}
7388

89+
func TestSyncer_validateBlock_DataHashMismatch(t *testing.T) {
90+
ds := dssync.MutexWrap(datastore.NewMapDatastore())
91+
st := store.New(ds)
92+
cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
93+
require.NoError(t, err)
94+
95+
addr, pub, signer := buildSyncTestSigner(t)
96+
97+
cfg := config.DefaultConfig()
98+
gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second), ProposerAddress: addr}
99+
100+
mockExec := testmocks.NewMockExecutor(t)
101+
102+
s := NewSyncer(
103+
st,
104+
mockExec,
105+
nil,
106+
cm,
107+
common.NopMetrics(),
108+
cfg,
109+
gen,
110+
nil,
111+
nil,
112+
zerolog.Nop(),
113+
common.DefaultBlockOptions(),
114+
make(chan error, 1),
115+
)
116+
117+
// Create header and data with correct hash
118+
data := makeData(gen.ChainID, 1, 2) // non-empty
119+
_, header := makeSignedHeaderBytes(t, gen.ChainID, 1, addr, pub, signer, nil, data)
120+
121+
err = s.validateBlock(header, data)
122+
require.NoError(t, err)
123+
124+
// Create header and data with mismatched hash
125+
data = makeData(gen.ChainID, 1, 2) // non-empty
126+
_, header = makeSignedHeaderBytes(t, gen.ChainID, 1, addr, pub, signer, nil, nil)
127+
err = s.validateBlock(header, data)
128+
require.Error(t, err)
129+
130+
// Create header and empty data
131+
data = makeData(gen.ChainID, 1, 0) // empty
132+
_, header = makeSignedHeaderBytes(t, gen.ChainID, 2, addr, pub, signer, nil, nil)
133+
err = s.validateBlock(header, data)
134+
require.Error(t, err)
135+
}
136+
74137
func TestProcessHeightEvent_SyncsAndUpdatesState(t *testing.T) {
75138
ds := dssync.MutexWrap(datastore.NewMapDatastore())
76139
st := store.New(ds)
@@ -104,9 +167,8 @@ func TestProcessHeightEvent_SyncsAndUpdatesState(t *testing.T) {
104167
s.ctx = context.Background()
105168
// Create signed header & data for height 1
106169
lastState := s.GetLastState()
107-
hdr := makeSignedHeader(t, gen.ChainID, 1, addr, pub, signer, lastState.AppHash)
108170
data := makeData(gen.ChainID, 1, 0)
109-
// For empty data, header.DataHash should be set by producer; here we don't rely on it for syncing
171+
_, hdr := makeSignedHeaderBytes(t, gen.ChainID, 1, addr, pub, signer, lastState.AppHash, data)
110172

111173
// Expect ExecuteTxs call for height 1
112174
mockExec.EXPECT().ExecuteTxs(mock.Anything, mock.Anything, uint64(1), mock.Anything, lastState.AppHash).
@@ -154,17 +216,17 @@ func TestSequentialBlockSync(t *testing.T) {
154216

155217
// Sync two consecutive blocks via processHeightEvent so ExecuteTxs is called and state stored
156218
st0 := s.GetLastState()
157-
hdr1 := makeSignedHeader(t, gen.ChainID, 1, addr, pub, signer, st0.AppHash)
158219
data1 := makeData(gen.ChainID, 1, 1) // non-empty
220+
_, hdr1 := makeSignedHeaderBytes(t, gen.ChainID, 1, addr, pub, signer, st0.AppHash, data1)
159221
// Expect ExecuteTxs call for height 1
160222
mockExec.EXPECT().ExecuteTxs(mock.Anything, mock.Anything, uint64(1), mock.Anything, st0.AppHash).
161223
Return([]byte("app1"), uint64(1024), nil).Once()
162224
evt1 := common.DAHeightEvent{Header: hdr1, Data: data1, DaHeight: 10}
163225
s.processHeightEvent(&evt1)
164226

165227
st1, _ := st.GetState(context.Background())
166-
hdr2 := makeSignedHeader(t, gen.ChainID, 2, addr, pub, signer, st1.AppHash)
167228
data2 := makeData(gen.ChainID, 2, 0) // empty data
229+
_, hdr2 := makeSignedHeaderBytes(t, gen.ChainID, 2, addr, pub, signer, st1.AppHash, data2)
168230
// Expect ExecuteTxs call for height 2
169231
mockExec.EXPECT().ExecuteTxs(mock.Anything, mock.Anything, uint64(2), mock.Anything, st1.AppHash).
170232
Return([]byte("app2"), uint64(1024), nil).Once()
@@ -287,17 +349,17 @@ func TestSyncLoopPersistState(t *testing.T) {
287349
// with n da blobs fetched
288350
for i := range myFutureDAHeight - myDAHeightOffset {
289351
chainHeight, daHeight := i, i+myDAHeightOffset
290-
_, sigHeader := makeSignedHeaderBytes(t, gen.ChainID, chainHeight, addr, pub, signer, nil, nil)
291-
emptyData := types.Data{
352+
emptyData := &types.Data{
292353
Metadata: &types.Metadata{
293-
ChainID: sigHeader.ChainID(),
294-
Height: sigHeader.Height(),
295-
Time: sigHeader.BaseHeader.Time,
354+
ChainID: gen.ChainID,
355+
Height: chainHeight,
356+
Time: uint64(time.Now().Add(time.Duration(chainHeight) * time.Second).UnixNano()),
296357
},
297358
}
359+
_, sigHeader := makeSignedHeaderBytes(t, gen.ChainID, chainHeight, addr, pub, signer, nil, emptyData)
298360
evts := []common.DAHeightEvent{{
299361
Header: sigHeader,
300-
Data: &emptyData,
362+
Data: emptyData,
301363
DaHeight: daHeight,
302364
}}
303365
daRtrMock.On("RetrieveFromDA", mock.Anything, daHeight).Return(evts, nil)

0 commit comments

Comments
 (0)