Implement block tree for the domain v2 #1650
Conversation
NingLin-P
requested review from
liuchengxu,
vedhavyas,
nazar-pc and
rg3l3dr
as code owners
|
CI failed due to a similar issue of #1599, namely the |
|
@NingLin-P Since this is at genesis, can't we schedule it for next block? This would be fastest route though we can also look into upstream and find out reason why extensions are not enabled at genesis. We do have a few TODOs for upstream for the same issue so we have a way to track these TODOs |
There was a problem hiding this comment.
Some early comments, will take another look later.
Regarding the genesis extension, I have created an issue upstream paritytech/substrate#14541. As Ved said, the quick workaround at present is to defer the genesis receipt generation to sometime later, we can always revisit once it's fixed upstream, and we can contribute to it for sure.
| } | ||
|
|
||
| fn successful_bundle_hashes() -> Vec<H256> { | ||
| Domains::successful_bundles() | ||
| Domains::successful_bundles_of_all_domains() |
There was a problem hiding this comment.
I dont think this is correct. This func is called under the assumption that these belong to a single domain. So should be scoped to specific domain.
There was a problem hiding this comment.
Not true, this func is called by bundle validator and is expected bundle of all domains, the use case you mentioned use Domains::successful_bundles(domain_id).
There was a problem hiding this comment.
Hmmm if the consensus node wants to fetch these bundles, which is the only case part that needs to see all domain bundles that I could think of, the result of this function is not very useful IMO.
There was a problem hiding this comment.
Hmmm if the consensus node wants to fetch these bundles, which is the only case part that needs to see all domain bundles that I could think of
This is the exact use case, we use the bundle validator in the consensus node's tx pool to detect duplicated bundles otherwise the attacker can use them to occupy the block space without cost.
…need runtime_genesis_config parameter Signed-off-by: linning <[email protected]>
…mains serve for all domains Signed-off-by: linning <[email protected]>
The v1 primitives can be removed and replaced by them after the domain client retired all of the v1 usage. Signed-off-by: linning <[email protected]>
This commit also remove the unused ReceiptsPruningDepth Signed-off-by: linning <[email protected]>
This commit includes the block tree types, storage items, the submit_bundle_v2 call and pre_dispatch/validate_unsigned verification. Signed-off-by: linning <[email protected]>
… instantiation Signed-off-by: linning <[email protected]>
Signed-off-by: linning <[email protected]>
Signed-off-by: linning <[email protected]>
|
This PR is rebased, the changes of resolving the comments are put into the corresponding commit, PATL, thanks! |
Signed-off-by: linning <[email protected]>
| let next_number = HeadDomainNumber::<T>::get(domain_id) | ||
| .checked_add(&One::one()) | ||
| .ok_or::<Error<T>>(BlockTreeError::MaxHeadDomainNumber.into())?; | ||
| HeadDomainNumber::<T>::set(domain_id, next_number); |
There was a problem hiding this comment.
The HeadDomainNumber is stored in the consensus chain, suppose there is a consensus block that contains extrinsic that updates a domain 'sHeadDomainNumber from 1 to 2. Operators who have processed this consensus block (drive & import a domain block) its best block number update from 1 to 2, which match your above statement.
How do some one definitely prove that unless there is an execution receipt to corroborate that statement ?
| let next_number = HeadDomainNumber::<T>::get(domain_id) | ||
| .checked_add(&One::one()) | ||
| .ok_or::<Error<T>>(BlockTreeError::MaxHeadDomainNumber.into())?; | ||
| HeadDomainNumber::<T>::set(domain_id, next_number); |
There was a problem hiding this comment.
I'm not sure if you understand my reasoning. Per my understanding, just submitting domain bundle is not the correct step to consider a domain chain has progressed unless there is ER associated with it. Lets take the current DomainBlock, It needs to have ER and per our definition of DomainBlockNumber, if there is a new domain block, then there is new head backing. Right now, we just do it on first bundle, which seem odd TBH.
| // consensus block, which also mean a domain block will be produced thus update `HeadDomainNumber` | ||
| // to this domain block's block number. | ||
| if SuccessfulBundles::<T>::get(domain_id).is_empty() { | ||
| let next_number = HeadDomainNumber::<T>::get(domain_id) |
There was a problem hiding this comment.
another parallel question, why are we extending the head all receipt types? I'm not sure I see this handled. What if the first bundle is actually creating a new branch?
We should only extend for type NewHead, no ?
There was a problem hiding this comment.
#1650 (comment) may answer this, although bundle and ER are submitted together, they are building different things. ER is used to build the block tree and may create a new branch in the tree while the bundle is used to build the domain chain.
| execution_receipt: &ExecutionReceiptOf<T>, | ||
| ) -> ReceiptType { | ||
| let receipt_number = execution_receipt.domain_block_number; | ||
| let head_receipt_number = HeadReceiptNumber::<T>::get(domain_id); |
There was a problem hiding this comment.
I dont understant this. IIUC head_receipt_number should be the head of the fork this receipt is part of. But head receipt number you are using may or may not be the same
There was a problem hiding this comment.
The HeadReceiptNumber is applied to the whole block tree not any specific fork in the tree, refer #1650 (comment).
| /// number will be produce. Used as a pointer in `ExecutionInbox` to identify the current under building | ||
| /// domain block, also used as a mapping of consensus block number to domain block number. | ||
| #[pallet::storage] | ||
| pub(super) type HeadDomainNumber<T: Config> = |
There was a problem hiding this comment.
I'm not able to find specific need for this storage item and HeadReceiptNumber. Can you help me understand how these are useful ?
There was a problem hiding this comment.
HeadDomainNumber is used to index the ExecutionInbox as the comment explains, for HeadReceiptNumber refer #1650 (comment).
Signed-off-by: linning <[email protected]>
The domain tests are broken becasue the genesis domain is delay for 1 block to initialize, thus the domain block can not produce block for the slot 1, which cause the tests stuck. This commit fix it by produce 1 consensus block upon setup to initialize the genesis domain. Signed-off-by: linning <[email protected]>
There was a problem hiding this comment.
looks good to me to proceed, a few minor comments, none of them are blockers.
Regarding whether we should take all bundles that are possibly from different domain branches as the input for the next domain block, we can revisit once it's more clearly defined in the spec.
There was a problem hiding this comment.
Though I'm still skeptical about the assumptions made specifically on bundles, I think we can revisit them once we have some concrete examples that might warrant for a refactor.
Please fix conflicts and we can get this in 👍🏼
Thank you for being patient @NingLin-P
close #1610
This PR implements block tree for the domain v2, which tracks the execution receipt for all domains.
This PR also has revised the
generate_genesis_state_roothost function to integrate the domain registry with block tree, but the genesis state root is not unique for every domain instance as the spec required. Because the genesis block builder takes runtime genesis config as an input and which is the same for all domain instances (the default value), adding digest to the genesis block header won't help as it won't affect the state root, we need to handle this issue in #1631.Code contributor checklist: