-
Notifications
You must be signed in to change notification settings - Fork 202
Description
Problem Definition
Cluster and consensus instances share the same storage.Headers interface for retrieving headers. This means that, given a block ID, a component may read a cluster header or a consensus header without error. This makes bugs and attacks more likely by requiring components to check that stored headers are for the correct chain.
An additional source of confusion is that storage.Headers provides methods ByHeight and BlockIDByHeight which return only consensus blocks (the storage module is hooked up only to the consensus height index), but this storage.Headers interfaces is nevertheless used in contexts where it expects to only interact with cluster blocks. These methods should use the cluster height index when Headers is bound to a cluster chain ID.
Finally, Headers includes methods related to chunk ID indexing (IndexByChunkID, etc.). These are irrelevant to cluster header storage. (They are also relevant to all node roles but Execution Nodes, even for consensus headers.) Minimally these should fail in a predictable and documented way if used in the wrong context (cluster chain). We should consider splitting them into a separate EN-only module as well.
Proposed Solution
- Bind a chain ID to each
storage.Headersinstance at construction. - Modify
storage.Headersto operate only on headers with a particular chain ID. It should return a sentinel error if it is queried with a block ID for the wrong type of header - Modify
storage.Headersto use the appropriate height index, depending on the chain ID the instance is bound to.
Definition of Done
- retrieve cluster header from
Headersinstance bound to consensus chain ID -> should fail with documented sentinel error - retrieve consensus header from
Headersinstance bound to cluster chain ID -> should fail with documented sentinel error - look up header by block height from
Headersinstance bound to consensus and cluster chain ID -> should use the appropriate height index - Add test case: the builder should reject a transaction which has a reference block ID targeting a cluster block
- Re-enable test
TestExtend_WithReferenceBlockFromClusterChain, should pass - Address this TODO:
flow-go/state/cluster/badger/mutator.go
Lines 251 to 252 in 3bdba3b
// TODO ensure the reference block is part of the main chain https://github.com/onflow/flow-go/issues/4204 _ = refBlock - Test that empty collections must have
ReferenceBlockID=ZeroID