Implement block tree for the domain v2 #1650

NingLin-P · 2023-07-06T23:00:33Z

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_root host 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:

I have read, understood and followed contributing guide

NingLin-P · 2023-07-07T13:01:10Z

CI failed due to a similar issue of #1599, namely the GenesisReceiptExtension is not registered when building the genesis storage, but this time we do not have a similar workaround. cc @liuchengxu

vedhavyas · 2023-07-08T16:40:44Z

@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

liuchengxu

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.

crates/sp-domains/src/lib.rs

crates/pallet-domains/src/lib.rs

crates/pallet-domains/src/block_tree.rs

crates/sp-domains/src/lib.rs

vedhavyas

Makes sense overall. Will take another look again.

crates/pallet-domains/src/lib.rs

vedhavyas · 2023-07-10T07:51:25Z

crates/subspace-runtime/src/lib.rs

        }

        fn successful_bundle_hashes() -> Vec<H256> {
-            Domains::successful_bundles()
+            Domains::successful_bundles_of_all_domains()


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.

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).

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.

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.

crates/pallet-domains/src/block_tree.rs

crates/sp-domains/src/lib.rs

crates/pallet-domains/src/tests.rs

crates/pallet-domains/src/block_tree.rs

…need runtime_genesis_config parameter Signed-off-by: linning <linningde25@gmail.com>

…mains serve for all domains Signed-off-by: linning <linningde25@gmail.com>

The v1 primitives can be removed and replaced by them after the domain client retired all of the v1 usage. Signed-off-by: linning <linningde25@gmail.com>

This commit also remove the unused ReceiptsPruningDepth Signed-off-by: linning <linningde25@gmail.com>

This commit includes the block tree types, storage items, the submit_bundle_v2 call and pre_dispatch/validate_unsigned verification. Signed-off-by: linning <linningde25@gmail.com>

… instantiation Signed-off-by: linning <linningde25@gmail.com>

Signed-off-by: linning <linningde25@gmail.com>

NingLin-P · 2023-07-10T18:34:35Z

This PR is rebased, the changes of resolving the comments are put into the corresponding commit, PATL, thanks!

crates/pallet-domains/src/lib.rs

crates/subspace-runtime/src/lib.rs

crates/pallet-domains/src/block_tree.rs

crates/pallet-domains/src/lib.rs

Signed-off-by: linning <linningde25@gmail.com>

crates/pallet-domains/src/benchmarking.rs

crates/pallet-domains/src/block_tree.rs

crates/pallet-domains/src/domain_registry.rs

crates/pallet-domains/src/lib.rs

vedhavyas · 2023-07-12T08:49:39Z

crates/pallet-domains/src/lib.rs

+                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);


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 ?

liuchengxu

Mostly make sense, nice work!

crates/pallet-domains/src/block_tree.rs

crates/pallet-domains/src/lib.rs

crates/pallet-domains/src/block_tree.rs

crates/pallet-domains/src/lib.rs

vedhavyas

Left a few questions on few storage items. I'm not sure why we need them and those seems to cause more confusion to me actually

crates/pallet-domains/src/lib.rs

vedhavyas · 2023-07-13T11:05:10Z

crates/pallet-domains/src/lib.rs

+                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);


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.

vedhavyas · 2023-07-13T11:43:46Z

crates/pallet-domains/src/lib.rs

+            // 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)


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 ?

#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.

crates/pallet-domains/src/lib.rs

vedhavyas · 2023-07-13T12:10:53Z

crates/pallet-domains/src/block_tree.rs

+    execution_receipt: &ExecutionReceiptOf<T>,
+) -> ReceiptType {
+    let receipt_number = execution_receipt.domain_block_number;
+    let head_receipt_number = HeadReceiptNumber::<T>::get(domain_id);


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

The HeadReceiptNumber is applied to the whole block tree not any specific fork in the tree, refer #1650 (comment).

vedhavyas · 2023-07-13T12:13:36Z

crates/pallet-domains/src/lib.rs

+    /// 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> =


I'm not able to find specific need for this storage item and HeadReceiptNumber. Can you help me understand how these are useful ?

HeadDomainNumber is used to index the ExecutionInbox as the comment explains, for HeadReceiptNumber refer #1650 (comment).

Signed-off-by: linning <linningde25@gmail.com>

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 <linningde25@gmail.com>

liuchengxu

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.

crates/pallet-domains/src/block_tree.rs

crates/pallet-domains/src/lib.rs

crates/pallet-domains/src/block_tree.rs

vedhavyas

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

crates/pallet-domains/src/block_tree.rs

Signed-off-by: linning <linningde25@gmail.com>

…pace#1650 autonomys/subspace#1672

NingLin-P requested review from liuchengxu, vedhavyas, nazar-pc and rg3l3dr as code owners July 6, 2023 23:00

liuchengxu reviewed Jul 9, 2023

View reviewed changes

vedhavyas reviewed Jul 10, 2023

View reviewed changes

NingLin-P added 8 commits July 10, 2023 20:26

Adjust the generate_genesis_state_root host function to remove the un…

cfac1fa

…need runtime_genesis_config parameter Signed-off-by: linning <linningde25@gmail.com>

Track successfully submitted bundles for all domains as now pallet-do…

2a55e3b

…mains serve for all domains Signed-off-by: linning <linningde25@gmail.com>

Add domain primitives for the domain v2 architecture

cc7cb2a

The v1 primitives can be removed and replaced by them after the domain client retired all of the v1 usage. Signed-off-by: linning <linningde25@gmail.com>

Add block tree specific runtime config

458a309

This commit also remove the unused ReceiptsPruningDepth Signed-off-by: linning <linningde25@gmail.com>

Impl block tree for the domain v2

b3e96dd

This commit includes the block tree types, storage items, the submit_bundle_v2 call and pre_dispatch/validate_unsigned verification. Signed-off-by: linning <linningde25@gmail.com>

Generate and import the genesis receipt to the block tree upon domain…

95b61cc

… instantiation Signed-off-by: linning <linningde25@gmail.com>

Revise and add test utilities for upcomming block tree test

fb18078

Signed-off-by: linning <linningde25@gmail.com>

Add block tree tests

4b92468

Signed-off-by: linning <linningde25@gmail.com>

NingLin-P force-pushed the block-tree-pr branch from f86d7a9 to 4b92468 Compare July 10, 2023 18:08

liuchengxu reviewed Jul 11, 2023

View reviewed changes

Apply review suggestions

520257e

Signed-off-by: linning <linningde25@gmail.com>

vedhavyas reviewed Jul 12, 2023

View reviewed changes

liuchengxu reviewed Jul 13, 2023

View reviewed changes

vedhavyas reviewed Jul 13, 2023

View reviewed changes

NingLin-P added 3 commits July 18, 2023 04:07

Apply review suggestions

69302ff

Signed-off-by: linning <linningde25@gmail.com>

Merge branch 'main' of github.com:subspace/subspace into block-tree-pr

b07e070

liuchengxu reviewed Jul 18, 2023

View reviewed changes

crates/pallet-domains/src/block_tree.rs Show resolved Hide resolved

crates/pallet-domains/src/block_tree.rs Outdated Show resolved Hide resolved

crates/pallet-domains/src/lib.rs Outdated Show resolved Hide resolved

crates/pallet-domains/src/block_tree.rs Show resolved Hide resolved

vedhavyas reviewed Jul 18, 2023

View reviewed changes

crates/pallet-domains/src/block_tree.rs Show resolved Hide resolved

NingLin-P added 2 commits July 18, 2023 19:33

Apply review suggestions

36297c1

Signed-off-by: linning <linningde25@gmail.com>

Merge branch 'main' of github.com:subspace/subspace into block-tree-pr

5cce800

Merge branch 'main' of github.com:subspace/subspace into block-tree-pr

d946795

NingLin-P requested review from vedhavyas and liuchengxu July 18, 2023 15:19

NingLin-P mentioned this pull request Jul 18, 2023

The unbounded gap between head receipt and the best domain block #1673

Closed

vedhavyas approved these changes Jul 18, 2023

View reviewed changes

NingLin-P enabled auto-merge July 18, 2023 17:13

NingLin-P merged commit d121d12 into main Jul 18, 2023
9 checks passed

NingLin-P deleted the block-tree-pr branch July 18, 2023 17:52

ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 4, 2023

import autonomys/subspace#1671 autonomys/subspace#1657 autonomys/subs…

9f062a1

…pace#1650 autonomys/subspace#1672

ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023

import autonomys/subspace#1671 autonomys/subspace#1657 autonomys/subs…

c530b5c

…pace#1650 autonomys/subspace#1672

ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023

import autonomys/subspace#1671 autonomys/subspace#1657 autonomys/subs…

a1773b7

…pace#1650 autonomys/subspace#1672

ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023

import autonomys/subspace#1671 autonomys/subspace#1657 autonomys/subs…

51fab17

…pace#1650 autonomys/subspace#1672

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement block tree for the domain v2 #1650

Implement block tree for the domain v2 #1650

NingLin-P commented Jul 6, 2023 •

edited

Loading

NingLin-P commented Jul 7, 2023

vedhavyas commented Jul 8, 2023 •

edited

Loading

liuchengxu left a comment

vedhavyas left a comment

vedhavyas Jul 10, 2023

NingLin-P Jul 10, 2023

vedhavyas Jul 10, 2023

NingLin-P Jul 10, 2023 •

edited

Loading

NingLin-P commented Jul 10, 2023

vedhavyas Jul 12, 2023

liuchengxu left a comment

vedhavyas left a comment

vedhavyas Jul 13, 2023

vedhavyas Jul 13, 2023

NingLin-P Jul 13, 2023 •

edited

Loading

vedhavyas Jul 13, 2023

NingLin-P Jul 13, 2023

vedhavyas Jul 13, 2023

NingLin-P Jul 13, 2023

liuchengxu left a comment •

edited

Loading

vedhavyas left a comment

Implement block tree for the domain v2 #1650

Implement block tree for the domain v2 #1650

Conversation

NingLin-P commented Jul 6, 2023 • edited Loading

Code contributor checklist:

NingLin-P commented Jul 7, 2023

vedhavyas commented Jul 8, 2023 • edited Loading

liuchengxu left a comment

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NingLin-P Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

NingLin-P commented Jul 10, 2023

Choose a reason for hiding this comment

liuchengxu left a comment

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NingLin-P Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuchengxu left a comment • edited Loading

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

NingLin-P commented Jul 6, 2023 •

edited

Loading

vedhavyas commented Jul 8, 2023 •

edited

Loading

NingLin-P Jul 10, 2023 •

edited

Loading

NingLin-P Jul 13, 2023 •

edited

Loading

liuchengxu left a comment •

edited

Loading