Fraud Proof: Invalid domain extrinsic root #1999

vedhavyas · 2023-09-23T11:16:46Z

This PR generates invalid extrinsic root fraud when there is a mismatch between operator local receipt and submitted receipt.

Apart from this, there are two notable changes:

All the domains will start using StateVersion::V1 for extrinsic root
A modified version of extrinsic root derivation without actual extrinsic data that exceeds 32 bytes. This could be upstreamed but at the moment i'm not sure how to propose the change as the upstream trie needs some bit of refactor before this could be integrated. This can be done at a later time IMO and not a priority.

Note: The polkadot-sdk commit is from the PR that is yet to be merged. Once that is merged, i'll update the commit ref here and is a non blocker for the review.

Closes: #1888

Code contributor checklist:

I have read, understood and followed contributing guide

nazar-pc

Reviewed commits until Substrate upgrade. I don't have enough context to understand everything, so left a bunch of questions and will probably have to have a call to clarify a bunch more, especially as it relates to Substrate changes.

nazar-pc · 2023-09-24T03:49:55Z

crates/sp-domains/src/fraud_proof.rs

+/// This is a representation of actual Randomness on Consensus chain state.
+/// Any change in key or value there should be changed here accordingly.
+pub fn block_randomness_final_key() -> Vec<u8> {
+    frame_support::storage::storage_prefix("Subspace".as_ref(), "BlockRandomness".as_ref()).to_vec()
+}


I do not like this at all. There is no connection to pallet-subspace, it is not even mentioned in the comment. There is actually BlockRandomness::storage_value_final_key() method thanks to StorageValue trait, if you need this key, I'd say you should probably expose runtime API that returns you this key instead. If there are other similar places in domains, they should be fixed similarly as well.

crates/sp-domains/src/fraud_proof.rs

domains/client/domain-operator/src/fraud_proof.rs

nazar-pc · 2023-09-24T04:26:36Z

domains/client/domain-operator/src/fraud_proof.rs

+                domain_id,
+                bad_receipt_hash,
+                valid_bundle_digests,
+                randomness_proof,


Why is randomness proof necessary? Can't something on consensus chain that cares about it store proofs? Maybe even on the client (depending on how proof is verified afterwards).

Block randomness changes per block and runtime needs to have access to randomness for the block in which ER is submitted. So we provide a randomness proof present at that instant.

nazar-pc · 2023-09-24T04:30:40Z

domains/client/domain-operator/src/domain_block_processor.rs

+                    execution_receipt.consensus_block_number,
+                    execution_receipt.hash(),
+                    ReceiptMismatchInfo::DomainExtrinsicsRoot {
+                        consensus_block_hash,
+                    },


Why do we have both consensus block number and hash at the same time? We can get number from hash.

This whole tuple looks strange: we have consensus block number, then has of ER, but then mismatch info enum just has consensus block hash, which is ignored below. There should be a more consistent way to do this.

We need consensus block number since aux storage is fork aware and indexes number to block hashes.

This whole tuple looks strange: we have consensus block number, then has of ER, but then mismatch info enum just has consensus block hash, which is ignored below. There should be a more consistent way to do this.

I agree. We can make it consistent but right now we are re-using some data types from Fraud proof V1 and there is a TODO to clean it up as we do not know how Fraud proof V2 would look like right now

nazar-pc · 2023-09-24T04:53:53Z

crates/sp-domains/src/verification.rs

+        .into_iter()
+        .zip(valid_bundle_digests)
+    {
+        let bundle_digest_hash = BlakeTwo256::hash_of(&bundle_digest.bundle_digest);


Bundle digest is already a hash, why does it need to be hashed again? It doesn't make a lot of sense especially reading next line where you compare hash to non-hashed bundle digest.

The naming is confusing here and updated it. ValidBundles contains the hash of the bundle digest for each bundle while this fraud proof carries the pre-image and is verified as part of the fraud proof verification.

Digest is still a hash, why is it necessary to hash the hash?

No it not the hash. valid_bundle has the digest hash that is part of the ER on the runtime while fraud proof contains the original digest_hash's preimage which we are hashing and comparing. The struct is found here - https://github.com/subspace/subspace/pull/1999/files#diff-0cccf0c02c5905d75d809f04db4222140f39b09cf7abf950758ba6163fd75e45R453

crates/sp-domains/src/verification.rs

nazar-pc · 2023-09-24T04:56:00Z

crates/sp-domains/src/verification.rs

+    Hashing,
+>(
+    consensus_state_root: CBlock::Hash,
+    bad_receipt: ExecutionReceipt<


It seems like bad_receipt is a sizable data structure, can it be passed by reference instead of as an owned value?

we do not need this structure after the verification, so passed them as Owned rather cloning inner fields within this function

crates/sp-domains/src/verification.rs

nazar-pc · 2023-09-24T05:00:12Z

crates/sp-domains/src/verification.rs

+    Ok(())
+}
+
+fn extrinsics_shuffling_seed<Hashing>(block_randomness: Randomness) -> Hashing::Out


If you need Randomness in the end, why is this function returning Hashing::Out if you have to convert it 3 times after that at all site?

Since this is used on the runtime, and there is no way to get H256 from the Hasing::Out which is Block::Hash. So I'm converting this to H256

nazar-pc

Given context of fraud proofs v1 that are mixed with v2 and other circumstances this looks reasonable, though I may not able to judge it properly due to lack of depth of corresponding part of the codebase. Would be nice to get CI green on paritytech/polkadot-sdk#1691 that this PR requires.

crates/pallet-domains/Cargo.toml

nazar-pc · 2023-09-25T19:28:43Z

crates/sp-domains/src/verification.rs

+        .into_iter()
+        .zip(valid_bundle_digests)
+    {
+        let bundle_digest_hash = BlakeTwo256::hash_of(&bundle_digest.bundle_digest);


Digest is still a hash, why is it necessary to hash the hash?

nazar-pc · 2023-09-25T19:29:30Z

crates/pallet-domains/src/lib.rs

@@ -1779,7 +1780,7 @@ impl<T: Config> Pallet<T> {
    }

    pub fn extrinsics_shuffling_seed() -> T::Hash {
-        let seed = EXTRINSICS_SHUFFLING_SEED;
+        let seed = DOMAIN_EXTRINSICS_SHUFFLING_SEED;


The concern this name was that it is essentially "seed's seed"

vedhavyas

I have renamed the const to DOMAIN_EXTRINSICS_SHUFFLING_SEED_SUBJECT since that is the what the variable is called in randomness trait. I'm open for an different that makes more sense 👍🏼

Note: Rebased last two commits rather than appending since they anyway handle the review suggestions and changes are much less from the previous version of commits

vedhavyas · 2023-09-26T16:39:58Z

crates/sp-domains/src/verification.rs

+        .into_iter()
+        .zip(valid_bundle_digests)
+    {
+        let bundle_digest_hash = BlakeTwo256::hash_of(&bundle_digest.bundle_digest);


No it not the hash. valid_bundle has the digest hash that is part of the ER on the runtime while fraud proof contains the original digest_hash's preimage which we are hashing and comparing. The struct is found here - https://github.com/subspace/subspace/pull/1999/files#diff-0cccf0c02c5905d75d809f04db4222140f39b09cf7abf950758ba6163fd75e45R453

nazar-pc

Let's update polkadot-sdk fork PR with latest version of paritytech/polkadot-sdk#1691, update it here, resolve conflicts and should be good to go.

nazar-pc · 2023-09-27T09:58:18Z

Substrate upgrade should really be its own PR

vedhavyas · 2023-09-27T10:12:56Z

@nazar-pc removed the commit that updates the substrate commit and will create a PR once you back port your PR 👍🏼

…verify the invalid domain extrinsincs block root. trie root derivation was taken from trie-db with necessary modifications to allow for generating root using just hash of extrinsics.

…k and use StateVersion::V1 for domains

nazar-pc

There were too many force-pushes to understand what is going on. I'll just assume that nothing has changed except Substrate upgrade.

vedhavyas · 2023-09-27T17:53:32Z

@nazar-pc yes, nothing has changed and rebased on top the upgrade PR. Will merge this in

NingLin-P · 2023-10-03T14:52:07Z

crates/sp-domains/src/verification.rs

+    for (valid_bundle, bundle_digest) in bad_receipt
+        .valid_bundles
+        .into_iter()
+        .zip(valid_bundle_digests)


We should check the length of bad_receipt.valid_bundles and fraud_proof.valid_bundle_digests to be equal first, otherwise the malicious operator can use a fraudulent valid_bundle_digests to construct a valid fraud proof that target a valid ER.

NingLin-P · 2023-10-03T14:56:19Z

crates/pallet-domains/src/lib.rs

+                ConsensusBlockInfo::<T>::insert(
+                    domain_id,
+                    parent_number,
+                    (parent_hash, T::Hash::default()),


Would prefer to have a comment explaining why it is okay to use the default value here

NingLin-P · 2023-10-03T15:01:30Z

crates/pallet-domains/src/tests.rs

+        let domain_block = get_block_tree_node_at::<Test>(domain_id, bad_receipt_at).unwrap();
+
+        let bad_receipt_hash = domain_block.execution_receipt.hash();
+        let (fraud_proof, root) = generate_invalid_domain_extrinsic_root_fraud_proof::<Test>(
+            domain_id,
+            bad_receipt_hash,
+            Randomness::from([1u8; 32]),
+        );


This is not legitimate, it basically creates valid fraud proof that targets a valid ER, I fixed this in 44d3a69 though.

NingLin-P · 2023-10-03T15:38:01Z

crates/pallet-domains/src/lib.rs

+
+        /// Submit parent state root to the blockchain.
+        #[pallet::call_index(11)]
+        #[pallet::weight((Weight::from_all(10_000), DispatchClass::Mandatory, Pays::No))]


Weight::from_all(10_000) seems not accurate, plz leave a comment for this.

NingLin-P · 2023-10-03T15:45:43Z

crates/sp-domains/src/lib.rs

@@ -351,7 +355,7 @@ pub struct ExecutionReceipt<Number, Hash, DomainNumber, DomainHash, Balance> {
    /// The block hash corresponding to `domain_block_number`.
    pub domain_block_hash: DomainHash,
    /// Extrinsic root field of the header of domain block referenced by this ER.
-    pub domain_block_extrinsic_root: DomainHash,
+    pub domain_block_extrinsic_root: H256,


Should be ExtrinsicsRoot

NingLin-P · 2023-10-03T15:56:24Z

domains/client/domain-operator/src/tests.rs

@@ -998,6 +999,120 @@ async fn test_invalid_total_rewards_proof_creation() {
    ferdie.produce_blocks(1).await.unwrap();
 }

+#[tokio::test(flavor = "multi_thread")]
+#[ignore]


The underlying test infra for Fraud proof is still in V1 and not fully implemented.

Do you mean #1527?

vedhavyas requested review from NingLin-P, nazar-pc and rg3l3dr as code owners September 23, 2023 11:16

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from 250f2f7 to dc1609c Compare September 23, 2023 15:18

nazar-pc reviewed Sep 24, 2023

View reviewed changes

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from 8753c26 to 02146b2 Compare September 25, 2023 12:21

nazar-pc reviewed Sep 25, 2023

View reviewed changes

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from 02146b2 to 3c94c2e Compare September 26, 2023 16:35

vedhavyas commented Sep 26, 2023

View reviewed changes

vedhavyas requested a review from nazar-pc September 26, 2023 16:45

nazar-pc reviewed Sep 26, 2023

View reviewed changes

vedhavyas requested a review from nazar-pc September 27, 2023 09:39

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from 688f59f to d16a9dc Compare September 27, 2023 10:11

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from d16a9dc to 01b83c0 Compare September 27, 2023 14:49

vedhavyas changed the base branch from main to substrate_upgrade September 27, 2023 14:51

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from 01b83c0 to 5ddbbeb Compare September 27, 2023 15:02

vedhavyas force-pushed the substrate_upgrade branch from 8a2b10b to c219eca Compare September 27, 2023 15:14

vedhavyas added 8 commits September 27, 2023 17:19

generate fraud proof for invalid domain extrinsics root

0620755

submit invalid domain extrinsics root proof

aec3b95

move total rewards fraud proof verification to domain primitives

f71847e

add trie root to derive extrinsics root using value and add infra to …

a0f3ef7

…verify the invalid domain extrinsincs block root. trie root derivation was taken from trie-db with necessary modifications to allow for generating root using just hash of extrinsics.

create state root inherent to pass parent state root in the next bloc…

95e8f60

…k and use StateVersion::V1 for domains

verify invalid domain extrinsic root fraud proof on runtime

51cb0e6

derive storage keys from runtime and update comments

af30f80

rename confusing variable and constant names

b700e16

vedhavyas force-pushed the fraud_proof/domain_block_extrinsic_root branch from 5ddbbeb to b700e16 Compare September 27, 2023 15:19

Base automatically changed from substrate_upgrade to main September 27, 2023 16:57

nazar-pc approved these changes Sep 27, 2023

View reviewed changes

vedhavyas merged commit a5f37ee into main Sep 27, 2023
10 checks passed

vedhavyas deleted the fraud_proof/domain_block_extrinsic_root branch September 27, 2023 17:53

NingLin-P reviewed Oct 3, 2023

View reviewed changes

vedhavyas mentioned this pull request Nov 1, 2023

System domain executor can do an unsigned root call #1483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fraud Proof: Invalid domain extrinsic root #1999

Fraud Proof: Invalid domain extrinsic root #1999

vedhavyas commented Sep 23, 2023

nazar-pc left a comment

nazar-pc Sep 24, 2023

nazar-pc Sep 24, 2023

vedhavyas Sep 25, 2023

nazar-pc Sep 24, 2023

vedhavyas Sep 25, 2023

nazar-pc Sep 24, 2023

vedhavyas Sep 25, 2023 •

edited

Loading

nazar-pc Sep 25, 2023

vedhavyas Sep 26, 2023

nazar-pc Sep 24, 2023

vedhavyas Sep 25, 2023

nazar-pc Sep 24, 2023

vedhavyas Sep 25, 2023

nazar-pc left a comment

nazar-pc Sep 25, 2023

nazar-pc Sep 25, 2023

vedhavyas left a comment •

edited

Loading

vedhavyas Sep 26, 2023

nazar-pc left a comment

nazar-pc commented Sep 27, 2023

vedhavyas commented Sep 27, 2023

nazar-pc left a comment

vedhavyas commented Sep 27, 2023

NingLin-P Oct 3, 2023

NingLin-P Oct 3, 2023

NingLin-P Oct 3, 2023

NingLin-P Oct 3, 2023

NingLin-P Oct 3, 2023

NingLin-P Oct 3, 2023

Fraud Proof: Invalid domain extrinsic root #1999

Fraud Proof: Invalid domain extrinsic root #1999

Conversation

vedhavyas commented Sep 23, 2023

Code contributor checklist:

nazar-pc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedhavyas Sep 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedhavyas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc commented Sep 27, 2023

vedhavyas commented Sep 27, 2023

nazar-pc left a comment

Choose a reason for hiding this comment

vedhavyas commented Sep 27, 2023

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

Choose a reason for hiding this comment

vedhavyas Sep 25, 2023 •

edited

Loading

vedhavyas left a comment •

edited

Loading