-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce basic storage proof for the stateless fraud proof #2746
Conversation
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 <[email protected]>
Signed-off-by: linning <[email protected]>
Signed-off-by: linning <[email protected]>
…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 <[email protected]>
0865c11
to
9dba588
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems annoyingly invasive 😕
#[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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -254,6 +242,10 @@ where | |||
} | |||
} | |||
|
|||
pub fn transaction_byte_fee_storage_key() -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
/// 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() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need it?
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 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:
NOTE: the FP generation happen in the domain client and the verification happen in the |
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 |
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
To keep a copy of these states in
|
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. |
Emm.. currently these value is query from the same consensus block that derives the domain block, if a bundle is constructed at consensus block
|
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. |
Not really, there are 2 checks about how long a bundle is considered valid to be included:
But it is possible that there are many bundles produced at different consensus blocks 1..4 and all are included in block 5. |
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? |
Yeah, basically that means we need historical consensus state to verify the bundle. |
That doesn't seem too bad if it is the same stuff for all domains and just for bundle logevity period. |
This problem remain
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 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 <[email protected]>
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this when verifying the FP in the consensus runtime, for the domain client we already have runtime API to get this.
} | ||
|
||
impl<T: Config> sp_domains::OnDomainInstantiated for Pallet<T> { | ||
fn on_domain_instantiated(domain_id: DomainId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this callback?
We can just make the Storage into a ValueQuery instead of OptionalQuery, no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pub struct InitializeDomainChainAllowlistUpdate; | ||
|
||
impl OnRuntimeUpgrade for InitializeDomainChainAllowlistUpdate { | ||
fn on_runtime_upgrade() -> Weight { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need to do this upgrade if its just ValueQuery I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be answered in #2746 (comment)
Signed-off-by: linning <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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: