feat(syncing): detect sequencer double-signs and halt with persisted …#3310
Open
CaelRowley wants to merge 3 commits intoevstack:mainfrom
Open
feat(syncing): detect sequencer double-signs and halt with persisted …#3310CaelRowley wants to merge 3 commits intoevstack:mainfrom
CaelRowley wants to merge 3 commits intoevstack:mainfrom
Conversation
Contributor
📝 Walkthrough🚥 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}
Contributor
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
Comment on lines
+22
to
+29
| type DoubleSignEvidence struct { | ||
| Height uint64 | ||
| FirstHeader *SignedHeader | ||
| AlternateHeader *SignedHeader | ||
| DetectedAt time.Time | ||
| FirstSource string | ||
| AlternateSource string | ||
| } |
Contributor
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.
Suggested change
| 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.
Contributor
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
Comment on lines
+54
to
+74
| func (e *DoubleSignEvidence) ToProto() (*pb.DoubleSignEvidence, error) { | ||
| if e == nil { | ||
| return nil, errors.New("evidence is nil") | ||
| } | ||
| first, err := e.FirstHeader.ToProto() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal first header: %w", err) | ||
| } | ||
| alt, err := e.AlternateHeader.ToProto() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal alternate header: %w", err) | ||
| } | ||
| return &pb.DoubleSignEvidence{ | ||
| Height: e.Height, | ||
| FirstHeader: first, | ||
| AlternateHeader: alt, | ||
| DetectedAt: e.DetectedAt.UnixNano(), | ||
| FirstSource: e.FirstSource, | ||
| AlternateSource: e.AlternateSource, | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
ToProto will panic if FirstHeader or AlternateHeader is nil.
The nil guard at Line 55 only checks e == nil. If either header field is nil (e.g., when ToProto or MarshalBinary is called without prior ValidateBasic), e.FirstHeader.ToProto() at Line 58 (and e.AlternateHeader.ToProto() at Line 62) will panic at runtime.
🛡️ Proposed fix
func (e *DoubleSignEvidence) ToProto() (*pb.DoubleSignEvidence, error) {
if e == nil {
return nil, errors.New("evidence is nil")
}
+ if e.FirstHeader == nil || e.AlternateHeader == nil {
+ return nil, errors.New("evidence requires both first and alternate headers")
+ }
first, err := e.FirstHeader.ToProto()📝 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
| func (e *DoubleSignEvidence) ToProto() (*pb.DoubleSignEvidence, error) { | |
| if e == nil { | |
| return nil, errors.New("evidence is nil") | |
| } | |
| first, err := e.FirstHeader.ToProto() | |
| if err != nil { | |
| return nil, fmt.Errorf("marshal first header: %w", err) | |
| } | |
| alt, err := e.AlternateHeader.ToProto() | |
| if err != nil { | |
| return nil, fmt.Errorf("marshal alternate header: %w", err) | |
| } | |
| return &pb.DoubleSignEvidence{ | |
| Height: e.Height, | |
| FirstHeader: first, | |
| AlternateHeader: alt, | |
| DetectedAt: e.DetectedAt.UnixNano(), | |
| FirstSource: e.FirstSource, | |
| AlternateSource: e.AlternateSource, | |
| }, nil | |
| } | |
| func (e *DoubleSignEvidence) ToProto() (*pb.DoubleSignEvidence, error) { | |
| if e == nil { | |
| return nil, errors.New("evidence is nil") | |
| } | |
| if e.FirstHeader == nil || e.AlternateHeader == nil { | |
| return nil, errors.New("evidence requires both first and alternate headers") | |
| } | |
| first, err := e.FirstHeader.ToProto() | |
| if err != nil { | |
| return nil, fmt.Errorf("marshal first header: %w", err) | |
| } | |
| alt, err := e.AlternateHeader.ToProto() | |
| if err != nil { | |
| return nil, fmt.Errorf("marshal alternate header: %w", err) | |
| } | |
| return &pb.DoubleSignEvidence{ | |
| Height: e.Height, | |
| FirstHeader: first, | |
| AlternateHeader: alt, | |
| DetectedAt: e.DetectedAt.UnixNano(), | |
| FirstSource: e.FirstSource, | |
| AlternateSource: e.AlternateSource, | |
| }, 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 `@types/double_sign_evidence.go` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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