Add PoS block seal indexing and duplicate seal evidence retention - #2121
Conversation
A block's seal is the (stake pool, VRF output) pair that identifies the block-producing opportunity claimed by a pool for a given slot. The VRF output is used for the seal identity instead of the VRF proof, because unlike the output, the proof bytes are not deterministic across signings of the same transcript.
Two new schema maps: the seal index (seal to the blocks known to carry it, bounded per seal by the indexing logic) and the append-only evidence records for seals seen on more than one block. The evidence records retain the signed headers, so they stay verifiable even if the blocks are later removed from storage.
Every checked PoS block gets its seal indexed, so the index covers all branches, not only the best chain. When a seal is seen on more than one block, the signed headers of the known blocks are retained as a self-certifying evidence record, and the index entry of the seal is extended up to a conservative cap. Enabled by default and configurable via the chainstate config; no block validation behavior is affected.
Two integration tests: a seal seen on two valid sibling blocks records evidence while both blocks stay fully valid, and disabling the tracking records nothing. The shared stake pool test setup is factored into the test helpers, and the storage simulation test now replays the seal indexing for the manually assembled reference storage, matching the storage footprint of a processed block.
Once a seal's index entry reaches its cap, new sightings are only logged: neither the index entry nor the evidence records are extended anymore, so the storage footprint of a single reused seal stays bounded. Already recorded evidence is never removed. A missing header of an indexed block is now logged as a warning instead of silently writing an incomplete evidence record, and the duplicate seal evidence map gets its deletion counterpart for future tooling.
…exing on the config
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| evidence.push_header(block.header().clone()); | ||
|
|
||
| log::info!( | ||
| "A PoS seal of pool {} was seen on more than one block; recorded evidence for block {}", |
There was a problem hiding this comment.
If every known block header is missing, this still writes an "evidence" record containing only the new block's header. A single signed header proves nothing about duplication, yet it will be persisted as DuplicateSealEvidence. Consider skipping the write (or returning an error) when no corroborating headers were collected, so the evidence store only holds meaningful records.
Suggestion:
| evidence.push_header(block.header().clone()); | |
| log::info!( | |
| "A PoS seal of pool {} was seen on more than one block; recorded evidence for block {}", | |
| evidence.push_header(block.header().clone()); | |
| if evidence.headers().len() < 2 { | |
| log::warn!( | |
| "No corrobating headers available for the duplicate seal of pool {}; no evidence recorded", | |
| seal.pool_id(), | |
| ); | |
| return Ok(()); | |
| } | |
| log::info!( | |
| "A PoS seal of pool {} was seen on more than one block; recorded evidence for block {}", |
There was a problem hiding this comment.
Fixed in 506455a: the evidence record is now only written when it retains at least one header besides the new block's (headers().len() >= 2); otherwise a warning is logged and nothing is stored. The block is still indexed, so a later sighting can fill in the missing headers. Covered by the new evidence_is_not_recorded_without_corroborating_headers test.
| let vrf_output = match pos_data.vrf_data() { | ||
| VRFReturn::Schnorrkel(vrf_data) => vrf_data.vrf_preout().into(), | ||
| }; |
There was a problem hiding this comment.
The seal is keyed by the raw VRF preout, whose transcript includes the block timestamp (construct_transcript attaches block_timestamp.as_int_seconds()). Since the seal never records the transcript/timestamp, a pool can trivially evade duplicate detection by re-signing the same slot draw with a ±1-second timestamp, producing a distinct VRF output and thus a distinct seal. This is acknowledged in the module docs, but consider whether the slot identity (epoch + slot index derived from consensus rules) should be part of the seal instead of the raw output, otherwise the slasher evidence can be bypassed by a careless/deliberate adversary at essentially zero cost.
There was a problem hiding this comment.
The module docs already acknowledged this; 506455a extends them to state explicitly that closing the gap would have to be anchored in the consensus rules. On the design question: keying the seal by the VRF output is equivalent in detection power to keying it by (pool, epoch, slot). The VRF output is deterministic over the transcript, which commits to (epoch index, sealed epoch randomness, block timestamp in whole seconds) for the pool's key (construct_transcript), so two blocks get the same seal iff they carry the same (pool, epoch, randomness, timestamp-second) — regardless of how the randomized VRF proof bytes differ. A ±1-second timestamp shift produces a different transcript, i.e. under the consensus rules — which operate at one-second slot granularity — it is a different slot draw, not the same draw re-signed, and the shifted draw has to satisfy the staking target check on its own. Detecting that case would require the consensus rules to define a coarser slot identity and validate blocks against it, which is a consensus change and out of scope for this additive, non-consensus index. If maintainers want that stricter notion of slot identity, it should be a follow-up consensus-level proposal.
Re-key the duplicate seal evidence map from the block id to the seal, so a reused seal is covered by a single extendable record instead of a fresh copy of the known headers per sighting. Only retain headers whose consensus data carries the seal under test, and skip writing an evidence record without at least one corroborating header, so the store only holds meaningful records. Gate the seal indexing in a single helper (index_block_seal_if_enabled) shared by the integration path and the test-suite storage replication, and drop the internal indexing function from the crate root export.
|
Addressed all 6 review findings in 506455a:
Storage-compatibility note on the evidence-map re-key: the block-id-keyed layout only ever existed in commits of this unmerged PR — the seal tables are absent from master, whose Verified: |
| // The index entry of the seal is at its cap, so the seal reuse is already | ||
| // covered by the recorded evidence. Log the sighting, but do not extend the | ||
| // index and do not record redundant evidence, keeping the storage footprint | ||
| // of a single reused seal bounded. | ||
| log::info!( |
There was a problem hiding this comment.
When the seal index entry is at MAX_BLOCKS_PER_SEAL, the sighting is dropped without checking that evidence for the seal actually exists. If earlier sightings took the 'no corroborating headers' path (headers unavailable), no evidence was recorded, and once the cap is reached no later sighting can ever produce evidence — silently weakening the duplication-detection guarantee. Consider recording evidence here too (at least if none exists yet), or verifying the evidence record exists before skipping.
There was a problem hiding this comment.
Fixed in 10b4ac1: a sighting of a seal whose index entry is at the cap now checks whether an evidence record exists and backfills it if none does (earlier sightings may have failed to collect corroborating headers, and past the cap no sighting would ever fill it in). The backfilled record is bounded by the index entry (at most one header per known block plus the new one), and once it exists the sightings are only logged again, so the storage footprint of a single reused seal stays bounded. Covered by the evidence_is_backfilled_when_the_cap_is_reached_without_evidence and evidence_is_backfilled_with_the_known_headers_at_the_cap tests.
| chainstate_ref | ||
| .index_block_seal_if_enabled(block, block_index.block_height()) | ||
| .map_err(BlockIntegrationError::OtherNonValidationError)?; |
There was a problem hiding this comment.
A failure in the auxiliary seal-indexing step is propagated as OtherNonValidationError, which aborts block integration entirely (attempt_to_process_block returns Err immediately). Seal duplication tracking is an observational/monitoring feature, not consensus-critical; an indexing error (e.g. storage issue in the seal tables) here would make the node reject otherwise valid blocks, harming liveness. Consider logging the failure and continuing block integration instead, so seal tracking never blocks consensus processing.
Suggestion:
| chainstate_ref | |
| .index_block_seal_if_enabled(block, block_index.block_height()) | |
| .map_err(BlockIntegrationError::OtherNonValidationError)?; | |
| // Seal indexing is observational; its failure must not block block integration. | |
| if let Err(err) = | |
| chainstate_ref.index_block_seal_if_enabled(block, block_index.block_height()) | |
| { | |
| log::error!( | |
| "Failed to index the seal of block {}: {err}", | |
| block.get_id() | |
| ); | |
| } |
There was a problem hiding this comment.
Agreed — tracking is observational and feeds no validation rule. Fixed in 10b4ac1: an indexing failure is now logged (with the block id) and the block integration continues, so a storage issue in the seal tables cannot reject otherwise valid blocks. The indexing of the affected block is lost, which the error log makes visible.
| /// Remove the seal index entry of the given seal. | ||
| /// | ||
| /// Currently unused: it is the deletion counterpart of the map, to be used by | ||
| /// the pruning of the entries whose blocks fell out of the reorg range. | ||
| fn del_seal_index_entry(&mut self, seal: &BlockSeal) -> Result<()>; |
There was a problem hiding this comment.
Seal index entries and evidence records are never removed: every processed PoS block adds one entry, so storage grows monotonically with the total number of processed blocks, including blocks on orphaned branches that never contributed to the chain. The trait method docs mention future pruning 'out of the reorg range', but no mechanism exists in this change. Even with per-seal capping, a long-lived node accumulates one seal entry per PoS block indefinitely. Worth an explicit tracking item or a pruning strategy before merging.
There was a problem hiding this comment.
Tracked: the growth concern, the unused deletion counterparts and the candidate strategies (reorg-range pruning vs a finality/epoch-based retention window that keeps the evidence records permanently vs documenting the accepted growth) are now captured in #2123, referenced from the trait docs of both deletion methods. Implementing a pruning strategy in this PR would drag consensus-adjacent retention decisions into what is meant to be an additive, non-consensus change, so it is deliberately left to the follow-up.
A sighting of a seal whose index entry is at the cap now backfills the evidence record if none exists, since the earlier sightings below the cap may have failed to collect corroborating headers and no later sighting would fill it in; the record stays bounded by the index entry. A failure of the observational seal indexing no longer rejects an otherwise valid block: it is logged and the block integration continues, so the seal tracking cannot harm the liveness of the node. The monotonic growth of the seal tables is tracked by #2123.
Document the effective default of the seal tracking config option and the storage coupling of the seal value encodings, pass the already loaded evidence record to the recording helper instead of fetching it twice, and drop the log_error attribute that logged the same indexing failure twice (the caller logs it with the block id).
|
Addressed the second review round (10b4ac1, 22e47f6):
Additionally, from a local re-review of the branch: the config template now documents the effective default ( Verified: |
| /// Store for PoS seal index entries, i.e. the blocks known to carry each seal. | ||
| /// The number of blocks per entry is bounded by the seal indexing logic. | ||
| /// Note: nodes that upgrade from a storage version without this map start with | ||
| /// an empty index; the index only covers the blocks processed after the upgrade. | ||
| pub DBSealIndex: Map<BlockSeal, SealIndexEntry>, |
There was a problem hiding this comment.
New DBSealIndex and DBDuplicateSealEvidence maps are added to the schema, but ChainstateStorageVersion::CURRENT is still Self(11) (chainstate/storage/src/internal/version.rs is unchanged), so nodes that upgrade will open their existing databases with these maps empty. The comments acknowledge that the index only covers post-upgrade blocks, meaning seal-duplication detection silently misses all pre-upgrade seals. If a partially-indexed history is an accepted trade-off, consider documenting it at the version/migration level (or bumping the version / adding a re-index flag); otherwise duplicated pre-upgrade seals would go unreported.
There was a problem hiding this comment.
Documented at the version level in f2f155f: ChainstateStorageVersion::CURRENT now notes that the seal maps start empty on upgraded nodes and only cover the blocks processed since, like the schema docs. A version bump or a re-index flag was deliberately avoided: the index is observational, and backfilling it would require replaying the whole chain (or keeping a separate catch-up mechanism), which is disproportionate for a duplication-detection aid — and pre-upgrade seals that get reused on new blocks are still detected from their new sightings onward.
| #[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)] | ||
| pub struct SealIndexEntry { | ||
| seal: BlockSeal, | ||
| blocks: Vec<(Id<Block>, BlockHeight)>, | ||
| } |
There was a problem hiding this comment.
The module docs on SealIndexEntry and DuplicateSealEvidence (and the schema docs) correctly note that any change to their encoding makes previously written records unreadable, and the key (BlockSeal) encoding is pinned by seal_encoding_is_stable. However, the value encodings are not pinned by any test: SealIndexEntry (Vec<(Id, BlockHeight)>) and DuplicateSealEvidence (Vec) depend on the serialization of Id, BlockHeight, and SignedBlockHeader, which could change upstream. Add roundtrip/stable-encoding tests for these two value types, mirroring seal_encoding_is_stable, so an accidental encoding change is caught at review/CI time.
Suggestion:
| #[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)] | |
| pub struct SealIndexEntry { | |
| seal: BlockSeal, | |
| blocks: Vec<(Id<Block>, BlockHeight)>, | |
| } | |
| #[test] | |
| fn seal_index_entry_codec_roundtrip() { | |
| let (_, seal) = make_seal(0, H256::zero()); | |
| let entry = SealIndexEntry::new(seal, vec![(Id::new(H256::from([3u8; 32])), BlockHeight::new(7))]); | |
| let decoded = SealIndexEntry::decode(&mut &entry.encode()[..]).unwrap(); | |
| assert_eq!(decoded, entry); | |
| } | |
| #[test] | |
| fn duplicate_seal_evidence_codec_roundtrip() { | |
| // ... analogous roundtrip over DuplicateSealEvidence with a signed header | |
| } |
There was a problem hiding this comment.
Added in f2f155f: seal_index_entry_codec_roundtrip and duplicate_seal_evidence_codec_roundtrip pin the decodability of both value types (the evidence one carries real signed headers, one per signature variant), mirroring the key's seal_encoding_is_stable/seal_codec_roundtrip pair. Upstream encoding drift of the embedded SignedBlockHeader would affect the whole block table before these maps, so the roundtrip plus the storage-coupling notes felt like the right calibration.
Document at the storage version that the seal maps start empty on the upgraded nodes and only cover the blocks processed since, and pin the encodings of the seal index entry and the duplicate seal evidence with roundtrip tests, like the encoding of the seal key.
| /// Note: the PoS seal maps introduced alongside this version start empty on | ||
| /// the nodes that upgrade, so the seal duplication tracking only covers the | ||
| /// blocks processed after the upgrade and does not backfill the history | ||
| /// (see the schema docs). | ||
| pub const CURRENT: Self = Self(11); |
There was a problem hiding this comment.
The two new maps (DBSealIndex, DBDuplicateSealEvidence) are added to the schema without bumping ChainstateStorageVersion. This means upgraded nodes silently start with empty seal indexes, and duplicate-seal detection (if it feeds consensus-relevant behavior later) will miss seals from pre-upgrade blocks — fresh and upgraded nodes will diverge in what they consider a duplicate seal. The comment documents the trade-off, which is acceptable only as long as the index is advisory/metadata only; if seal duplication ever gates block acceptance, a version bump with backfill (or consensus-rule anchoring) will be required. Consider adding an explicit assertion/test that no consensus check depends on the seal maps, so the constraint is enforced rather than just described.
There was a problem hiding this comment.
The constraint is now spelled out in the version note (6f48cfc): the seal maps must remain advisory, and a storage version bump with a backfill (or a consensus-rule anchoring of the seals) becomes mandatory if they ever gate block acceptance. An enforcing test would only re-encode the module structure: the index functions are private to the seal index module, their results are ignored by the only production caller, and the storage reads are reachable only through them — the property is architectural, and the note makes the obligation explicit for whoever touches it next.
| /// Store for duplicate PoS seal evidence records, keyed by the seal itself, | ||
| /// so a reused seal is covered by a single, extendable evidence record. | ||
| /// Note: like the seal index, this map starts empty on upgraded nodes. | ||
| pub DBDuplicateSealEvidence: Map<BlockSeal, DuplicateSealEvidence>, |
There was a problem hiding this comment.
DBDuplicateSealEvidence is keyed by seal and its records are never removed (del_duplicate_seal_evidence is documented as unused), while seal index entries are only pruned in a follow-up (#2123). Both maps grow monotonically with chain history: every newly seen seal adds a row, and evidence records additionally embed full signed headers, so their size can be substantial. Combined with the same storage version (so there is no reset path for existing nodes), this gives unbounded write amplification and storage growth on long-running nodes. Since the deletion APIs already exist, consider at least wiring the reorg-path deletion or documenting a concrete bound for the evidence map, rather than leaving growth fully unbounded until #2123.
There was a problem hiding this comment.
The concrete bounds are now documented in the schema (6f48cfc): DBSealIndex grows by roughly one entry per processed PoS block — proportionally to the chain history, like the other per-block tables — and its pruning is what #2123 is about. DBDuplicateSealEvidence is bounded differently: a record exists only for a seal that was actually reused (expected none on an honest network, at most one bounded record per reused seal) and is kept permanently on purpose — it is the self-certifying evidence of the duplication, so wiring a reorg-path deletion would defeat it (an equivocation attempt on an orphaned branch is exactly what the evidence should retain).
The storage version notes that the seal maps must remain advisory, with a version bump and a backfill required before they could gate block acceptance, and the schema docs spell out the concrete growth: the index grows proportionally to the chain history like the other per-block tables, while the evidence records exist only for the actually reused seals and are kept permanently.
2 participants
Every checked PoS block gets its seal (stake pool id + VRF output) indexed, bounded per seal, covering all branches. When the same seal is seen on more than one block, the signed headers are retained as self-certifying evidence records. Additive only: no consensus, validation, or p2p behavior changes; configurable via chainstate config (default on).