Manually track consensus block hash for ER verification #1847
Conversation
The consensus block hash is only tracked if the block contains bundle and it is pruned when the corresponding ER is pruned. This commit also adjust the test correspondingly. Signed-off-by: linning <linningde25@gmail.com>
…kHashCount Signed-off-by: linning <linningde25@gmail.com>
NingLin-P
requested review from
vedhavyas,
nazar-pc and
rg3l3dr
as code owners
| @@ -491,10 +490,6 @@ parameter_types! { | |||
| pub const MaxPendingStakingOperation: u32 = 100; | |||
| } | |||
|
|
|||
| // `BlockTreePruningDepth` should <= `BlockHashCount` because we need the consensus block hash to verify | |||
| // execution receipt, which is used to construct the node of the block tree. | |||
| const_assert!(BlockTreePruningDepth::get() <= BlockHashCount::get()); | |||
There was a problem hiding this comment.
Nice. I noticed earlier that BlockHashCount was unused so removing the assertion make sense
There was a problem hiding this comment.
This is the last use of static_assertions, dependency should have been removed as well
| /// for more details, this will be fixed once https://github.com/subspace/subspace/issues/1731 is implemented. | ||
| #[pallet::storage] | ||
| #[pallet::getter(fn consensus_hash)] | ||
| pub type ConsensusBlockHash<T: Config> = |
There was a problem hiding this comment.
I think there is a another optimization that can done here using reference counting for all the domains rather than storing the same hash for each domain.
There was a problem hiding this comment.
I have considered this but it seems more complected because each domain may submit bundle in different consensus blocks so the consensus hash needed to store is different from domain to domain, and when should a consensus hash be pruned is also different from domain to domain, the complexity come when we need to track these difference.
| @@ -267,6 +282,11 @@ pub(crate) fn process_execution_receipt<T: Config>( | |||
| } | |||
| let _ = ExecutionInbox::<T>::clear_prefix((domain_id, to_prune), u32::MAX, None); | |||
|
|
|||
| ConsensusBlockHash::<T>::remove( | |||
There was a problem hiding this comment.
why not prune it when we see the first ER instead of waiting until challenge period is over since we do not require this for the same ERs which comes from other operators ?
There was a problem hiding this comment.
Well the ER that is still in the challenge period can be pruned by fraud proof and resubmitted by other operators thus we need the consensus hash for the verification.
Currently, we use the consensus block hash from the
frame-systemto verify ER, even though we have the assumption ofBlockTreePruningDepth<=BlockHashCount, which is not enough to guarantee the consensus block hash must exist when verifying ER because the domain block is not mapping to the consensus block one by one, if there are too many empty consensus blocks, the older hash will be pruned.This PR fixes the issue by manually tracking the consensus block hash in
pallet-domains, the storage is still unbounded in some cases see #1673 but the situation is better than in domain v1 because now we only need to store the consensus hash if the consensus block contains bundle, the unbounded storage issue should be fixed once #1731 is implemented.Code contributor checklist: