Merge the `valid_bundles`, `invalid_bundles`, and `block_extrinsics_roots` fields of the ER into one `bundles` field and add more precise fraud detection #1995

NingLin-P · 2023-09-22T13:14:11Z

This PR merges the valid_bundles, invalid_bundles, and block_extrinsics_roots fields of the ER into one bundles field, this simplifies the verification and fraud detection of the ER.

This PR also brings more precise fraud detection of the ER::bundles, with cases unconsidered before handled properly:

A bundle can contain multiple invalid extrinsic
- Before: the honest operator uses the first invalid extrinsic as the invalid_type of the bundle, but the malicious operator can construct fraud proof to prove the bundle is invalid of a different type due to another invalid extrinsic, the fraud proof will be accept and pruning the valid ER
- Now: an extrinsic_index is added to the invalid_type, thus the verifier can get the exact extrinsic when verifying the proof
An extrinsic can be considered invalid due to multiple invalid_type at the same time
- Before: even with extrinsic_index added, a single extrinsic can be OutOfRangeTx and InvalidXDM at the same time, thus the malicious operator can still construct valid fraud proof to prune the valid ER
- Now: define a checking order for each invalid_type (like the order in the invalid ER fraud proof), thus the honest operator can know the exact invalidation to prove/verify according the mismatch of invalid_type

Introducing an extrinsic_index also means the invalid bundle fraud proof doesn't need to carry the whole bundle instead just a single invalid extrinsic with a storage proof of the bundle_extrinsic_root cc @ParthDesai

Code contributor checklist:

I have read, understood and followed contributing guide

…undleValidity from sp-domains Minor refactoring to avoid naming conflicy with incoming changes Signed-off-by: linning <linningde25@gmail.com>

…ields into the bundles field in ER The bundles field have type of Vec<InboxedBundle>, the InboxedBundle contains all the data of the previous three fields and addtionally the invalid extrinsic index is added to InvalidBundleType which will be used to detect the bundles mismatch in the next commit Signed-off-by: linning <linningde25@gmail.com>

This version of find_inboxed_bundles_mismatch not only updated with changes of the ER fields but also consider more cases that not are considered before like: a bundle can contains multiple invalid extrinsics and an extrinsic can be considered as invalid due to multiple invalid_type at the same time, and handle these case properly. Signed-off-by: linning <linningde25@gmail.com>

crates/pallet-domains/src/block_tree.rs

This commit fix the test test_invalid_domain_extrinsic_root_proof, before this commit it is not a legitimate test at all since it try to generate valid fraud proof for a valid receipt. Signed-off-by: linning <linningde25@gmail.com>

vedhavyas

Did a first pass. I'm confused on few things and raised questions. Will do another pass later

vedhavyas · 2023-10-04T15:35:27Z

domains/client/block-preprocessor/src/lib.rs

@@ -47,6 +47,11 @@ type DomainBlockElements<CBlock> = (
    MaybeNewRuntime,
 );

+enum BundleValidity<Extrinsic> {


it is weird seeing some of the type in sp-domains and some in here. Not sure I understand why this is needed ?

Naming conflict unfortunately, would be great if you suggest a better name for this.

crates/pallet-domains/src/block_tree.rs

crates/sp-domains/src/lib.rs

vedhavyas · 2023-10-04T15:49:19Z

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

@@ -118,11 +118,6 @@ where
            ))
        })?;

-        if local_receipt.invalid_bundles != receipt.invalid_bundles {


shouldn't this check be done for inboxed_bundles correct ?

inboxed_bundles is checked in the domain_block_processor but not the ReceiptValidator which is used in the block-preprocessor since we don't consider bundle as invalid due to invalid ER anymore. And you remind me that ReceiptValidator should be removed completely.

vedhavyas · 2023-10-04T15:52:46Z

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

-    }
+    // TODO: update test
+    // #[test]
+    // fn invalid_bundles_fraud_proof_detection() {


Why comment them? You can just ignore them instead

vedhavyas · 2023-10-04T15:57:12Z

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

+    // constructed/imported thus the `external_receipt` must have the same `bundles_extrinsics_roots`
+    //
+    // NOTE: this also check `local_receipt.bundles` and `external_receipt.bundles` have the same length
+    if local_receipt.bundles_extrinsics_roots() != external_receipt.bundles_extrinsics_roots() {


isn't this another type of invalid ER and should be proved on the consensus or is this already checked as part of bundle submission?

It has already been checked as part of bundle submission, but I want to add the check here to make it more robust and also give more context about why the following expect/unreachable is unreachable.

vedhavyas · 2023-10-04T15:59:02Z

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

-/// pointing to first mismatch in invalid bundles array.
-pub(crate) fn verify_invalid_bundles_field<Block, CBlock>(
+// Find the first mismatch of the `InboxedBundle` in the `ER::bundles` list
+pub(crate) fn find_inboxed_bundles_mismatch<Block, CBlock>(


It would be nice to have removed the commented code instead. Git diff after uncommenting and updating is painful to review TBH.

Thanks for letting me know! will do that next time.

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

vedhavyas · 2023-10-04T16:09:27Z

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

+    // The `local_bundle` and `external_bundle` are checked to have the same `extrinsic_root`
+    // thus they must being mismatch due to bundle validity
+    let mismatch_type = match (local_bundle.bundle.clone(), external_bundle.bundle.clone()) {
+        (


Please do not use nestings like. I have no clue how to process this function. Would be ideal if fucntionality to check a single bundle mismatch would be easier and does not hurt readability. Also Git diff from commented code to uncommented code is not helping either

Please do not use nestings like. I have no clue how to process this function. Would be ideal if fucntionality to check a single bundle mismatch would be easier and does not hurt readability.

Could you be more specific? I use match to handle each mismatch case in a dedicated branch which should be straightforward and clean IMHO, commented/uncommented code could hurt readability when reviewing though.

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

NingLin-P requested review from ParthDesai and dariolina September 22, 2023 13:14

NingLin-P requested review from vedhavyas, nazar-pc and rg3l3dr as code owners September 22, 2023 13:14

NingLin-P added 3 commits October 2, 2023 19:35

Rename InboxedBundle storage item to InboxedBundleAuthor and remove B…

2cddcd2

…undleValidity from sp-domains Minor refactoring to avoid naming conflicy with incoming changes Signed-off-by: linning <linningde25@gmail.com>

NingLin-P force-pushed the ER-refactoring branch from 7366e3c to 3836bd6 Compare October 2, 2023 12:26

dariolina reviewed Oct 3, 2023

View reviewed changes

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

Fix test test_invalid_domain_extrinsic_root_proof

838ac71

This commit fix the test test_invalid_domain_extrinsic_root_proof, before this commit it is not a legitimate test at all since it try to generate valid fraud proof for a valid receipt. Signed-off-by: linning <linningde25@gmail.com>

vedhavyas reviewed Oct 4, 2023

View reviewed changes

NingLin-P mentioned this pull request Oct 5, 2023

Implement OutOfRangeTx variant of Invalid bundle fraud proof #2039

Closed

1 task

NingLin-P added 2 commits October 6, 2023 05:07

Apply review suggestions

2da9280

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

Merge branch 'main' of github.com:subspace/subspace into ER-refactoring

d957fc4

vedhavyas approved these changes Oct 6, 2023

View reviewed changes

NingLin-P added this pull request to the merge queue Oct 6, 2023

Merged via the queue into main with commit 7728b93 Oct 6, 2023
10 checks passed

NingLin-P deleted the ER-refactoring branch October 6, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge the `valid_bundles`, `invalid_bundles`, and `block_extrinsics_roots` fields of the ER into one `bundles` field and add more precise fraud detection #1995

Merge the `valid_bundles`, `invalid_bundles`, and `block_extrinsics_roots` fields of the ER into one `bundles` field and add more precise fraud detection #1995

NingLin-P commented Sep 22, 2023 •

edited

Loading

vedhavyas left a comment

vedhavyas Oct 4, 2023

NingLin-P Oct 5, 2023

vedhavyas Oct 4, 2023

NingLin-P Oct 5, 2023

vedhavyas Oct 4, 2023

vedhavyas Oct 4, 2023

NingLin-P Oct 5, 2023

vedhavyas Oct 4, 2023

NingLin-P Oct 5, 2023

vedhavyas Oct 4, 2023

NingLin-P Oct 5, 2023

Merge the valid_bundles, invalid_bundles, and block_extrinsics_roots fields of the ER into one bundles field and add more precise fraud detection #1995

Merge the valid_bundles, invalid_bundles, and block_extrinsics_roots fields of the ER into one bundles field and add more precise fraud detection #1995

Conversation

NingLin-P commented Sep 22, 2023 • edited Loading

Code contributor checklist:

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

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

Choose a reason for hiding this comment

Merge the `valid_bundles`, `invalid_bundles`, and `block_extrinsics_roots` fields of the ER into one `bundles` field and add more precise fraud detection #1995

Merge the `valid_bundles`, `invalid_bundles`, and `block_extrinsics_roots` fields of the ER into one `bundles` field and add more precise fraud detection #1995

NingLin-P commented Sep 22, 2023 •

edited

Loading