feat(syncing): detect sequencer double-signs and halt with persisted …#3310
feat(syncing): detect sequencer double-signs and halt with persisted …#3310CaelRowley wants to merge 11 commits intoevstack:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded end-to-end double-sign detection: new DoubleSignEvidence type and protobuf, detection/persistence/reporting logic, metrics and store key support, pending signed-header cache and manager APIs, wiring into DA retriever, P2P handler, and Syncer, plus extensive unit and integration tests. Also added a small cache accessor ChangesDouble-Sign Feature (single coherent DAG)
Sequence Diagram(s)sequenceDiagram
participant P2P as P2P Handler
participant DA as DA Retriever
participant Cache as CacheManager
participant Store as Store
participant Reporter as reportDoubleSign
participant Syncer as Syncer
P2P->>Cache: GetPendingSignedHeader / Validate header
P2P->>Store: GetHeaderByHeight (fallback)
P2P->>Reporter: detectDoubleSign(incoming)
alt Evidence produced
Reporter->>Store: persistEvidence(ds/<h>/<altHash>)
Reporter->>Syncer: handleDoubleSign(evidence)
Syncer->>Syncer: sendCriticalError -> halt syncer
else No evidence
P2P->>Cache: SetPendingSignedHeader(incoming, source=p2p)
P2P->>Syncer: continue processing
end
DA->>Cache: GetPendingSignedHeader / Validate header
DA->>Store: GetHeaderByHeight (fallback)
DA->>Reporter: detectDoubleSign(incoming)
alt Evidence produced
Reporter->>Store: persistEvidence(ds/<h>/<altHash>)
Reporter->>Syncer: handleDoubleSign(evidence)
Syncer->>Syncer: sendCriticalError -> halt syncer
else No evidence
DA->>Cache: SetPendingSignedHeader(incoming, source=da)
DA->>Syncer: continue processing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
types/double_sign_evidence.go (2)
32-47: ⚡ Quick winConsider strengthening
ValidateBasicdefensively.
ValidateBasicis reachable frompersistEvidenceand (transitively)UnmarshalBinary, but it currently doesn't:
- Call
FirstHeader.ValidateBasic()/AlternateHeader.ValidateBasic()on the embedded headers.- Verify the consistency check that
buildEvidenceFromPairenforces (ProposerAddressequality).- Reject empty
FirstSource/AlternateSource.Tightening this prevents persisting malformed proto-decoded evidence and keeps the validation invariants in one place rather than relying on every caller.
♻️ Proposed change
func (e *DoubleSignEvidence) ValidateBasic() error { if e == nil { return errors.New("evidence is nil") } if e.FirstHeader == nil || e.AlternateHeader == nil { return errors.New("evidence requires both first and alternate headers") } if e.FirstHeader.Height() != e.Height || e.AlternateHeader.Height() != e.Height { return fmt.Errorf("evidence height %d does not match both headers (%d, %d)", e.Height, e.FirstHeader.Height(), e.AlternateHeader.Height()) } if bytes.Equal(e.FirstHeader.Hash(), e.AlternateHeader.Hash()) { return errors.New("evidence headers have identical hash — no equivocation") } + if !bytes.Equal(e.FirstHeader.ProposerAddress, e.AlternateHeader.ProposerAddress) { + return errors.New("evidence headers have different proposers — not an equivocation") + } + if e.FirstSource == "" || e.AlternateSource == "" { + return errors.New("evidence requires both source labels") + } return nil }As per coding guidelines: "Validate all external inputs at type boundaries to prevent invalid state".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types/double_sign_evidence.go` around lines 32 - 47, DoubleSignEvidence.ValidateBasic currently omits checks on embedded headers and some invariants; update ValidateBasic to call FirstHeader.ValidateBasic() and AlternateHeader.ValidateBasic(), verify that FirstHeader.ProposerAddress equals AlternateHeader.ProposerAddress (the same invariant enforced by buildEvidenceFromPair), and reject empty FirstSource and AlternateSource (return errors if zero-length). Preserve the existing height/hash checks and error messages, and ensure all new failures return clear errors from ValidateBasic.
73-92: ⚡ Quick winAdd nil checks for required protobuf sub-messages in
FromProto.
p.FirstHeaderandp.AlternateHeaderare required message fields that should be validated before use, consistent with howSignedHeader.FromProtochecksother.Header == niland howValidateBasicenforces both headers are present. The codebase pattern is to validate required nested messages upfront.♻️ Proposed change
func (e *DoubleSignEvidence) FromProto(p *pb.DoubleSignEvidence) error { if p == nil { return errors.New("proto evidence is nil") } + if p.FirstHeader == nil || p.AlternateHeader == nil { + return errors.New("proto evidence missing first or alternate header") + } first := new(SignedHeader) if err := first.FromProto(p.FirstHeader); err != nil { return fmt.Errorf("unmarshal first header: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types/double_sign_evidence.go` around lines 73 - 92, DoubleSignEvidence.FromProto must validate required nested proto messages before using them: check that p.FirstHeader and p.AlternateHeader are not nil at the top of DoubleSignEvidence.FromProto and return a descriptive error if either is nil, then proceed to call SignedHeader.FromProto on each; this mirrors SignedHeader.FromProto's own nil checks and the expectations in ValidateBasic and ensures you don't pass nil into first.FromProto or alt.FromProto.block/internal/syncing/doublesign.go (1)
47-57: 💤 Low valueUse a typed constant for the
"stored"source.
types.EvidenceSourceP2Pandtypes.EvidenceSourceDAalready exist; the third source ("stored") is referenced as a magic string here and almost certainly in tests. AddingEvidenceSourceStoredkeeps the canonical set of ingestion-source labels in one place and avoids drift if the value ever changes.♻️ Proposed change
In
types/double_sign_evidence.go:const ( EvidenceSourceP2P = "p2p" EvidenceSourceDA = "da" + // EvidenceSourceStored is used when the conflicting header was loaded + // from the local store rather than observed live on a network path. + EvidenceSourceStored = "stored" )In this file:
- return buildEvidenceFromPair(storedHeader, incoming, "stored", incomingSource), nil + return buildEvidenceFromPair(storedHeader, incoming, types.EvidenceSourceStored, incomingSource), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign.go` around lines 47 - 57, The code uses the magic string "stored" as an evidence source; add a new typed constant (e.g., EvidenceSourceStored) to the existing enum/constants in types (where EvidenceSourceP2P and EvidenceSourceDA are defined, likely in double_sign_evidence.go) and replace the literal "stored" in buildEvidenceFromPair calls (in function that references storedHeader, incoming, "stored", incomingSource) with types.EvidenceSourceStored; update any tests that assert the string value to use the new constant so all ingestion-source labels are centralized and consistent.block/internal/syncing/syncer.go (1)
1078-1080: ⚡ Quick winDon't silently swallow
reportDoubleSign's error.
reportDoubleSignalready invokessendCriticalErrorfor the halt, so the returned error here is most likely an evidence-persistence failure. Discarding it means an operator sees the node halt without any signal that the evidence record atds/<height>/<alt-hash>failed to write — exactly the artifact human consensus is supposed to inspect per the PR's resolution model.📝 Suggested log on error
func (s *Syncer) handleDoubleSign(ctx context.Context, ev *types.DoubleSignEvidence) { - _ = reportDoubleSign(ctx, s.store, s.metrics, s.logger, s.doubleSignSeen, s.sendCriticalError, ev) + if err := reportDoubleSign(ctx, s.store, s.metrics, s.logger, s.doubleSignSeen, s.sendCriticalError, ev); err != nil { + s.logger.Error().Err(err).Uint64("height", ev.Height()).Msg("failed to report double-sign evidence") + } }Adjust the height accessor (
ev.Height()/ev.Conflicting[0].Height()) to whatevertypes.DoubleSignEvidenceexposes.As per coding guidelines: "Use structured logging with appropriate log levels" and "Wrap errors with context using fmt.Errorf in Go code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/syncer.go` around lines 1078 - 1080, handleDoubleSign is currently ignoring the error returned by reportDoubleSign; instead capture its error return and log it (don't swallow it) using the structured logger (e.g., s.logger.Error) with wrapped context (fmt.Errorf) and include identifying evidence fields such as ev.Height() and the conflicting block hash/alt-hash (e.g., ev.Conflicting[0].Hash or whatever the DoubleSignEvidence exposes) so operators see that persistence of the evidence record (ds/<height>/<alt-hash>) failed; note that reportDoubleSign already triggers sendCriticalError for the halt, so only log/wrap the returned error here with clear context.block/internal/syncing/p2p_handler_doublesign_test.go (1)
85-90: 💤 Low valueOptional: simplify the forged-header construction by skipping the no-op proto round-trip.
var pbDecoded = pbHdrcopies the pointer — both names refer to the same proto object. Theproto.Unmarshal(bin, pbDecoded)call re-decodesbin(marshaled frompbHdr) back into the very same message, leaving it unchanged. The scoped block and the intermediate variable add noise without changing the outcome.♻️ Proposed simplification
forged := new(types.SignedHeader) -{ - var pbDecoded = pbHdr - require.NoError(t, proto.Unmarshal(bin, pbDecoded)) - require.NoError(t, forged.FromProto(pbDecoded)) -} +require.NoError(t, forged.FromProto(pbHdr))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/p2p_handler_doublesign_test.go` around lines 85 - 90, The test currently does a no-op proto round-trip by setting var pbDecoded = pbHdr and calling proto.Unmarshal(bin, pbDecoded) before forged.FromProto; remove the unnecessary pbDecoded and proto.Unmarshal call and call forged.FromProto directly with pbHdr (or use a proper proto.Clone(pbHdr) if you need an independent copy) so the forged header is constructed without the redundant unmarshal step; update the block that references forged, pbHdr, proto.Unmarshal, and forged.FromProto accordingly.block/internal/syncing/doublesign_test.go (2)
386-393: 💤 Low valueOptional: same no-op proto round-trip as in
p2p_handler_doublesign_test.go.
binis already a proto-marshal ofpbHdr(with tampered signature).r.tryDecodeHeader(bin, 100)receives those raw bytes to exercise the legacy decode path, which is the correct intent. The intermediateproto.Unmarshal(bin, pbHdr)call (same pointer) adds nothing: it decodesbinback into the same message it was marshaled from.♻️ Proposed simplification
good := env.signHeaderAtHeight(5, 0x01) pbHdr, err := good.ToProto() require.NoError(t, err) pbHdr.Signature = append([]byte(nil), good.Signature...) pbHdr.Signature[0] ^= 0xff bin, err := proto.Marshal(pbHdr) require.NoError(t, err) -mockClient := testmocks.NewMockClient(t) +// bin is a proto-encoded header with a tampered signature; pass it directly. +mockClient := testmocks.NewMockClient(t)(Drop the intermediate
proto.Unmarshalstep entirely —binis already correct.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign_test.go` around lines 386 - 393, The test performs a no-op proto round-trip that is unnecessary: after creating and tampering pbHdr (via env.signHeaderAtHeight and good.ToProto) you already marshal it into bin, so the intermediate proto.Unmarshal(bin, pbHdr) is redundant; remove that proto.Unmarshal call and pass bin directly to r.tryDecodeHeader (the bytes are already the intended legacy-encoded input), keeping the tampering of pbHdr.Signature and the subsequent proto.Marshal(pbHdr) unchanged.
386-393: 💤 Low valueOptional: same redundant proto round-trip pattern as in
TestP2PHandler_InvalidSigRejectedBeforeDetector.The intermediate
proto.Marshal→proto.Unmarshalcycle is a no-op here:binis marshaled frompbHdr, then decoded back into the same pointer, restoring identical state.tryDecodeHeaderreceivesbinwhich represents the proto-encoded format needed for the legacy path — that part is correct. The forged header itself can be derived directly.♻️ Proposed simplification
good := env.signHeaderAtHeight(5, 0x01) pbHdr, err := good.ToProto() require.NoError(t, err) pbHdr.Signature = append([]byte(nil), good.Signature...) pbHdr.Signature[0] ^= 0xff bin, err := proto.Marshal(pbHdr) require.NoError(t, err)(The rest of the test uses
bindirectly — noforgedtype.SignedHeader is needed fortryDecodeHeader.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign_test.go` around lines 386 - 393, The test performs an unnecessary proto marshal/unmarshal round-trip to create `bin` after mutating `pbHdr.Signature`; simplify by mutating the proto header produced by `env.signHeaderAtHeight(...).ToProto()` (the `pbHdr` variable) and then directly proto.Marshal that `pbHdr` into `bin` to pass to `tryDecodeHeader`, removing the redundant unmarshal/restore sequence and any unused `forged` SignedHeader creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@block/internal/syncing/doublesign_test.go`:
- Around line 618-625: Replace the bare string literal "stored" used for
FirstSource in the DoubleSignEvidence test with a typed evidence-source value to
match AlternateSource usage: either add a typed constant (e.g.,
EvidenceSourceStored types.EvidenceSource = "stored") in the types package and
use types.EvidenceSourceStored for FirstSource in the two test instances (p2pEv
and daEv), or define a test-scoped constant in this file (e.g., stored :=
types.EvidenceSource("stored")) and use that for FirstSource so both FirstSource
and AlternateSource are typed as types.EvidenceSource.
- Around line 618-625: Add a typed constant for the "stored" evidence-source
alongside the existing EvidenceSourceP2P and EvidenceSourceDA (e.g., const
EvidenceSourceStored types.EvidenceSource = "stored") in the same file where
EvidenceSourceP2P/DA are defined (the types/double_sign_evidence.go symbol
block). Replace all string-literal uses of "stored" in production code (e.g., in
the DoubleSign handling in doublesign.go where the sentinel is currently
hardcoded) and tests (symbols: the DoubleSignEvidence struct usages in
doublesign_test.go and doublesign_branches_test.go) with the new typed constant
types.EvidenceSourceStored so the sentinel is consistent everywhere.
In `@types/double_sign_evidence.go`:
- Around line 22-29: The DoubleSignEvidence struct lacks JSON tags and a
String() method; update the type DoubleSignEvidence by adding JSON tags to each
field (Height, FirstHeader, AlternateHeader, DetectedAt, FirstSource,
AlternateSource) so they marshal/unmarshal with conventional names (e.g.,
`height`, `first_header`, etc.), and implement a String() method on
DoubleSignEvidence that returns a human-readable representation (include Height,
DetectedAt, FirstSource, AlternateSource and concise summaries of
FirstHeader/AlternateHeader) to satisfy the codebase's inspection/debugging
requirements.
---
Nitpick comments:
In `@block/internal/syncing/doublesign_test.go`:
- Around line 386-393: The test performs a no-op proto round-trip that is
unnecessary: after creating and tampering pbHdr (via env.signHeaderAtHeight and
good.ToProto) you already marshal it into bin, so the intermediate
proto.Unmarshal(bin, pbHdr) is redundant; remove that proto.Unmarshal call and
pass bin directly to r.tryDecodeHeader (the bytes are already the intended
legacy-encoded input), keeping the tampering of pbHdr.Signature and the
subsequent proto.Marshal(pbHdr) unchanged.
- Around line 386-393: The test performs an unnecessary proto marshal/unmarshal
round-trip to create `bin` after mutating `pbHdr.Signature`; simplify by
mutating the proto header produced by `env.signHeaderAtHeight(...).ToProto()`
(the `pbHdr` variable) and then directly proto.Marshal that `pbHdr` into `bin`
to pass to `tryDecodeHeader`, removing the redundant unmarshal/restore sequence
and any unused `forged` SignedHeader creation.
In `@block/internal/syncing/doublesign.go`:
- Around line 47-57: The code uses the magic string "stored" as an evidence
source; add a new typed constant (e.g., EvidenceSourceStored) to the existing
enum/constants in types (where EvidenceSourceP2P and EvidenceSourceDA are
defined, likely in double_sign_evidence.go) and replace the literal "stored" in
buildEvidenceFromPair calls (in function that references storedHeader, incoming,
"stored", incomingSource) with types.EvidenceSourceStored; update any tests that
assert the string value to use the new constant so all ingestion-source labels
are centralized and consistent.
In `@block/internal/syncing/p2p_handler_doublesign_test.go`:
- Around line 85-90: The test currently does a no-op proto round-trip by setting
var pbDecoded = pbHdr and calling proto.Unmarshal(bin, pbDecoded) before
forged.FromProto; remove the unnecessary pbDecoded and proto.Unmarshal call and
call forged.FromProto directly with pbHdr (or use a proper proto.Clone(pbHdr) if
you need an independent copy) so the forged header is constructed without the
redundant unmarshal step; update the block that references forged, pbHdr,
proto.Unmarshal, and forged.FromProto accordingly.
In `@block/internal/syncing/syncer.go`:
- Around line 1078-1080: handleDoubleSign is currently ignoring the error
returned by reportDoubleSign; instead capture its error return and log it (don't
swallow it) using the structured logger (e.g., s.logger.Error) with wrapped
context (fmt.Errorf) and include identifying evidence fields such as ev.Height()
and the conflicting block hash/alt-hash (e.g., ev.Conflicting[0].Hash or
whatever the DoubleSignEvidence exposes) so operators see that persistence of
the evidence record (ds/<height>/<alt-hash>) failed; note that reportDoubleSign
already triggers sendCriticalError for the halt, so only log/wrap the returned
error here with clear context.
In `@types/double_sign_evidence.go`:
- Around line 32-47: DoubleSignEvidence.ValidateBasic currently omits checks on
embedded headers and some invariants; update ValidateBasic to call
FirstHeader.ValidateBasic() and AlternateHeader.ValidateBasic(), verify that
FirstHeader.ProposerAddress equals AlternateHeader.ProposerAddress (the same
invariant enforced by buildEvidenceFromPair), and reject empty FirstSource and
AlternateSource (return errors if zero-length). Preserve the existing
height/hash checks and error messages, and ensure all new failures return clear
errors from ValidateBasic.
- Around line 73-92: DoubleSignEvidence.FromProto must validate required nested
proto messages before using them: check that p.FirstHeader and p.AlternateHeader
are not nil at the top of DoubleSignEvidence.FromProto and return a descriptive
error if either is nil, then proceed to call SignedHeader.FromProto on each;
this mirrors SignedHeader.FromProto's own nil checks and the expectations in
ValidateBasic and ensures you don't pass nil into first.FromProto or
alt.FromProto.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e8f031-b97f-4dad-94c6-82f3ba156f19
⛔ Files ignored due to path filters (1)
types/pb/evnode/v1/evnode.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
block/internal/cache/generic_cache.goblock/internal/cache/generic_cache_test.goblock/internal/cache/manager.goblock/internal/cache/manager_test.goblock/internal/common/metrics.goblock/internal/syncing/da_retriever.goblock/internal/syncing/da_retriever_test.goblock/internal/syncing/doublesign.goblock/internal/syncing/doublesign_branches_test.goblock/internal/syncing/doublesign_test.goblock/internal/syncing/p2p_handler.goblock/internal/syncing/p2p_handler_doublesign_test.goblock/internal/syncing/p2p_handler_test.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_forced_inclusion_test.goblock/internal/syncing/syncer_test.gopkg/store/keys.goproto/evnode/v1/evnode.prototypes/double_sign_evidence.go
| type DoubleSignEvidence struct { | ||
| Height uint64 | ||
| FirstHeader *SignedHeader | ||
| AlternateHeader *SignedHeader | ||
| DetectedAt time.Time | ||
| FirstSource string | ||
| AlternateSource string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add JSON tags and a String() method to DoubleSignEvidence.
The repository coding guidelines for types/**/*.go require JSON tags on struct fields and a String() method on core types. Both help when inspecting persisted/decoded evidence in logs and debug tooling.
♻️ Proposed change
type DoubleSignEvidence struct {
- Height uint64
- FirstHeader *SignedHeader
- AlternateHeader *SignedHeader
- DetectedAt time.Time
- FirstSource string
- AlternateSource string
+ Height uint64 `json:"height"`
+ FirstHeader *SignedHeader `json:"first_header"`
+ AlternateHeader *SignedHeader `json:"alternate_header"`
+ DetectedAt time.Time `json:"detected_at"`
+ FirstSource string `json:"first_source"`
+ AlternateSource string `json:"alternate_source"`
}
+
+// String returns a concise human-readable summary of the evidence.
+func (e *DoubleSignEvidence) String() string {
+ if e == nil {
+ return "DoubleSignEvidence(nil)"
+ }
+ var firstHash, altHash string
+ if e.FirstHeader != nil {
+ firstHash = e.FirstHeader.Hash().String()
+ }
+ if e.AlternateHeader != nil {
+ altHash = e.AlternateHeader.Hash().String()
+ }
+ return fmt.Sprintf("DoubleSignEvidence{height=%d first=%s(%s) alt=%s(%s) detected_at=%s}",
+ e.Height, firstHash, e.FirstSource, altHash, e.AlternateSource, e.DetectedAt.Format(time.RFC3339Nano))
+}As per coding guidelines: "Add JSON tags to type struct fields for inspection and marshaling" and "Implement String() methods for all core types to aid in debugging and inspection".
📝 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.
| type DoubleSignEvidence struct { | |
| Height uint64 | |
| FirstHeader *SignedHeader | |
| AlternateHeader *SignedHeader | |
| DetectedAt time.Time | |
| FirstSource string | |
| AlternateSource string | |
| } | |
| type DoubleSignEvidence struct { | |
| Height uint64 `json:"height"` | |
| FirstHeader *SignedHeader `json:"first_header"` | |
| AlternateHeader *SignedHeader `json:"alternate_header"` | |
| DetectedAt time.Time `json:"detected_at"` | |
| FirstSource string `json:"first_source"` | |
| AlternateSource string `json:"alternate_source"` | |
| } | |
| // String returns a concise human-readable summary of the evidence. | |
| func (e *DoubleSignEvidence) String() string { | |
| if e == nil { | |
| return "DoubleSignEvidence(nil)" | |
| } | |
| var firstHash, altHash string | |
| if e.FirstHeader != nil { | |
| firstHash = e.FirstHeader.Hash().String() | |
| } | |
| if e.AlternateHeader != nil { | |
| altHash = e.AlternateHeader.Hash().String() | |
| } | |
| return fmt.Sprintf("DoubleSignEvidence{height=%d first=%s(%s) alt=%s(%s) detected_at=%s}", | |
| e.Height, firstHash, e.FirstSource, altHash, e.AlternateSource, e.DetectedAt.Format(time.RFC3339Nano)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/double_sign_evidence.go` around lines 22 - 29, The DoubleSignEvidence
struct lacks JSON tags and a String() method; update the type DoubleSignEvidence
by adding JSON tags to each field (Height, FirstHeader, AlternateHeader,
DetectedAt, FirstSource, AlternateSource) so they marshal/unmarshal with
conventional names (e.g., `height`, `first_header`, etc.), and implement a
String() method on DoubleSignEvidence that returns a human-readable
representation (include Height, DetectedAt, FirstSource, AlternateSource and
concise summaries of FirstHeader/AlternateHeader) to satisfy the codebase's
inspection/debugging requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
types/double_sign_evidence.go (1)
111-117: ⚡ Quick winWrap the
proto.Unmarshalerror inUnmarshalBinary.Line 114 returns the raw error from
proto.Unmarshal. Per the coding guidelines ("Wrap errors with context usingfmt.Errorf"), this should be wrapped so callers get actionable context.♻️ Proposed fix
func (e *DoubleSignEvidence) UnmarshalBinary(data []byte) error { p := new(pb.DoubleSignEvidence) if err := proto.Unmarshal(data, p); err != nil { - return err + return fmt.Errorf("proto unmarshal double sign evidence: %w", err) } return e.FromProto(p) }As per coding guidelines: "Wrap errors with context using
fmt.Errorfin Go code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types/double_sign_evidence.go` around lines 111 - 117, In DoubleSignEvidence.UnmarshalBinary, wrap the error returned by proto.Unmarshal with contextual information using fmt.Errorf (e.g., include "failed to unmarshal DoubleSignEvidence" or similar) so callers receive actionable context; update the error return at the proto.Unmarshal call in UnmarshalBinary to return the wrapped error and ensure fmt is imported if not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@types/double_sign_evidence.go`:
- Around line 54-74: DoubleSignEvidence.ToProto currently only checks e == nil
but calls e.FirstHeader.ToProto() and e.AlternateHeader.ToProto() which will
panic if those fields are nil; update DoubleSignEvidence.ToProto to validate
that e.FirstHeader and e.AlternateHeader are non-nil before calling ToProto
(e.g., if e.FirstHeader == nil return an error like fmt.Errorf("marshal first
header: header is nil") and similarly for AlternateHeader), then proceed to call
FirstHeader.ToProto() and AlternateHeader.ToProto() and return the assembled
pb.DoubleSignEvidence.
---
Nitpick comments:
In `@types/double_sign_evidence.go`:
- Around line 111-117: In DoubleSignEvidence.UnmarshalBinary, wrap the error
returned by proto.Unmarshal with contextual information using fmt.Errorf (e.g.,
include "failed to unmarshal DoubleSignEvidence" or similar) so callers receive
actionable context; update the error return at the proto.Unmarshal call in
UnmarshalBinary to return the wrapped error and ensure fmt is imported if not
already.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b38bf58-b4ab-4b27-8362-0e93a90b1d81
📒 Files selected for processing (4)
block/internal/syncing/doublesign.goblock/internal/syncing/doublesign_branches_test.goblock/internal/syncing/doublesign_test.gotypes/double_sign_evidence.go
✅ Files skipped from review due to trivial changes (2)
- block/internal/syncing/doublesign_test.go
- block/internal/syncing/doublesign.go
🚧 Files skipped from review as they are similar to previous changes (1)
- block/internal/syncing/doublesign_branches_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@types/double_sign_evidence.go`:
- Around line 32-50: Update DoubleSignEvidence.ValidateBasic to fully validate
nested SignedHeader contents and the evidence Source set: ensure e.FirstHeader
and e.AlternateHeader are non-nil and call their ValidateBasic/consistency
checks (verify header signatures, commit presence, heights, round, blockID
consistency as applicable via SignedHeader.ValidateBasic or equivalent),
re-check proposers and that header hashes differ, and validate that e.Source (or
Source string/enum) is in the allowed set; then, in FromProto (the constructor
that builds DoubleSignEvidence from protobufs), call the updated ValidateBasic()
and return an error if it fails so malformed protobufs are rejected at the
boundary. Use the unique symbols DoubleSignEvidence.ValidateBasic, FromProto,
and SignedHeader (or SignedHeader.ValidateBasic) to locate and modify the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75193429-0609-4fe8-b0cc-38b99be22131
📒 Files selected for processing (3)
block/internal/syncing/doublesign_branches_test.goblock/internal/syncing/doublesign_test.gotypes/double_sign_evidence.go
| // ValidateBasic checks structural consistency of the evidence. | ||
| func (e *DoubleSignEvidence) ValidateBasic() error { | ||
| if e == nil { | ||
| return errors.New("evidence is nil") | ||
| } | ||
| if e.FirstHeader == nil || e.AlternateHeader == nil { | ||
| return errors.New("evidence requires both first and alternate headers") | ||
| } | ||
| if e.FirstHeader.Height() != e.Height || e.AlternateHeader.Height() != e.Height { | ||
| return fmt.Errorf("evidence height %d does not match both headers (%d, %d)", | ||
| e.Height, e.FirstHeader.Height(), e.AlternateHeader.Height()) | ||
| } | ||
| if bytes.Equal(e.FirstHeader.Hash(), e.AlternateHeader.Hash()) { | ||
| return errors.New("evidence headers have identical hash — no equivocation") | ||
| } | ||
| if !bytes.Equal(e.FirstHeader.ProposerAddress, e.AlternateHeader.ProposerAddress) { | ||
| return errors.New("evidence headers have different proposers — not an equivocation") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Reject malformed evidence at the protobuf boundary.
FromProto() only enforces non-nil submessages, and ValidateBasic() currently only checks header-to-header relationships. That means a corrupted protobuf with invalid nested SignedHeaders, mismatched invariants, or arbitrary source strings can deserialize cleanly and then participate in persistence/reporting. Please make ValidateBasic() validate both nested headers plus the allowed source set, and call it from FromProto() before returning.
🛡️ Proposed fix
func (e *DoubleSignEvidence) ValidateBasic() error {
if e == nil {
return errors.New("evidence is nil")
}
if e.FirstHeader == nil || e.AlternateHeader == nil {
return errors.New("evidence requires both first and alternate headers")
}
+ if err := e.FirstHeader.ValidateBasic(); err != nil {
+ return fmt.Errorf("validate first header: %w", err)
+ }
+ if err := e.AlternateHeader.ValidateBasic(); err != nil {
+ return fmt.Errorf("validate alternate header: %w", err)
+ }
if e.FirstHeader.Height() != e.Height || e.AlternateHeader.Height() != e.Height {
return fmt.Errorf("evidence height %d does not match both headers (%d, %d)",
e.Height, e.FirstHeader.Height(), e.AlternateHeader.Height())
}
if bytes.Equal(e.FirstHeader.Hash(), e.AlternateHeader.Hash()) {
return errors.New("evidence headers have identical hash — no equivocation")
}
if !bytes.Equal(e.FirstHeader.ProposerAddress, e.AlternateHeader.ProposerAddress) {
return errors.New("evidence headers have different proposers — not an equivocation")
}
+ switch e.FirstSource {
+ case EvidenceSourceP2P, EvidenceSourceDA, EvidenceSourceStored:
+ default:
+ return fmt.Errorf("invalid first source %q", e.FirstSource)
+ }
+ switch e.AlternateSource {
+ case EvidenceSourceP2P, EvidenceSourceDA, EvidenceSourceStored:
+ default:
+ return fmt.Errorf("invalid alternate source %q", e.AlternateSource)
+ }
return nil
}
@@
e.DetectedAt = time.Unix(0, p.DetectedAt).UTC()
e.FirstSource = p.FirstSource
e.AlternateSource = p.AlternateSource
- return nil
+ if err := e.ValidateBasic(); err != nil {
+ return fmt.Errorf("validate double sign evidence: %w", err)
+ }
+ return nil
}As per coding guidelines: "Implement ValidateBasic() methods that check required fields, validate ranges, and check consistency for all types" and "Validate all external inputs at type boundaries to prevent invalid state".
Also applies to: 80-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/double_sign_evidence.go` around lines 32 - 50, Update
DoubleSignEvidence.ValidateBasic to fully validate nested SignedHeader contents
and the evidence Source set: ensure e.FirstHeader and e.AlternateHeader are
non-nil and call their ValidateBasic/consistency checks (verify header
signatures, commit presence, heights, round, blockID consistency as applicable
via SignedHeader.ValidateBasic or equivalent), re-check proposers and that
header hashes differ, and validate that e.Source (or Source string/enum) is in
the allowed set; then, in FromProto (the constructor that builds
DoubleSignEvidence from protobufs), call the updated ValidateBasic() and return
an error if it fails so malformed protobufs are rejected at the boundary. Use
the unique symbols DoubleSignEvidence.ValidateBasic, FromProto, and SignedHeader
(or SignedHeader.ValidateBasic) to locate and modify the code.
There was a problem hiding this comment.
We should handle this in the syncer.go directly, no need to duplicate it between the da retriever and p2p handler
There was a problem hiding this comment.
Thanks for the review, I refactored out the logic into Syncer.detectDoubleSign but the retriever/handler calls must stay because of an in batch DA case if two conflicting headers from a malicious sequencer land at the same DA height, the retriever fetches both in one batch and drops the second one before it emits DAHeightEvent. So the syncer would never see the alternate.
|
|
||
| // handleDoubleSign persists evidence, bumps the metric, and halts the syncer | ||
| // via sendCriticalError. Wired into the DA retriever and P2P handler. | ||
| func (s *Syncer) handleDoubleSign(ctx context.Context, ev *types.DoubleSignEvidence) { |
There was a problem hiding this comment.
the arguments are weird, it returns an error but uses internally sendCriticalError and we ignore the error here.
There was a problem hiding this comment.
You are right, I updated this so reportDoubleSign no longer takes a criticalErr callback. It just returns the wrapped error, and handleDoubleSign forwards it to sendCriticalError explicitly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
block/internal/syncing/doublesign_syncer_integration_test.go (1)
78-79: 💤 Low valueManual
doubleSignSeeninit in tests duplicatesStart()setup.The integration harness skips
Start()and manually initializess.doubleSignSeen = newDoubleSignDedup(). This will silently break ifStart()adds more state initialization for double-sign detection in the future. Consider extracting aninitSyncWorkersHelper()(or similar) on*Syncerthat bothStart()and tests call, so test setup stays in sync with production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign_syncer_integration_test.go` around lines 78 - 79, The test manually initializes s.doubleSignSeen = newDoubleSignDedup(), duplicating Start() setup and risking drift; add a new initialization helper method (e.g., func (s *Syncer) initSyncWorkersHelper()) that contains all double-sign/detection-related initialization (initializing doubleSignSeen and any future related fields), update Syncer.Start() to call initSyncWorkersHelper(), and change the test to call s.initSyncWorkersHelper() instead of directly assigning s.doubleSignSeen so test setup stays in sync with production; reference the Syncer.Start and the doubleSignSeen field when making these changes.block/internal/syncing/doublesign_test.go (1)
76-101: ⚖️ Poor tradeoffLGTM — test detector accurately mirrors
Syncer.detectDoubleSign.The test
makeDetectDoubleSignreproduces the real syncer's detection flow (firstObservation → buildEvidence → cache pending or capture+persist) while substituting a CAS-based capture for the halt pipeline. The CAS loop at lines 87–94 is correct foratomic.Pointer[*[]…]updates. One minor risk: ifSyncer.detectDoubleSignevolves (e.g., adds new side-effects like metrics), this test detector will silently diverge. Consider extracting the detection core into a helper that both production and tests can share, parameterized on the "what to do on detection" callback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign_test.go` around lines 76 - 101, The test-only detector in makeDetectDoubleSign mirrors Syncer.detectDoubleSign but may silently diverge if detectDoubleSign gains side-effects (e.g., metrics); refactor the detection core used by firstObservation → buildEvidence → persistEvidence into a shared helper function (e.g., detectDoubleSignCore) that accepts a callback for "on detection" behavior, then change Syncer.detectDoubleSign to call detectDoubleSignCore with the production callback and update makeDetectDoubleSign to call the same core with a test callback that captures evidence into e.capLock (and still calls persistEvidence as needed) so both production and tests use identical detection logic while only the reaction differs.block/internal/syncing/syncer.go (1)
1085-1098: 💤 Low valueDetection error path still caches the alternate as a "first observation".
When
firstObservationerrors (line 1090–1091), the warning is logged but the function falls through and cachesheaderviaSetPendingSignedHeaderat line 1096 as if it were a fresh first observation. This is reasonable for resilience, but it means a flapping store error during cross-source detection can hide an equivocation: the alternate gets recorded as canonical-pending, and a subsequent observation of the actual canonical might compare against the alternate, recording it as the “first” header in the evidence (FirstSourcewill be wrong).Consider not caching on error, or at least logging at a higher severity that detection was bypassed:
♻️ Defensive variant
func (s *Syncer) detectDoubleSign(ctx context.Context, header *types.SignedHeader, source string) bool { if header == nil { return false } prior, priorSource, err := firstObservation(ctx, s.store, s.cache, header.Height()) if err != nil { - s.logger.Warn().Err(err).Uint64("height", header.Height()).Msg("double-sign detection error") + s.logger.Error().Err(err).Uint64("height", header.Height()). + Msg("double-sign detection bypassed; skipping pending cache update to avoid mislabeling evidence") + return false } else if ev := buildEvidenceFromPair(prior, header, priorSource, source); ev != nil { s.handleDoubleSign(ctx, ev) return true } s.cache.SetPendingSignedHeader(header, source) return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/syncer.go` around lines 1085 - 1098, In Syncer.detectDoubleSign, don't cache the header when firstObservation returns an error (or at minimum escalate the log) because an intermittent store error can make the alternate become the recorded "first" observation; change the control flow so that after firstObservation returns an error you either return false immediately (skipping s.cache.SetPendingSignedHeader) or explicitly avoid calling SetPendingSignedHeader for that header, and also upgrade the warning to a higher-severity log (or include context) so missed detection is obvious; reference detectDoubleSign, firstObservation, SetPendingSignedHeader, buildEvidenceFromPair, and handleDoubleSign when making this change.block/internal/syncing/doublesign.go (2)
96-142: 💤 Low valuePersist-then-dedup ordering swallows errors on duplicate calls.
reportDoubleSignalways invokespersistEvidence(line 110) even after the first sighting. On a deduplicated call, the function returnsnilat line 113, silently dropping anypersistErrfrom the redundant persist attempt. While the comment justifies retrying for transient first-call failures, aftermarkSeenhas returnedtrueonce, the evidence is already known to be persisted (or its failure was already logged on the first call). Re-persisting on every subsequent observation adds I/O without benefit, and any error is dropped silently.Consider gating the persist call so it runs at most twice (once on first sighting, once if the first failed):
♻️ Suggested refactor
- // Persist on every call: idempotent, and a retry covers a transient - // failure on the first attempt. - persistErr := persistEvidence(ctx, st, ev) - - if seen != nil && !seen.markSeen(ev.Height, altHashStr) { - return nil - } - - if persistErr != nil { - logger.Error().Err(persistErr). - Uint64("height", ev.Height). - Str("first_hash", firstHashStr). - Str("alternate_hash", altHashStr). - Msg("failed to persist double-sign evidence") - } + if seen != nil && !seen.markSeen(ev.Height, altHashStr) { + return nil + } + + if err := persistEvidence(ctx, st, ev); err != nil { + logger.Error().Err(err). + Uint64("height", ev.Height). + Str("first_hash", firstHashStr). + Str("alternate_hash", altHashStr). + Msg("failed to persist double-sign evidence") + }Note this changes semantics if you specifically want a retry on first-call persistence failure. If retry-on-failure is the goal, consider tracking persistence success separately from
markSeen(e.g. only flipmarkSeenonce persistence succeeded), so subsequent attempts retry but successful ones short-circuit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign.go` around lines 96 - 142, The function reportDoubleSign currently calls persistEvidence unconditionally and then returns nil when seen.markSeen(...) indicates a duplicate, which discards persistErr; change the ordering so persistEvidence is only called when this is the first sighting: call seen.markSeen(ev.Height, altHashStr) first (if seen != nil) and if it returns false immediately return nil without invoking persistEvidence; otherwise call persistEvidence and proceed as before; alternatively (if you want retry-on-failure semantics) flip the logic so you only call seen.markSeen(...) after persistEvidence succeeds (i.e., persist, check error, and only then markSeen) — update the code paths around reportDoubleSign, persistEvidence, and seen.markSeen to implement one of these two behaviors so duplicate calls don’t swallow errors.
22-24: 💤 Low valueUse a typed
EvidenceSourceparameter to mirror the source constants.The
EvidenceSourceP2P,EvidenceSourceDA, andEvidenceSourceStoredconstants intypes/double_sign_evidence.goare untyped string constants. Instead of acceptingsource string, definetype EvidenceSource stringin the types package and update the function signature tosource EvidenceSource. This prevents arbitrary strings from being passed and makes the contract self-documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block/internal/syncing/doublesign.go` around lines 22 - 24, The doubleSignDetector type currently takes a plain string for source which allows arbitrary values; change it to use the typed EvidenceSource to mirror the existing constants (EvidenceSourceP2P, EvidenceSourceDA, EvidenceSourceStored) by adding type EvidenceSource string in the types package (where the constants are declared) and updating the doubleSignDetector signature from func(ctx context.Context, header *types.SignedHeader, source string) bool to func(ctx context.Context, header *types.SignedHeader, source types.EvidenceSource) bool and adjust all callers to pass the typed constants (e.g., types.EvidenceSourceP2P) so the API enforces the allowed source values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@block/internal/syncing/syncer.go`:
- Around line 1096-1097: Pending signed-header entries inserted via
SetPendingSignedHeader (syncer.go) are never evicted except by
RemovePendingSignedHeader or DeleteHeight, causing unbounded growth; modify the
cache manager backing s.cache to attach metadata (insertion height or timestamp)
for each pending header and implement an eviction policy (either TTL-based using
the timestamp or a height-window using the insertion/observed height) plus a
periodic cleanup routine that removes entries outside the allowed window; update
SetPendingSignedHeader to record the metadata, ensure RemovePendingSignedHeader
and DeleteHeight still remove entries, and expose configuration for
TTL/height-window so the periodic sweeper in the cache manager can prune
orphaned/far-future/invalid-fork headers.
---
Nitpick comments:
In `@block/internal/syncing/doublesign_syncer_integration_test.go`:
- Around line 78-79: The test manually initializes s.doubleSignSeen =
newDoubleSignDedup(), duplicating Start() setup and risking drift; add a new
initialization helper method (e.g., func (s *Syncer) initSyncWorkersHelper())
that contains all double-sign/detection-related initialization (initializing
doubleSignSeen and any future related fields), update Syncer.Start() to call
initSyncWorkersHelper(), and change the test to call s.initSyncWorkersHelper()
instead of directly assigning s.doubleSignSeen so test setup stays in sync with
production; reference the Syncer.Start and the doubleSignSeen field when making
these changes.
In `@block/internal/syncing/doublesign_test.go`:
- Around line 76-101: The test-only detector in makeDetectDoubleSign mirrors
Syncer.detectDoubleSign but may silently diverge if detectDoubleSign gains
side-effects (e.g., metrics); refactor the detection core used by
firstObservation → buildEvidence → persistEvidence into a shared helper function
(e.g., detectDoubleSignCore) that accepts a callback for "on detection"
behavior, then change Syncer.detectDoubleSign to call detectDoubleSignCore with
the production callback and update makeDetectDoubleSign to call the same core
with a test callback that captures evidence into e.capLock (and still calls
persistEvidence as needed) so both production and tests use identical detection
logic while only the reaction differs.
In `@block/internal/syncing/doublesign.go`:
- Around line 96-142: The function reportDoubleSign currently calls
persistEvidence unconditionally and then returns nil when seen.markSeen(...)
indicates a duplicate, which discards persistErr; change the ordering so
persistEvidence is only called when this is the first sighting: call
seen.markSeen(ev.Height, altHashStr) first (if seen != nil) and if it returns
false immediately return nil without invoking persistEvidence; otherwise call
persistEvidence and proceed as before; alternatively (if you want
retry-on-failure semantics) flip the logic so you only call seen.markSeen(...)
after persistEvidence succeeds (i.e., persist, check error, and only then
markSeen) — update the code paths around reportDoubleSign, persistEvidence, and
seen.markSeen to implement one of these two behaviors so duplicate calls don’t
swallow errors.
- Around line 22-24: The doubleSignDetector type currently takes a plain string
for source which allows arbitrary values; change it to use the typed
EvidenceSource to mirror the existing constants (EvidenceSourceP2P,
EvidenceSourceDA, EvidenceSourceStored) by adding type EvidenceSource string in
the types package (where the constants are declared) and updating the
doubleSignDetector signature from func(ctx context.Context, header
*types.SignedHeader, source string) bool to func(ctx context.Context, header
*types.SignedHeader, source types.EvidenceSource) bool and adjust all callers to
pass the typed constants (e.g., types.EvidenceSourceP2P) so the API enforces the
allowed source values.
In `@block/internal/syncing/syncer.go`:
- Around line 1085-1098: In Syncer.detectDoubleSign, don't cache the header when
firstObservation returns an error (or at minimum escalate the log) because an
intermittent store error can make the alternate become the recorded "first"
observation; change the control flow so that after firstObservation returns an
error you either return false immediately (skipping
s.cache.SetPendingSignedHeader) or explicitly avoid calling
SetPendingSignedHeader for that header, and also upgrade the warning to a
higher-severity log (or include context) so missed detection is obvious;
reference detectDoubleSign, firstObservation, SetPendingSignedHeader,
buildEvidenceFromPair, and handleDoubleSign when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc590e6a-0cf3-4783-9ddc-cc3ebaf06403
📒 Files selected for processing (12)
block/internal/syncing/da_retriever.goblock/internal/syncing/da_retriever_test.goblock/internal/syncing/doublesign.goblock/internal/syncing/doublesign_branches_test.goblock/internal/syncing/doublesign_syncer_integration_test.goblock/internal/syncing/doublesign_test.goblock/internal/syncing/p2p_handler.goblock/internal/syncing/p2p_handler_doublesign_test.goblock/internal/syncing/p2p_handler_test.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_forced_inclusion_test.goblock/internal/syncing/syncer_test.go
✅ Files skipped from review due to trivial changes (1)
- block/internal/syncing/syncer_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- block/internal/syncing/syncer_forced_inclusion_test.go
- block/internal/syncing/da_retriever_test.go
- block/internal/syncing/p2p_handler_test.go
- block/internal/syncing/p2p_handler_doublesign_test.go
| s.cache.SetPendingSignedHeader(header, source) | ||
| return false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look up the cache.Manager impl to find any eviction or bound on pending signed headers.
fd -t f -e go . | xargs rg -nP --type=go '(SetPendingSignedHeader|pendingSignedHeader|PendingSignedHeader)' -C3Repository: evstack/ev-node
Length of output: 26424
🏁 Script executed:
# Check for DeleteHeight usage and height pruning logic
rg -n 'DeleteHeight' --type=go -C 2Repository: evstack/ev-node
Length of output: 2699
🏁 Script executed:
# Check for any height rotation or cleanup logic in syncer
rg -n 'Prune|cleanup|evict' --type=go -i block/internal/syncing/Repository: evstack/ev-node
Length of output: 4824
🏁 Script executed:
# Check if there's a height window or finality-based cleanup mechanism
rg -n 'window|finality|prune' --type=go block/internal/cache/manager.go -i -C 2Repository: evstack/ev-node
Length of output: 41
🏁 Script executed:
# Find all places DeleteHeight is called to understand the eviction triggers
rg -n 'DeleteHeight\(' --type=go -B 5 -A 2Repository: evstack/ev-node
Length of output: 3213
🏁 Script executed:
# Look for when headers are marked as "da included" or persisted
rg -n 'DAIncluded|HeaderDAIncluded' --type=go block/internal/cache/ block/internal/syncing/syncer.go -C 2Repository: evstack/ev-node
Length of output: 28478
🏁 Script executed:
# Look for comments around SetPendingSignedHeader that explain lifecycle
rg -n 'SetPendingSignedHeader|pending.*header' --type=go block/internal/syncing/syncer.go -B 3 -A 3 | head -60Repository: evstack/ev-node
Length of output: 206
🏁 Script executed:
# Check if there's a height window or validity constraint that bounds headers
rg -n 'MaxBlockAge|ValidateHeader|window|future.*height' --type=go block/internal/syncing/syncer.go -i | head -30Repository: evstack/ev-node
Length of output: 465
Add an eviction policy to the pending signed-header cache.
SetPendingSignedHeader is called for every non-equivocating header observation (syncer.go:1096). Currently, entries are removed only after successful sync (RemovePendingSignedHeader at line 807) or when the height is DA-included (DeleteHeight). Headers that never sync successfully—orphans, far-future heights, headers from invalid forks—accumulate indefinitely since the cache is an unbounded map with no TTL or LRU eviction. Implement a cleanup policy (TTL-based or height-window-based) in the cache manager.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@block/internal/syncing/syncer.go` around lines 1096 - 1097, Pending
signed-header entries inserted via SetPendingSignedHeader (syncer.go) are never
evicted except by RemovePendingSignedHeader or DeleteHeight, causing unbounded
growth; modify the cache manager backing s.cache to attach metadata (insertion
height or timestamp) for each pending header and implement an eviction policy
(either TTL-based using the timestamp or a height-window using the
insertion/observed height) plus a periodic cleanup routine that removes entries
outside the allowed window; update SetPendingSignedHeader to record the
metadata, ensure RemovePendingSignedHeader and DeleteHeight still remove
entries, and expose configuration for TTL/height-window so the periodic sweeper
in the cache manager can prune orphaned/far-future/invalid-fork headers.
2 participants
Overview
Closes #1672
This change introduces detection of double-signing (equivocation) at the same block height during synchronization from DA and P2P sources.
Previously, synchronization processed the first observed block per height and silently ignored subsequent conflicting headers. This PR adds detection and handling for conflicting headers to surface potential sequencer equivocation.
Behavior
For each incoming
SignedHeaderfrom DA or P2P, the syncer enforces equivocation detection at that block height.ds/<height>/<alt-hash>.Changes
(height, alternative hash)to avoid duplicate reporting.ValidateBasicprior to detection to prevent invalid data from being recorded as evidence.double_signs_detected_totalPrometheus counter metric.Summary by CodeRabbit
New Features
Chores
Tests