From 7dd74a2ebd722f309647fa5a713209902f88dea2 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Tue, 3 Mar 2026 10:55:31 +0100 Subject: [PATCH 1/3] feat: validate P2P DA height hints in the syncer against the latest DA height. --- block/internal/syncing/syncer.go | 31 +++++- block/internal/syncing/syncer_test.go | 141 +++++++++++++++++++++++--- 2 files changed, 156 insertions(+), 16 deletions(-) diff --git a/block/internal/syncing/syncer.go b/block/internal/syncing/syncer.go index ce96c23ef2..b0617a0225 100644 --- a/block/internal/syncing/syncer.go +++ b/block/internal/syncing/syncer.go @@ -625,9 +625,38 @@ func (s *Syncer) processHeightEvent(ctx context.Context, event *common.DAHeightE } } if len(daHeightHints) > 0 { + currentDAHeight := s.daRetrieverHeight.Load() + + // Only fetch the latest DA height if any hint is suspiciously far ahead. + const daHintMaxDrift = uint64(200) + needsValidation := false + for _, h := range daHeightHints { + if h > currentDAHeight+daHintMaxDrift { + needsValidation = true + break + } + } + + var latestDAHeight uint64 + if needsValidation { + var err error + latestDAHeight, err = s.daClient.GetLatestDAHeight(ctx) + if err != nil { + s.logger.Warn().Err(err).Msg("failed to fetch latest DA height") + needsValidation = false // ignore error as height is checked in the daRetreiver + } + } + for _, daHeightHint := range daHeightHints { // Skip if we've already fetched past this height - if daHeightHint < s.daRetrieverHeight.Load() { + if daHeightHint < currentDAHeight { + continue + } + + if needsValidation && daHeightHint > latestDAHeight { + s.logger.Warn().Uint64("da_height_hint", daHeightHint). + Uint64("latest_da_height", latestDAHeight). + Msg("ignoring unreasonable DA height hint from P2P") continue } diff --git a/block/internal/syncing/syncer_test.go b/block/internal/syncing/syncer_test.go index 5ffd607d5b..a998af8f4e 100644 --- a/block/internal/syncing/syncer_test.go +++ b/block/internal/syncing/syncer_test.go @@ -5,6 +5,7 @@ import ( crand "crypto/rand" "crypto/sha512" "errors" + "math" "sync/atomic" "testing" "testing/synctest" @@ -667,10 +668,14 @@ func TestProcessHeightEvent_TriggersAsyncDARetrieval(t *testing.T) { mockExec := testmocks.NewMockExecutor(t) mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once() + // Use a mock DA client that reports a latest height above the hint + mockDAClient := testmocks.NewMockClient(t) + mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(200), nil).Maybe() + s := NewSyncer( st, mockExec, - nil, + mockDAClient, cm, common.NopMetrics(), cfg, @@ -696,19 +701,7 @@ func TestProcessHeightEvent_TriggersAsyncDARetrieval(t *testing.T) { DaHeightHints: [2]uint64{100, 100}, } - // Current height is 0 (from init), event height is 2. - // processHeightEvent checks: - // 1. height <= currentHeight (2 <= 0 -> false) - // 2. height != currentHeight+1 (2 != 1 -> true) -> stores as pending event - - // We need to simulate height 1 being processed first so height 2 is "next" - // OR we can just test that it DOES NOT trigger DA retrieval if it's pending. - // Wait, the logic for DA retrieval is BEFORE the "next block" check? - // Let's check syncer.go... - // Yes, "If this is a P2P event with a DA height hint, trigger targeted DA retrieval" block is AFTER "If this is not the next block in sequence... return" - - // So we need to be at height 1 to process height 2. - // Let's set the store height to 1. + // Set the store height to 1 so the event can be processed as "next". batch, err := st.NewBatch(context.Background()) require.NoError(t, err) require.NoError(t, batch.SetHeight(1)) @@ -721,7 +714,64 @@ func TestProcessHeightEvent_TriggersAsyncDARetrieval(t *testing.T) { assert.Equal(t, uint64(100), priorityHeight) } -func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) { +func TestProcessHeightEvent_RejectsUnreasonableDAHint(t *testing.T) { + ds := dssync.MutexWrap(datastore.NewMapDatastore()) + st := store.New(ds) + cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop()) + require.NoError(t, err) + + addr, _, _ := buildSyncTestSigner(t) + cfg := config.DefaultConfig() + gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second), ProposerAddress: addr} + + mockExec := testmocks.NewMockExecutor(t) + mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once() + + // Mock DA client reports latest DA height of 100 + mockDAClient := testmocks.NewMockClient(t) + mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(100), nil).Maybe() + + s := NewSyncer( + st, + mockExec, + mockDAClient, + cm, + common.NopMetrics(), + cfg, + gen, + extmocks.NewMockStore[*types.P2PSignedHeader](t), + extmocks.NewMockStore[*types.P2PData](t), + zerolog.Nop(), + common.DefaultBlockOptions(), + make(chan error, 1), + nil, + ) + require.NoError(t, s.initializeState()) + s.ctx = context.Background() + s.daRetriever = NewDARetriever(nil, cm, gen, zerolog.Nop()) + + // Set store height to 1 so event at height 2 is "next" + batch, err := st.NewBatch(context.Background()) + require.NoError(t, err) + require.NoError(t, batch.SetHeight(1)) + require.NoError(t, batch.Commit()) + + // Send a malicious P2P hint with math.MaxUint64 + evt := common.DAHeightEvent{ + Header: &types.SignedHeader{Header: types.Header{BaseHeader: types.BaseHeader{ChainID: "c", Height: 2}}}, + Data: &types.Data{Metadata: &types.Metadata{ChainID: "c", Height: 2}}, + Source: common.SourceP2P, + DaHeightHints: [2]uint64{math.MaxUint64, math.MaxUint64}, + } + + s.processHeightEvent(t.Context(), &evt) + + // Verify that NO priority height was queued — the hint was rejected + priorityHeight := s.daRetriever.PopPriorityHeight() + assert.Equal(t, uint64(0), priorityHeight, "unreasonable DA hint should be rejected") +} + +func TestProcessHeightEvent_AcceptsValidDAHint(t *testing.T) { ds := dssync.MutexWrap(datastore.NewMapDatastore()) st := store.New(ds) cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop()) @@ -734,10 +784,71 @@ func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) { mockExec := testmocks.NewMockExecutor(t) mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once() + // Mock DA client reports latest DA height of 100 + mockDAClient := testmocks.NewMockClient(t) + mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(100), nil).Maybe() + s := NewSyncer( st, mockExec, + mockDAClient, + cm, + common.NopMetrics(), + cfg, + gen, + extmocks.NewMockStore[*types.P2PSignedHeader](t), + extmocks.NewMockStore[*types.P2PData](t), + zerolog.Nop(), + common.DefaultBlockOptions(), + make(chan error, 1), nil, + ) + require.NoError(t, s.initializeState()) + s.ctx = context.Background() + s.daRetriever = NewDARetriever(nil, cm, gen, zerolog.Nop()) + + // Set store height to 1 so event at height 2 is "next" + batch, err := st.NewBatch(context.Background()) + require.NoError(t, err) + require.NoError(t, batch.SetHeight(1)) + require.NoError(t, batch.Commit()) + + // Send a valid P2P hint at height 50, which is below the latest DA height of 100 + evt := common.DAHeightEvent{ + Header: &types.SignedHeader{Header: types.Header{BaseHeader: types.BaseHeader{ChainID: "c", Height: 2}}}, + Data: &types.Data{Metadata: &types.Metadata{ChainID: "c", Height: 2}}, + Source: common.SourceP2P, + DaHeightHints: [2]uint64{50, 50}, + } + + s.processHeightEvent(t.Context(), &evt) + + // Verify that the priority height was queued — the hint is valid + priorityHeight := s.daRetriever.PopPriorityHeight() + assert.Equal(t, uint64(50), priorityHeight, "valid DA hint should be queued") +} + +func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) { + ds := dssync.MutexWrap(datastore.NewMapDatastore()) + st := store.New(ds) + cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop()) + require.NoError(t, err) + + addr, _, _ := buildSyncTestSigner(t) + cfg := config.DefaultConfig() + gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second), ProposerAddress: addr} + + mockExec := testmocks.NewMockExecutor(t) + mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once() + + // Mock DA client reports latest DA height above the hints we'll send + mockDAClient := testmocks.NewMockClient(t) + mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(300), nil).Maybe() + + s := NewSyncer( + st, + mockExec, + mockDAClient, cm, common.NopMetrics(), cfg, From 6f209715ec213d0dd22c876512d20572b38e1376 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Tue, 3 Mar 2026 11:25:16 +0100 Subject: [PATCH 2/3] Review feedback --- block/internal/syncing/syncer.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block/internal/syncing/syncer.go b/block/internal/syncing/syncer.go index b0617a0225..500c231179 100644 --- a/block/internal/syncing/syncer.go +++ b/block/internal/syncing/syncer.go @@ -640,10 +640,12 @@ func (s *Syncer) processHeightEvent(ctx context.Context, event *common.DAHeightE var latestDAHeight uint64 if needsValidation { var err error - latestDAHeight, err = s.daClient.GetLatestDAHeight(ctx) + xCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + latestDAHeight, err = s.daClient.GetLatestDAHeight(xCtx) + cancel() if err != nil { s.logger.Warn().Err(err).Msg("failed to fetch latest DA height") - needsValidation = false // ignore error as height is checked in the daRetreiver + needsValidation = false // ignore error as height is checked in the daRetriever } } From ba1e3cf0e1686374ed62ad908a00b205b4a8c71f Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Tue, 3 Mar 2026 11:31:06 +0100 Subject: [PATCH 3/3] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 281d03d532..5dcf30f689 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Store pending blocks separately from executed blocks key. [#3073](https://github.com/evstack/ev-node/pull/3073) - Fixes issues with force inclusion verification on sync nodes. [#3057](https://github.com/evstack/ev-node/pull/3057) - Add flag to `local-da` to produce empty DA blocks (closer to the real system). [#3057](https://github.com/evstack/ev-node/pull/3057) +- Validate P2P DA height hints against the latest known DA height to prevent malicious peers from triggering runaway catchup . [#3128](https://github.com/evstack/ev-node/pull/3128) ## v1.0.0-rc.4