### `BlockId` refactoring status and next steps 1. Some of core traits are already cleaned from `BlockId`, however there are still some methods that are related to `BlockId` or *block number*. The summary of use cases is in the sections to follow. 1. Reworking `header` function led to some call sequences that are far from being perfect, [example](https://github.com/paritytech/substrate/pull/12874/commits/d06359cab8e6eb78d625b7520b37496252139a22). Namely, in some modules there is a requirement to fetch `header` using _block number_, current [PR](https://github.com/paritytech/substrate/pull/12874) works around this by calling `header(hash(n))` combo. 1. In some cases `expect_header(block_id)` is called to hide the combo, (as `expect_header` was not reworked yet) 1. To solve this _"problem"_ `HeaderBackend` trait could be extended with dedicated functions for fetching header by _block number_: -- `header_for_number(NumberFor<Block>) -> Result<Option<Block::Header>>` -- `expect_header_for_number(NumberFor<Block>) -> Result<Block::Header>` -- `header(Block::Hash) -> Result<Option<Block::Header>>` -- `expect_header(Block::Hash) -> Result<Block::Header>` New naming may also make finding all block-number usages and their refactorization easier. I would follow with this naming convention in needed for the rest of methods (e.g. `block`, `block_status`) Implementation of `_for_number` methods could make use of new _service_ which will move number-hash mapping out of DB. ### Where the `BlockId` is currently used: #### Still in _client_ API: - trait `Chain` _common block validation for consensus_ -- `block_status` - trait `BlockIdTo` -- `to_hash` -- `to_number` - trait `HeaderBackend`: -- `status` -- `expect_header` -- `block_hash_from_id` -- `block_number_from_id` - trait `BlockBackend` -- `block` -- `block_status` - impl `Client`: `Chain`, `BlockBackend`, `CallApiAt`, `HeaderBackend`, `BlockIdTo`, `BlockBuilderProvider` -- `code_at` -- `runtime_version_at` -- `block_status` - trait `ExecutionExtensions` -- `manager_and_extensions` -- `extensions` #### In `runtime_api()` _subsystem_: Refactoring here is probably doable, requires some adjustments in _runtime-related_ macros. Question if we want to get rid of `BlockId` parameter to runtime functions. This maybe inconvenient in some cases. Maybe we should introduce dual functions (following `_for_number` approach) at the top-level runtime API. - trait `GetRuntimeVersionAt` -- `runtime_version` - trait `ApiExt` -- `has_api` -- `has_api_with` -- `api_version` - trait `CallApiAt` -- `runtime_version_at` -- `state_at` - `WasmSubstitute` -- `matches` _(operates on block number)_ -- `get` - trait `CallExecutor` -- `prove_execution` -- `contextual_call` -- `call` -- `runtime_version` - trait `LocalCallExecutor` -- `check_override` #### Other usages - Benchmark::run (from - to) -- `measure_block` -- `consumed_weight` - impl `PowBlockImport` -- `check_inherents` - trait `PowAlgorithm` -- `verify` - DB Something that we want to get rid of: -- API: `remove_from_db`, `read_header`, `read_db` -- `HeaderBackend::status` -- internal implementation `Backend::prune_block` - InMem: -- `Blockchain::id` (blockid -> hash) -- `HeaderBackend::status` - `AuraVerifier` -- `check_inherents` (_runtime_api_ called) - `BabeVerifier` -- `check_inherents` (_runtime_api_ called) -- `check_and_report_equivocation` - `Beefy` -- `expect_validator_set` - `BlockBuilderProvider` -- `new_block` -- `new_block_at` - BasicAuthorship `Proposer` -- impl uses `BlockId` - `BlockRequestHandler` -- `handle_request` / `get_block_response` #### Network tests utils - `PeerClient` -- `has_state_at` - `Peer` -- `generate_blocks_at` -- `push_blocks_at` -- `push_blocks_at_without_informing_sync` -- `push_blocks_at_without_announcing` -- `generate_tx_blocks_at` #### utils - impl `BlockNumberOrHash` -- `parse` - `pub fn check_block` ### Where _Block number_ is used _not completed yet_ - `have_state_at(hash, number)` - `hash(n)` usages ``` client/consensus/babe/src/lib.rs 1775 pub fn revert client/db/src/lib.rs fn prune_blocks: 1753 fn prune_displaced_branches client/finality-grandpa/src/warp_proof.rs|100 WarpSyncProof::generate client/finality-grandpa/src/environment.rs 1249 pub(crate) fn finalize_block client/rpc/src/chain/mod.rs|83 ChainBackend::block_hash client/finality-grandpa/src/import.rs|366 GrandpaBlockImport::make_authorities_changes client/service/src/client/client.rs|1036 Client::block_status client/network/sync/src/lib.rs|804 ChainSync::on_block_data / AncestorSearch primitives/blockchain/src/backend.rs|227 trait Backend::best_containing primitives/blockchain/src/backend.rs|53 HeaderBackend::block_hash_from_id ```