Skip to content

feat: ensure p2p DAHint within limits #3128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
julienrbrt merged 3 commits into from
Mar 3, 2026
Merged

feat: ensure p2p DAHint within limits #3128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7dd74a2
feat: validate P2P DA height hints in the syncer against the latest D…
alpe Mar 3, 2026
6f20971
Review feedback
alpe Mar 3, 2026
ba1e3cf
Changelog
alpe Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix spacing before period.

There's an extra space before the period in "catchup ." — it should be "catchup."

✏️ Proposed fix 
-- 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)
+- 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)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- 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)
- 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)
🧰 Tools
🪛 LanguageTool

[grammar] ~29-~29: Ensure spelling is correct
Context: ...malicious peers from triggering runaway catchup . [#3128](https://github.com/evstack/ev...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 29, The CHANGELOG entry contains an extra space before
the period in the sentence "Validate P2P DA height hints against the latest
known DA height to prevent malicious peers from triggering runaway catchup .";
remove the stray space so the sentence ends "runaway catchup." (Update the exact
line text in CHANGELOG.md to eliminate the space before the period.)


## v1.0.0-rc.4

Expand Down
33 changes: 32 additions & 1 deletion block/internal/syncing/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,9 +625,40 @@ 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)
Copy link
Contributor Author

@alpe alpe Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an arbitrary value to ensure that we do not hit DA height request on every message

needsValidation := false
for _, h := range daHeightHints {
if h > currentDAHeight+daHintMaxDrift {
needsValidation = true
break
}
}

var latestDAHeight uint64
if needsValidation {
var err error
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 daRetriever
}
}

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).
Copy link
Member

@julienrbrt julienrbrt Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be really noisy during syncing, when we are receiving p2p blocks very far ahead.
I think the 200 is too reasonable, or the log line should be a debug log instead of warning.

Uint64("latest_da_height", latestDAHeight).
Msg("ignoring unreasonable DA height hint from P2P")
continue
}

Expand Down
141 changes: 126 additions & 15 deletions block/internal/syncing/syncer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
crand "crypto/rand"
"crypto/sha512"
"errors"
"math"
"sync/atomic"
"testing"
"testing/synctest"
Expand Down Expand Up @@ -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,
Expand All @@ -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))
Expand All @@ -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())
Expand All @@ -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,
Expand Down
Loading