fix backward incompatibility - #7827
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses backward-incompatible bootstrap data by ensuring “self-notarized” headers are not left stale (nonce 0) after bootstrapping from storage, and by removing the previous “stale self-notarized headers” compatibility path from the meta processor.
Changes:
- Extend the storage bootstrapper interface with
completeSelfNotarizedHeaders()and invoke it after applying boot infos. - Implement missing self-notarized header derivation for metachain bootstrap by looking back through stored meta blocks and referenced shard headers.
- Remove stale-self-notarized detection/bypass logic from
metaProcessorand add a best-effort completion step duringCommitBlock.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| process/sync/storageBootstrap/interface.go | Adds completeSelfNotarizedHeaders() to the bootstrapper contract. |
| process/sync/storageBootstrap/baseStorageBootstrapper.go | Calls completeSelfNotarizedHeaders() after applying boot infos. |
| process/sync/storageBootstrap/metaStorageBootstrapper.go | Implements lookback-based derivation of missing per-shard self-notarized headers. |
| process/sync/storageBootstrap/shardStorageBootstrapper.go | Adds a no-op implementation to satisfy the new interface method. |
| process/block/metablock.go | Removes stale-header bypass logic; adds completion of missing self-notarized headers and adjusts logging/flow. |
| process/block/export_test.go | Removes exported test hooks related to the removed stale-header mechanism. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(missingShards) > 0 { | ||
| log.Warn("completeSelfNotarizedHeaders: could not derive all self-notarized headers", | ||
| "numStillMissing", len(missingShards)) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
completeSelfNotarizedHeaders always returns nil even when missingShards is still non-empty (it only logs a warning). Given later meta processing now relies on non-stale self-notarized headers, this can let the node continue bootstrapping in a broken state and fail later in verifyShardDataAgainstHeaders. Consider returning a non-nil error when not all shards could be derived (and/or when loading the metablock fails), so the caller can abort bootstrap or trigger a safer fallback.
| errComplete := st.bootstrapper.completeSelfNotarizedHeaders(bootInfos[0].LastHeader.Hash) | ||
| if errComplete != nil { | ||
| log.Warn("could not complete self notarized headers", "error", errComplete.Error()) |
There was a problem hiding this comment.
applyBootInfos logs and continues when completeSelfNotarizedHeaders fails. If completing self-notarized headers is required for post-bootstrap correctness, continuing here can lead to hard-to-diagnose failures later during block processing. Consider propagating this error (return it from applyBootInfos) or otherwise making the bootstrap outcome explicit (e.g., fail fast or fall back to a compatibility mode).
| errComplete := st.bootstrapper.completeSelfNotarizedHeaders(bootInfos[0].LastHeader.Hash) | |
| if errComplete != nil { | |
| log.Warn("could not complete self notarized headers", "error", errComplete.Error()) | |
| err = st.bootstrapper.completeSelfNotarizedHeaders(bootInfos[0].LastHeader.Hash) | |
| if err != nil { | |
| log.Debug("could not complete self notarized headers", "error", err.Error()) | |
| return err |
| ) | ||
| if errGet != nil { | ||
| log.Trace("getLastSelfNotarizedHeaderByShard.GetMetaHeader", "error", errGet.Error()) | ||
| log.Warn("getLastSelfNotarizedHeaderByShard: could not get referenced meta header, self notarized may not be updated", |
There was a problem hiding this comment.
This log was promoted from Trace to Warn inside a loop over referenced meta hashes. If referenced meta headers are temporarily missing from pool/storage during normal operation or bootstrap, this can generate a large volume of warnings. Consider keeping it at Debug/Trace or adding rate-limiting/sampling, and reserve Warn for cases that are expected to break processing.
| log.Warn("getLastSelfNotarizedHeaderByShard: could not get referenced meta header, self notarized may not be updated", | |
| log.Debug("getLastSelfNotarizedHeaderByShard: could not get referenced meta header, self notarized may not be updated", |
| lastSelfNotarizedHeader, _, err := mp.blockTracker.GetLastSelfNotarizedHeader(shardHdr.GetShardID()) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
verifyShardDataAgainstHeaders assumes GetLastSelfNotarizedHeader returns a non-nil header whenever err == nil, then dereferences it. Some test stubs (and potentially future implementations) return (nil, nil, nil), which would panic here. Consider explicitly checking for a nil header (even when err == nil) and returning a meaningful error instead of panicking.
| } | |
| } | |
| if lastSelfNotarizedHeader == nil { | |
| return fmt.Errorf("nil last self notarized header for shard %d", shardHdr.GetShardID()) | |
| } |
| mp.completeMissingSelfNotarizedHeaders(header) | ||
|
|
There was a problem hiding this comment.
New behavior backfills missing per-shard self-notarized headers via storage lookback (completeMissingSelfNotarizedHeaders). Please add/extend unit tests to cover at least: (1) bootstrapped state where some shards have nonce 0 self-notarized headers and the method successfully derives them; (2) lookback exhaustion / missing storage data path, asserting the intended outcome (error vs warning + continue).
| if errGet != nil { | ||
| continue |
There was a problem hiding this comment.
should we set hadLoadErrors = true also on this path?
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?