Introduce basic storage proof for the stateless fraud proof #2746

NingLin-P · 2024-05-07T21:57:00Z

This PR is part of #2652

This PR introduces some basic storage proof generation & verification that will be used later in the stateless fraud proof with some refactoring, not much logic changes.

One notable logic change is the last commit, which ensures there is always a value in the DomainChainAllowlistUpdate storage map for a given domain, even if it is an empty value. This is necessary because in the invalid extrinsic root fraud proof we need to generate a storage proof of this value for a given domain, while storage proof is a proof-of-existence not a proof-of-absence, we generate a proof-of-empty-value in this case.

Code contributor checklist:

I have read, understood and followed contributing guide

This commit is pure refactoring to export access to the storage item and its storage key for the incoming storage proof Signed-off-by: linning <linningde25@gmail.com>

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

…iven domain Even DomainChainAllowlistUpdate is an empty value for a given domain it is still necessary to generate a proof-of-empty-value storage proof for the fraud proof Signed-off-by: linning <linningde25@gmail.com>

nazar-pc

This seems annoyingly invasive 😕

nazar-pc · 2024-05-13T09:20:55Z

crates/pallet-transaction-fees/src/lib.rs

-#[derive(Encode, Decode, TypeInfo)]
-struct BlockTransactionByteFee<Balance: Codec> {
-    // The value of `transaction_byte_fee` for the current block
-    current: Balance,
-    // The value of `transaction_byte_fee` for the next block
-    next: Balance,
-}
-
-impl<Balance: Codec + tokens::Balance> Default for BlockTransactionByteFee<Balance> {
-    fn default() -> Self {
-        BlockTransactionByteFee {
-            current: Balance::max_value(),
-            next: Balance::max_value(),
-        }
-    }
-}
-
 #[frame_support::pallet]
 mod pallet {
-    use super::{BalanceOf, BlockTransactionByteFee, CollectedFees, WeightInfo};
+    use super::{BalanceOf, CollectedFees, WeightInfo};
    use frame_support::pallet_prelude::*;
    use frame_support::traits::Currency;
    use frame_system::pallet_prelude::*;
-    use subspace_runtime_primitives::FindBlockRewardAddress;


Why is this refactoring necessary? It was an implementation detail of pallet-transaction-fees, you shouldn't need to access it from the outside. I also don't understand how it helps you if the rest of APIs that worked with this type remained private.

nazar-pc · 2024-05-13T09:23:51Z

crates/pallet-transaction-fees/src/lib.rs

@@ -254,6 +242,10 @@ where
        }
    }

+    pub fn transaction_byte_fee_storage_key() -> Vec<u8> {


Similarly I really don't like that this abstraction is now leaking to the outside. Can you explain why it is needed and why can't it be achieved without making more implemenation details public?

nazar-pc · 2024-05-13T09:33:28Z

crates/sp-domains/src/lib.rs

+/// Digest storage key in frame_system.
+/// Unfortunately, the digest storage is private and not possible to derive the key from it directly.
+pub fn system_digest_final_key() -> Vec<u8> {
+    frame_support::storage::storage_prefix("System".as_ref(), "Digest".as_ref()).to_vec()
+}


Why do you need it?

NingLin-P · 2024-05-13T10:31:50Z

All the above comments are related to one thing so I will respond here.

Fraud proof is essentially proving the process of how a domain block is derived from a consensus block, during this process there are some consensus chain states involved e.g. the TransactionByteFee and Digest states so they are not private in the domain perspective.

In main these states are getting directly from a host function while in the stateless FP we are using the storage proof and we need to export these stuff:

the storage key is required for both generating and verifying the storage proof
the state struct definition is required for extracting the state out of the storage proof (i.e. parsing) during FP verification

NOTE: the FP generation happen in the domain client and the verification happen in the pallet-domains

nazar-pc · 2024-05-13T10:36:33Z

Hm... so this seems to have potentially unbounded and really undocumented in scope set of dependencies on what is happening in consensus, meaning we'll have leaky abstractions in many places just to do fraud proofs.

Would it be possible to store all the stuff potentially necessary for such fraud proofs somewhere in pallet-domains state so we can store it all in a well known place and have a single storage proof? Or am I missing something?

NingLin-P · 2024-05-13T11:33:45Z

Would it be possible to store all the stuff potentially necessary for such fraud proofs somewhere in pallet-domains state so we can store it all in a well known place and have a single storage proof?

It is possible just trade-off about the cost of complexity and storage, the following is a list of the required consensus states outside of pallet-domains:

pallet_subspace::BlockRandomness
pallet_timestamp::Now
pallet_transaction_fees::TransactionByteFee
pallet_runtime_configs::EnableDynamicCostOfStorage
paller_messenger::DomainChainAllowlistUpdate
frame_system::Digest

To keep a copy of these states in pallet-domains there are 2 approach:

Push, whenever these states is updated they call into pallet-doamins (via a trait abstraction) to update the copy there. Since there are some upstream pallets it may be difficult to land this change.
Poll, in the on_finalize of pallet-domains it reads these states from the above pallet and stores a copy

nazar-pc · 2024-05-13T11:38:07Z

Can't those values be added to bundle header that is verified during inclusion into the chain and verified by all farmers? Then the only proof you need is the bundle header proof.

NingLin-P · 2024-05-13T11:56:11Z

Can't those values be added to bundle header that is verified during inclusion into the chain and verified by all farmers?

Emm.. currently these value is query from the same consensus block that derives the domain block, if a bundle is constructed at consensus block #1 but then included in #5:

how the consensus chain verify the bundle header that carries the state from #1?
if there is another bundle constructed at #2 included in the same consensus block, which bundle's value we should use to derive the domain block?

nazar-pc · 2024-05-13T11:58:55Z

if a bundle is constructed at consensus block #1 but then included in #5

I don't think this is allowed right now anyway. I belive bundles have to be included in the next block, this was discussed a few times on my memory and it was considered okay.

NingLin-P · 2024-05-13T12:22:09Z

Not really, there are 2 checks about how long a bundle is considered valid to be included:

After BundleLongevity number of consensus blocks, a bundle is considered to expire due to stale proof-of-time
- This is reserved as necessary time for the bundle to gossip in the network and reach the next block producer
Bundle A can become invalid before BundleLongevity blocks if another bundle B is included in the consensus chain (thus derive a domain block) after A is produced
- This is because the new domain block derived from bundle B may make the extrinsic in bundle A become illegal (e.g. due to nonce, tx fee)

But it is possible that there are many bundles produced at different consensus blocks 1..4 and all are included in block 5.

nazar-pc · 2024-05-13T12:25:24Z

Hm, interesting, that is news to me, but makes sense.

What is the difficulty with verifying bundles for previous blocks? The fact that some information about previous consensus block needs to be stored in runtime state?

NingLin-P · 2024-05-13T12:31:06Z

The fact that some information about previous consensus block needs to be stored in runtime state?

Yeah, basically that means we need historical consensus state to verify the bundle.

nazar-pc · 2024-05-13T12:54:44Z

That doesn't seem too bad if it is the same stuff for all domains and just for bundle logevity period.

NingLin-P · 2024-05-13T13:08:19Z

if there is another bundle constructed at #2 included in the same consensus block, which bundle's value we should use to derive the domain block?

This problem remain

To keep a copy of these states in pallet-domains there are 2 approach:
Poll, in the on_finalize of pallet-domains it reads these states from the above pallet and stores a copy

I think this approach along with using the value of the consensus block that derives the domain block instead of the consensus block when the bundle is produced is simpler and lower cost (one copy in the pallet-domains vs one copy per bundle).

Also, I think this is more like refactoring for long term maintenance purposes and I already made changes depending on the current PR so I would like to leave a TODO and open a tracking issue for now and proceed with the current implementation.

In this way we can know the exact storage proof that is failing Signed-off-by: linning <linningde25@gmail.com>

vedhavyas · 2024-05-14T10:50:24Z

crates/sp-domains/src/lib.rs

+// The domain storage fee multiplier used to charge a higher storage fee to the domain
+// transaction to even out the duplicated/illegal domain transaction storage cost, which
+// can not be eliminated right now.
+pub const DOMAIN_STORAGE_FEE_MULTIPLIER: Balance = 3;


I'm not sure why need to move this here. Right now its defined per runtime. So why not just open runtime api to get this on client if we need it ?

We need this when verifying the FP in the consensus runtime, for the domain client we already have runtime API to get this.

vedhavyas · 2024-05-14T10:59:29Z

domains/pallets/messenger/src/lib.rs

+}
+
+impl<T: Config> sp_domains::OnDomainInstantiated for Pallet<T> {
+    fn on_domain_instantiated(domain_id: DomainId) {


Why do we need this callback?
We can just make the Storage into a ValueQuery instead of OptionalQuery, no ?

ValueQuery or OptionalQuery just controls what to return if the requested key-value does not exist in the state, which doesn't mean it actually stores a Default::default or None value in the state.

When the key-value does not exist in the state read_proof will return a MissingValue error which makes sense because the storage proof is a proof-of-existence, not a proof-of-absence. What this callback does is manually store an empty value for the domain so we can generate a storage proof to prove this is an empty value.

vedhavyas · 2024-05-14T11:01:15Z

crates/subspace-runtime/src/lib.rs

+pub struct InitializeDomainChainAllowlistUpdate;
+
+impl OnRuntimeUpgrade for InitializeDomainChainAllowlistUpdate {
+    fn on_runtime_upgrade() -> Weight {


We do need to do this upgrade if its just ValueQuery I believe

Should be answered in #2746 (comment)

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

vedhavyas

Overall make sense.
But I do have my reservations regarding the Storage proof for Domain runtime code. Seems unnecessary over storing them in Consensus runtime.
But not going to block this

NingLin-P requested a review from dariolina May 7, 2024 21:57

NingLin-P requested review from vedhavyas, nazar-pc and rg3l3dr as code owners May 7, 2024 21:57

NingLin-P added 4 commits May 11, 2024 00:17

Move storage struct defination, export storage item/storage key

115bc30

This commit is pure refactoring to export access to the storage item and its storage key for the incoming storage proof Signed-off-by: linning <linningde25@gmail.com>

Add storage proof infra

41803bc

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

Add all the necessary storage proofs that are required by fraud proof

ef9cd8c

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

NingLin-P force-pushed the storage-proof-in-fp branch from 0865c11 to 9dba588 Compare May 10, 2024 16:25

NingLin-P changed the base branch from main to new-fp-host-func May 10, 2024 16:27

nazar-pc reviewed May 13, 2024

View reviewed changes

Upate storage proof verification error

2a40d96

In this way we can know the exact storage proof that is failing Signed-off-by: linning <linningde25@gmail.com>

vedhavyas reviewed May 14, 2024

View reviewed changes

Base automatically changed from new-fp-host-func to main May 15, 2024 09:29

An error occurred while trying to automatically change base from new-fp-host-func to main May 15, 2024 09:29

NingLin-P added 2 commits May 15, 2024 17:50

Merge branch 'main' into storage-proof-in-fp

cdeaeb3

Fix cargo fmt

f0b6959

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

vedhavyas approved these changes May 16, 2024

View reviewed changes

NingLin-P added this pull request to the merge queue May 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 16, 2024

NingLin-P added this pull request to the merge queue May 16, 2024

Merged via the queue into main with commit 8950a39 May 16, 2024
9 checks passed

NingLin-P deleted the storage-proof-in-fp branch May 16, 2024 17:24

dariolina added the need to audit This change needs to be audited label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce basic storage proof for the stateless fraud proof #2746

Introduce basic storage proof for the stateless fraud proof #2746

NingLin-P commented May 7, 2024

nazar-pc left a comment

nazar-pc May 13, 2024

nazar-pc May 13, 2024

nazar-pc May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

vedhavyas May 14, 2024

NingLin-P May 14, 2024

vedhavyas May 14, 2024

NingLin-P May 14, 2024 •

edited

vedhavyas May 14, 2024

NingLin-P May 14, 2024 •

edited

vedhavyas left a comment

Introduce basic storage proof for the stateless fraud proof #2746

Introduce basic storage proof for the stateless fraud proof #2746

Conversation

NingLin-P commented May 7, 2024

Code contributor checklist:

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc May 13, 2024

Choose a reason for hiding this comment

nazar-pc May 13, 2024

Choose a reason for hiding this comment

nazar-pc May 13, 2024

Choose a reason for hiding this comment

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

nazar-pc commented May 13, 2024

NingLin-P commented May 13, 2024

vedhavyas May 14, 2024

Choose a reason for hiding this comment

NingLin-P May 14, 2024

Choose a reason for hiding this comment

vedhavyas May 14, 2024

Choose a reason for hiding this comment

NingLin-P May 14, 2024 • edited

Choose a reason for hiding this comment

vedhavyas May 14, 2024

Choose a reason for hiding this comment

NingLin-P May 14, 2024 • edited

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

NingLin-P May 14, 2024 •

edited

NingLin-P May 14, 2024 •

edited