Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rigorously define "canonical chain" and ensure correct maintenance and use of canonical chain #8154

Open
robin-near opened this issue Dec 2, 2022 · 0 comments
Assignees

Comments

@robin-near
Copy link
Contributor

Right now, canonical chain is updated using the header_head (which is separate from "head"); we should consider defining canonical chain as the chain of the "head" instead, as that is the head used for consensus. Throughout the codebase there are some inconsistencies of how the canonical chain reverse mappings (height or ordinal -> block) are used; we should make sure each case is correct and fix those that are not.

Columns maintained for canonical chain reverse mapping, and a survey of their use cases:

  • DBCol::BlockHeight (height to block hash or None)
    • Used by mark_block_as_challenged to detect whether a block being challenged is on the canonical chain or not (however, this logic appears incorrect because it is using the result to determine whether to update the head, not the header head, yet this storage field is maintained using the header head; that is an inconsistency)
    • Used by check_transaction_validity_period, apparently to check whether the reference base block is on the canonical chain and if so apply a simpler validity period check.
    • Used by get_earliest_block_hash to find the smallest height that corresponds to a block.
    • Used by debug get_validator_status; although here I think we don’t need to use this call; we already have the block hash
    • Also indirectly via get_block_header_by_height
      • Used by Chain::create_light_client_block, querying two blocks ahead
      • Used by Chain::find_common_header to query whether a block hash is on the canonical chain
      • Used by Chain::retrieve_headers to answer header sync
      • Used by Client::collect_block_approval to look up the block specified by Skip (this is incorrect and is being fixed)
      • Used by ViewClient::maybe_block_id_to_block_header, and get_block_header_by_reference
      • Used by HeaderSync::get_locator
  • DBCol::NextBlockHashes (block hash to next block hash)
    • Used by Chain::get_next_block_hash_with_new_chunk which is used by GetExecutionOutcome rpc call.
    • Used by create_light_client_block_view
    • Used by block_sync to traverse the skeleton to figure out the next range of blocks to download
    • Used by GetValidatorInfo rpc
  • DBCol::BlockOrdinal (block ordinal to block hash)
    • Only used by get_merkle_tree_node, which is used by get_block_proof, which is needed by an rpc call. get_block_proof takes the head block and also a past block to construct a proof for the past block in the head block’s merkle tree, and in order to construct the tree, some other nodes in the tree need to be queried (subtrees that are complete, and leaf nodes otherwise), and these queries are based on the tree node index, which is the same as the block ordinal, and that’s why we need the ordinal → block hash query. This implicitly assumes that the query will only be performed on the canonical chain - which is not necessarily a correct assumption since the head hash is provided by the caller.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant