Skip to content

Commit 042b75a

Browse files
alpealpe
authored
feat: ensure p2p DAHint within limits (#3128)
* feat: validate P2P DA height hints in the syncer against the latest DA height. * Review feedback * Changelog
1 parent e877782 commit 042b75a

3 files changed

Lines changed: 159 additions & 16 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2626
- Store pending blocks separately from executed blocks key. [#3073](https://github.com/evstack/ev-node/pull/3073)
2727
- Fixes issues with force inclusion verification on sync nodes. [#3057](https://github.com/evstack/ev-node/pull/3057)
2828
- Add flag to `local-da` to produce empty DA blocks (closer to the real system). [#3057](https://github.com/evstack/ev-node/pull/3057)
29+
- 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)
2930

3031
## v1.0.0-rc.4
3132

‎block/internal/syncing/syncer.go‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -625,9 +625,40 @@ func (s *Syncer) processHeightEvent(ctx context.Context, event *common.DAHeightE
625625
}
626626
}
627627
if len(daHeightHints) > 0 {
628+
currentDAHeight := s.daRetrieverHeight.Load()
629+
630+
// Only fetch the latest DA height if any hint is suspiciously far ahead.
631+
const daHintMaxDrift = uint64(200)
632+
needsValidation := false
633+
for _, h := range daHeightHints {
634+
if h > currentDAHeight+daHintMaxDrift {
635+
needsValidation = true
636+
break
637+
}
638+
}
639+
640+
var latestDAHeight uint64
641+
if needsValidation {
642+
var err error
643+
xCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond)
644+
latestDAHeight, err = s.daClient.GetLatestDAHeight(xCtx)
645+
cancel()
646+
if err != nil {
647+
s.logger.Warn().Err(err).Msg("failed to fetch latest DA height")
648+
needsValidation = false // ignore error as height is checked in the daRetriever
649+
}
650+
}
651+
628652
for _, daHeightHint := range daHeightHints {
629653
// Skip if we've already fetched past this height
630-
if daHeightHint < s.daRetrieverHeight.Load() {
654+
if daHeightHint < currentDAHeight {
655+
continue
656+
}
657+
658+
if needsValidation && daHeightHint > latestDAHeight {
659+
s.logger.Warn().Uint64("da_height_hint", daHeightHint).
660+
Uint64("latest_da_height", latestDAHeight).
661+
Msg("ignoring unreasonable DA height hint from P2P")
631662
continue
632663
}
633664

‎block/internal/syncing/syncer_test.go‎

Lines changed: 126 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
crand "crypto/rand"
66
"crypto/sha512"
77
"errors"
8+
"math"
89
"sync/atomic"
910
"testing"
1011
"testing/synctest"
@@ -667,10 +668,14 @@ func TestProcessHeightEvent_TriggersAsyncDARetrieval(t *testing.T) {
667668
mockExec := testmocks.NewMockExecutor(t)
668669
mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once()
669670

671+
// Use a mock DA client that reports a latest height above the hint
672+
mockDAClient := testmocks.NewMockClient(t)
673+
mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(200), nil).Maybe()
674+
670675
s := NewSyncer(
671676
st,
672677
mockExec,
673-
nil,
678+
mockDAClient,
674679
cm,
675680
common.NopMetrics(),
676681
cfg,
@@ -696,19 +701,7 @@ func TestProcessHeightEvent_TriggersAsyncDARetrieval(t *testing.T) {
696701
DaHeightHints: [2]uint64{100, 100},
697702
}
698703

699-
// Current height is 0 (from init), event height is 2.
700-
// processHeightEvent checks:
701-
// 1. height <= currentHeight (2 <= 0 -> false)
702-
// 2. height != currentHeight+1 (2 != 1 -> true) -> stores as pending event
703-
704-
// We need to simulate height 1 being processed first so height 2 is "next"
705-
// OR we can just test that it DOES NOT trigger DA retrieval if it's pending.
706-
// Wait, the logic for DA retrieval is BEFORE the "next block" check?
707-
// Let's check syncer.go...
708-
// 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"
709-
710-
// So we need to be at height 1 to process height 2.
711-
// Let's set the store height to 1.
704+
// Set the store height to 1 so the event can be processed as "next".
712705
batch, err := st.NewBatch(context.Background())
713706
require.NoError(t, err)
714707
require.NoError(t, batch.SetHeight(1))
@@ -721,7 +714,64 @@ func TestProcessHeightEvent_TriggersAsyncDARetrieval(t *testing.T) {
721714
assert.Equal(t, uint64(100), priorityHeight)
722715
}
723716

724-
func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) {
717+
func TestProcessHeightEvent_RejectsUnreasonableDAHint(t *testing.T) {
718+
ds := dssync.MutexWrap(datastore.NewMapDatastore())
719+
st := store.New(ds)
720+
cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
721+
require.NoError(t, err)
722+
723+
addr, _, _ := buildSyncTestSigner(t)
724+
cfg := config.DefaultConfig()
725+
gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second), ProposerAddress: addr}
726+
727+
mockExec := testmocks.NewMockExecutor(t)
728+
mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once()
729+
730+
// Mock DA client reports latest DA height of 100
731+
mockDAClient := testmocks.NewMockClient(t)
732+
mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(100), nil).Maybe()
733+
734+
s := NewSyncer(
735+
st,
736+
mockExec,
737+
mockDAClient,
738+
cm,
739+
common.NopMetrics(),
740+
cfg,
741+
gen,
742+
extmocks.NewMockStore[*types.P2PSignedHeader](t),
743+
extmocks.NewMockStore[*types.P2PData](t),
744+
zerolog.Nop(),
745+
common.DefaultBlockOptions(),
746+
make(chan error, 1),
747+
nil,
748+
)
749+
require.NoError(t, s.initializeState())
750+
s.ctx = context.Background()
751+
s.daRetriever = NewDARetriever(nil, cm, gen, zerolog.Nop())
752+
753+
// Set store height to 1 so event at height 2 is "next"
754+
batch, err := st.NewBatch(context.Background())
755+
require.NoError(t, err)
756+
require.NoError(t, batch.SetHeight(1))
757+
require.NoError(t, batch.Commit())
758+
759+
// Send a malicious P2P hint with math.MaxUint64
760+
evt := common.DAHeightEvent{
761+
Header: &types.SignedHeader{Header: types.Header{BaseHeader: types.BaseHeader{ChainID: "c", Height: 2}}},
762+
Data: &types.Data{Metadata: &types.Metadata{ChainID: "c", Height: 2}},
763+
Source: common.SourceP2P,
764+
DaHeightHints: [2]uint64{math.MaxUint64, math.MaxUint64},
765+
}
766+
767+
s.processHeightEvent(t.Context(), &evt)
768+
769+
// Verify that NO priority height was queued — the hint was rejected
770+
priorityHeight := s.daRetriever.PopPriorityHeight()
771+
assert.Equal(t, uint64(0), priorityHeight, "unreasonable DA hint should be rejected")
772+
}
773+
774+
func TestProcessHeightEvent_AcceptsValidDAHint(t *testing.T) {
725775
ds := dssync.MutexWrap(datastore.NewMapDatastore())
726776
st := store.New(ds)
727777
cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
@@ -734,10 +784,71 @@ func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) {
734784
mockExec := testmocks.NewMockExecutor(t)
735785
mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once()
736786

787+
// Mock DA client reports latest DA height of 100
788+
mockDAClient := testmocks.NewMockClient(t)
789+
mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(100), nil).Maybe()
790+
737791
s := NewSyncer(
738792
st,
739793
mockExec,
794+
mockDAClient,
795+
cm,
796+
common.NopMetrics(),
797+
cfg,
798+
gen,
799+
extmocks.NewMockStore[*types.P2PSignedHeader](t),
800+
extmocks.NewMockStore[*types.P2PData](t),
801+
zerolog.Nop(),
802+
common.DefaultBlockOptions(),
803+
make(chan error, 1),
740804
nil,
805+
)
806+
require.NoError(t, s.initializeState())
807+
s.ctx = context.Background()
808+
s.daRetriever = NewDARetriever(nil, cm, gen, zerolog.Nop())
809+
810+
// Set store height to 1 so event at height 2 is "next"
811+
batch, err := st.NewBatch(context.Background())
812+
require.NoError(t, err)
813+
require.NoError(t, batch.SetHeight(1))
814+
require.NoError(t, batch.Commit())
815+
816+
// Send a valid P2P hint at height 50, which is below the latest DA height of 100
817+
evt := common.DAHeightEvent{
818+
Header: &types.SignedHeader{Header: types.Header{BaseHeader: types.BaseHeader{ChainID: "c", Height: 2}}},
819+
Data: &types.Data{Metadata: &types.Metadata{ChainID: "c", Height: 2}},
820+
Source: common.SourceP2P,
821+
DaHeightHints: [2]uint64{50, 50},
822+
}
823+
824+
s.processHeightEvent(t.Context(), &evt)
825+
826+
// Verify that the priority height was queued — the hint is valid
827+
priorityHeight := s.daRetriever.PopPriorityHeight()
828+
assert.Equal(t, uint64(50), priorityHeight, "valid DA hint should be queued")
829+
}
830+
831+
func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) {
832+
ds := dssync.MutexWrap(datastore.NewMapDatastore())
833+
st := store.New(ds)
834+
cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
835+
require.NoError(t, err)
836+
837+
addr, _, _ := buildSyncTestSigner(t)
838+
cfg := config.DefaultConfig()
839+
gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second), ProposerAddress: addr}
840+
841+
mockExec := testmocks.NewMockExecutor(t)
842+
mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once()
843+
844+
// Mock DA client reports latest DA height above the hints we'll send
845+
mockDAClient := testmocks.NewMockClient(t)
846+
mockDAClient.EXPECT().GetLatestDAHeight(mock.Anything).Return(uint64(300), nil).Maybe()
847+
848+
s := NewSyncer(
849+
st,
850+
mockExec,
851+
mockDAClient,
741852
cm,
742853
common.NopMetrics(),
743854
cfg,

0 commit comments

Comments
 (0)