Illegal tx detection in context of whole bundle #2263

ParthDesai · 2023-11-22T22:18:40Z

Description

This PR updates domain runtime as follows in first commit:

Modification of check_transaction_validity runtime api: The modification allows it to validate unsigned and SelfContained evm txs along with signed txs.
Addition of check_transaction_validity_and_do_pre_dispatch api: This api validates the transaction as per check_transaction_validity + also preform pre_dispatch on the extrinsic to have changes like nonce updation persist in the buffer of RuntimeApiImpl.
Modification of storage_keys_for_verifying_transaction_validity: The modification changes function signature to make passing AccountId optional.
Addition of check_bundle_extrinsics_validity: This runtime api internally uses check_transaction_validity_and_do_pre_dispatch to validates extrinsic in the bundle and in case any illegal tx is detected it returns Err.

Changes to the domain client is as follows in second commit:

In domain proposer: While gathering tx to build a bundle, we utilize a single runtime instance to maintain context over multiple runtime calls to check_transaction_validity_and_do_pre_dispatch. In case an illegal tx is detected, it is ignored and its changes are rolled back from the context saved in that runtime instance. the call to the check_transaction_validity_and_do_pre_dispatch is shifted last to make sure that tx's changes do not persist if any check failes
In block preprocessor: We are using single runtime instance and check_transaction_validity_and_do_pre_dispatch to maintain context when iterating through the bundle transactions. This check is after inherent extrinsic as inherent extrinsic will always fail the validation check.

Code contributor checklist:

I have read, understood and followed contributing guide

…efactoring changes

… api to check illegaltx

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

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

domains/primitives/runtime/src/lib.rs

NingLin-P · 2023-11-27T09:32:36Z

domains/runtime/evm/src/lib.rs

-                        storage_keys: storage_keys_for_verifying_tx_validity_inner(signer, block_number, Self::extrinsic_era(uxt)),
+            let maybe_signer = check_transaction_validity_inner(&lookup, uxt, block_number, block_hash)?;
+
+            let storage_keys = storage_keys_for_verifying_tx_validity_inner(maybe_signer, block_number, Self::extrinsic_era(uxt));


I wonder if we still need to return these storage keys here, since the generation of fraud proof happens when there is mismatch of ER::inbo_bundles instead of when this check return error.

vedhavyas · 2023-11-27T09:44:10Z

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

+                let transaction_validity_result =
+                    runtime_api_instance.execute_in_transaction(|api| {
+                        let transaction_validity_result = api
+                            .check_transaction_validity_and_do_pre_dispatch(


If we are doing this check on bundle level, should we do this check on individual extrinsics above?
Would they go through the same approach making individual extrinsic check redundant ?

We don't need the individual extrinsic level check IMHO since the bundle level is a superset of it. Ideally, we should perform the check on the block level (like the consensus chain does when producing block) but bundle level is the best we can do.

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

domains/runtime/evm/src/lib.rs

NingLin-P · 2023-11-27T11:36:26Z

domains/runtime/evm/src/lib.rs

+            // We invoke `pre_dispatch` in addition to `validate_transaction`(even though the validation is almost same) as that will add the side effect of SignedExtension in the storage buffer
+            // which would help to maintain context across multiple transaction validity check against same
+            // runtime instance.
+            match xt.signed {


While it is desired to perform a more exhaustive validation here to cover different types of tx and checks, the storage_keys only contains a few storages used in the SignedExtra check which may not be exhaustive, i.e. if the pre_dispatch of a tx requires access to another storage which is not included in the storage_keys then we are unable to construct a valid fraud proof. I think this is a broader problem we need to think on cc @vedhavyas

@NingLin-P I have found a way. Idea is to use ProofRecorder to get the keys accessed during runtime execution. These keys are not public at the moment. I have opened a PR here: paritytech/polkadot-sdk#2561

Once that is in, to create storage proof, we simply have to execute desired runtime call and get the accessed keys from ProofRecorder.

@ParthDesai You should also patch this in our fork so that we use it right away instead of waiting until we upgrade substrate

NingLin-P · 2023-11-27T11:50:56Z

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

+                let transaction_validity_result =
+                    runtime_api_instance.execute_in_transaction(|api| {
+                        let transaction_validity_result = api
+                            .check_transaction_validity_and_do_pre_dispatch(


We don't need the individual extrinsic level check IMHO since the bundle level is a superset of it. Ideally, we should perform the check on the block level (like the consensus chain does when producing block) but bundle level is the best we can do.

vedhavyas

I'm confused with so many new API being introduced/changed but we would just need one API to check the bundle validity here 🤔

vedhavyas · 2023-11-30T12:53:13Z

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

+            // Using `check_transaction_and_do_pre_dispatch` instead of `check_transaction_validity`
+            // to maintain side-effect in the storage buffer.
+            let is_legal_tx = if api_version == 2 {
+                runtime_api.check_transaction_and_do_pre_dispatch(


If we are doing a full bundle check during the bundle generation, shouldn't we use the same here and mark bunlde invalid if bundle check fails?

We do that. If this check fails it is marked as invalid.

crates/sp-domains/src/lib.rs

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

vedhavyas · 2023-11-30T13:08:23Z

domains/primitives/runtime/src/lib.rs

+
+        /// New in v2.
+        /// Checks validity of bundle extrinsics. Internally calls `check_transaction_and_do_pre_dispatch`
+        fn check_bundle_extrinsics_validity(bundle_extrinsics: Vec<<Block as BlockT>::Extrinsic>, block_number: NumberFor<Block>,


I dont see this being used during bundle proposal. I'm confused why there are so many api when all we need is this API to check the bundle validity and return the extrinsic index that fails the precheck and the storage proof until this extrinsic. This also make me think how big can the storage proof be

This also make me think how big can the storage proof be

This depends on the exact storage accessed during the pre_dispatch of the illegal tx, but the size should be guarded by the excessive transaction check since pre_dispatch is also part of the execution of the tx.

@vedhavyas Reason why we have used check_transaction_and_do_pre_dispatch and not check_bundle_extrinsics_validity both during bundle proposal and bundle validation is because they both are equivalent in the sense bundle one deals with vector of extrinsic and the other one deal with single extrinsic.

I think, I am starting to see how it is easy to be confused by this. Let me remove this api. We can always add it back if it is required.

@vedhavyas Regarding storage proof, idea is that you only check for extrinsic signed by same signer. Which covers both nonce and tx payment check. This would mean storage proof is bounded to storage keys used during validation of all extrinsics signed by same signer.

ParthDesai · 2023-11-30T14:21:56Z

I'm confused with so many new API being introduced/changed but we would just need one API to check the bundle validity here 🤔

At the time of writing this PR, idea was to have two equivalent apis one operate on single tx and other one operate on set of tx. Looking back, I realized that check_bundle_extrinsics is not needed. We can always use check_transaction_and_do_pre_dispatch instead. I will be removing that.

NingLin-P

The changes look good to me in general and I'm okay to move forward and leave the storage keys collect in a separate PR.

Some nits are left, mostly just some cleanup and the test (PTAL at how other tests using the helper function to reduce the size of the test)

NingLin-P · 2023-12-06T15:14:49Z

domains/primitives/runtime/src/lib.rs

        fn check_transaction_validity(
            uxt: &<Block as BlockT>::Extrinsic,
            block_number: NumberFor<Block>,
            block_hash: <Block as BlockT>::Hash
        ) -> Result<(), CheckTxValidityError>;

+        /// Checks the validity of extrinsic
+        /// v2 can check validity of unsigned and self contained extrinsics as well.
+        fn check_transaction_validity(


This is essentially a wrapper of TaggedTransactionQueue::validate_transaction with additional storage keys returned in the error, since we will get the storage keys in other ways I think this runtime API can be removed and the CheckTxValidityError can be replaced with TransactionValidityError

Okay. I can remove the new version of the API. But, as stated in another comment, we cannot deprecate a runtime api without adding newer version of that runtime api. So, old version need to stay as it is till we upgrade to new network.

NingLin-P · 2023-12-06T15:17:55Z

domains/primitives/runtime/src/lib.rs

        fn storage_keys_for_verifying_transaction_validity(
            account_id: opaque::AccountId,
            block_number: NumberFor<Block>,
            tx_era: Option<Era>,
        ) -> Result<Vec<Vec<u8>>, VerifyTxValidityError>;

+        /// Returns the storage keys of states accessed in the API `check_transaction_validity`
+        /// In v2, signer account id is optional to support getting storage keys for unsigned tx as well.
+        fn storage_keys_for_verifying_transaction_validity(


IIUC, this runtime API is used to return a fixed set of storage key should also deprecate, right?

Problem is, we cannot deprecate a runtime api without adding newer version of that runtime api in its place. As far as I understand, what this translates to is there is no clear upgrade path on how to remove/deprecate an api without introducing a new one.

Ref: https://paritytech.github.io/polkadot-sdk/master/sp_api/macro.decl_runtime_apis.html

I guess we need to remove it in next version of the network.

I think we are going to have a maintenance branch for gemini-3g soon, so the compatibility with gemini-3g won't be a concern anymore. cc @jfrank-summit

If breaking changes are introduced, PR should be marked with corresponding label

Okay. Label added. Will remove those apis as well as api versioning requirement as well.

NingLin-P · 2023-12-06T15:39:00Z

domains/runtime/evm/src/lib.rs

+            // Initializing block related storage required for validation
+            System::initialize(
+                &(System::block_number() + BlockNumber::one()),
+                &block_hash,
+                &Default::default(),
+            );


This will call multiple times for each extrinsic and the result will be preserved as we use the same runtime API instance for all extrinsic. From the System::initialize implementation it seems fine though.

Yes. We deliberately invoke System::initialize here as it is required to validate the SignedExtension. This is something I imported from Executive::validate_transaction.

crates/sp-domains-fraud-proof/src/tests.rs

NingLin-P · 2023-12-06T16:05:27Z

crates/sp-domains-fraud-proof/src/tests.rs

+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn check_bundle_validity_runtime_api_should_work() {


It seems this check is duplicated with runtime_instance_assumptions_are_correct

The tests are mostly similar, but the runtime assumption test is there to make sure that the assumptions we made throughout the code regarding runtime api is valid.

While this test is more concerned about testing the api.

Anyways, I have renamed runtime assumption test api to be more clear.

ParthDesai · 2023-12-10T05:18:50Z

@NingLin-P I have simplified the runtime api changes now (deleted storage keys related apis and the tx validity api). And accordingly tests are also more clear and simplified with changes suggested by the review incorporated.

NingLin-P

Rest LGTM 👍

crates/sp-domains-fraud-proof/src/tests.rs

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

domains/runtime/evm/src/lib.rs

vedhavyas

Make sense but why did we break compatibility here?
I apologize for any confusion but we still want to maintain compatibility for gemini-3g though

ParthDesai · 2023-12-11T13:50:28Z

Make sense but why did we break compatibility here? I apologize for any confusion but we still want to maintain compatibility for gemini-3g though

@vedhavyas As per @NingLin-P and my conversation with @jfrank-summit this fraud proof will be part of the next iteration of the network. So, I just cleaned up legacy stuff related to storage keys and validating txs.

vedhavyas · 2023-12-11T13:53:46Z

@vedhavyas As per @NingLin-P and my conversation with @jfrank-summit this fraud proof will be part of the next iteration of the network. So, I just cleaned up legacy stuff related to storage keys and validating txs.

I agree but we cannot upgrade on Gemini-3g at this state. You can leave a TODO to cleanup once we are prepping for the next network. We have bunch of TODOs that will be gone before next network

vedhavyas

LGTM without approval since some comments needs to be resolved

…etection-first-draft

ParthDesai · 2023-12-12T17:12:50Z

@NingLin-P @vedhavyas Comments are resolved now. If there are no further concern, can you guys approve it?

ParthDesai requested review from vedhavyas, NingLin-P, nazar-pc and rg3l3dr as code owners November 22, 2023 22:18

ParthDesai force-pushed the illegal-tx-detection-first-draft branch 2 times, most recently from fad1133 to 8d24955 Compare November 24, 2023 06:55

ParthDesai added 2 commits November 24, 2023 20:11

add new runtime apis to validate and do pre dispatch for tx + other r…

ce7cdd1

…efactoring changes

modifying domain pre-processor and domain proposer to use new runtime…

11a2f8a

… api to check illegaltx

ParthDesai force-pushed the illegal-tx-detection-first-draft branch from 8d24955 to 11a2f8a Compare November 24, 2023 16:11

NingLin-P reviewed Nov 27, 2023

View reviewed changes

vedhavyas reviewed Nov 27, 2023

View reviewed changes

NingLin-P reviewed Nov 27, 2023

View reviewed changes

apply review suggestions

69b02cd

vedhavyas reviewed Nov 30, 2023

View reviewed changes

remove check_bundle_validity runtime api to simplify the implementation

d5f0a5d

NingLin-P reviewed Dec 6, 2023

View reviewed changes

ParthDesai force-pushed the illegal-tx-detection-first-draft branch from 8858d88 to d1ba55f Compare December 10, 2023 05:09

addressing feedback - backward incompatible apis + test simplification

e022889

ParthDesai force-pushed the illegal-tx-detection-first-draft branch from d1ba55f to e022889 Compare December 10, 2023 05:14

remove fp-rpc from dev dependency as it is not used

6dd4449

NingLin-P reviewed Dec 11, 2023

View reviewed changes

crates/sp-domains-fraud-proof/src/tests.rs Outdated Show resolved Hide resolved

domains/client/block-preprocessor/src/lib.rs Outdated Show resolved Hide resolved

domains/runtime/evm/src/lib.rs Outdated Show resolved Hide resolved

domains/runtime/evm/src/lib.rs Outdated Show resolved Hide resolved

vedhavyas reviewed Dec 11, 2023

View reviewed changes

vedhavyas reviewed Dec 12, 2023

View reviewed changes

ParthDesai added 2 commits December 12, 2023 20:25

addressing minor nits

66178a4

Merge branch 'main' of github.com:subspace/subspace into illegal-tx-d…

e535a93

…etection-first-draft

ParthDesai enabled auto-merge December 12, 2023 17:14

NingLin-P approved these changes Dec 12, 2023

View reviewed changes

ParthDesai added this pull request to the merge queue Dec 12, 2023

Merged via the queue into main with commit 67758ca Dec 12, 2023
10 checks passed

ParthDesai deleted the illegal-tx-detection-first-draft branch December 12, 2023 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Illegal tx detection in context of whole bundle #2263

Illegal tx detection in context of whole bundle #2263

ParthDesai commented Nov 22, 2023 •

edited

Loading

NingLin-P Nov 27, 2023

vedhavyas Nov 27, 2023

NingLin-P Nov 27, 2023

NingLin-P Nov 27, 2023

ParthDesai Nov 30, 2023

vedhavyas Nov 30, 2023

NingLin-P Nov 27, 2023

vedhavyas left a comment

vedhavyas Nov 30, 2023

ParthDesai Nov 30, 2023

vedhavyas Nov 30, 2023

NingLin-P Nov 30, 2023

ParthDesai Nov 30, 2023

ParthDesai Nov 30, 2023

ParthDesai commented Nov 30, 2023

NingLin-P left a comment

NingLin-P Dec 6, 2023

ParthDesai Dec 8, 2023

NingLin-P Dec 6, 2023

ParthDesai Dec 8, 2023

NingLin-P Dec 8, 2023

nazar-pc Dec 8, 2023

ParthDesai Dec 9, 2023 •

edited

Loading

NingLin-P Dec 6, 2023

ParthDesai Dec 8, 2023

NingLin-P Dec 6, 2023

ParthDesai Dec 10, 2023 •

edited

Loading

ParthDesai commented Dec 10, 2023

NingLin-P left a comment

vedhavyas left a comment

ParthDesai commented Dec 11, 2023

vedhavyas commented Dec 11, 2023

vedhavyas left a comment

ParthDesai commented Dec 12, 2023

Illegal tx detection in context of whole bundle #2263

Illegal tx detection in context of whole bundle #2263

Conversation

ParthDesai commented Nov 22, 2023 • edited Loading

Description

This PR updates domain runtime as follows in first commit:

Changes to the domain client is as follows in second commit:

Code contributor checklist:

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

ParthDesai commented Nov 30, 2023

NingLin-P 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

ParthDesai Dec 9, 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

ParthDesai Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

ParthDesai commented Dec 10, 2023

NingLin-P left a comment

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

ParthDesai commented Dec 11, 2023

vedhavyas commented Dec 11, 2023

vedhavyas left a comment

Choose a reason for hiding this comment

ParthDesai commented Dec 12, 2023

ParthDesai commented Nov 22, 2023 •

edited

Loading

ParthDesai Dec 9, 2023 •

edited

Loading

ParthDesai Dec 10, 2023 •

edited

Loading