Ignore trie nodes while recording a proof #8172
Conversation
…ck-data-multiple-blocks
…ck-data-multiple-blocks
…ck-data-multiple-blocks
…ck-data-multiple-blocks
…ck-data-multiple-blocks
…-audience node_dev'
…om:paritytech/polkadot-sdk into bkchr-parachain-block-data-multiple-blocks
Yeah exactly. This pull request is just preparing this work. All of this is not yet used. |
| /// Trie nodes that should not be recorded. | ||
| /// | ||
| /// Only applies when proof recording is enabled. | ||
| pub ignored_nodes_by_proof_recording: Option<IgnoredNodes<Block::Hash>>, |
There was a problem hiding this comment.
Had a second thought here.
Maybe instead of providing some specific proof recorder parameters here, it would be better to provide some callback which would allow caller of Proposer API to customize the proof recorder instantiated by proposer internals? This could be more flexible for future extensions.
Something like:
let maybe_proposal = self
.proposer
.propose(
&parent_header,
&inherent_data.0,
inherent_data.1,
Digest { logs: digest },
proposal_duration,
|proof_recorder| { proof_recorder.with_ignored_nodes(ignored_node) },
)
.await
.map_err(|e| Box::new(e) as Box<dyn Error + Send>)?;
| fn select_next_core() -> (CoreSelector, ClaimQueueOffset) { | ||
| let blocks_per_pov = BlocksPerPoV::get(); | ||
|
|
||
| if blocks_per_pov == 0 { |
There was a problem hiding this comment.
I think this will break elastic scaling zombienet tests, because they have blocks_per_pov not set, but still require the correct core selector logic.
There was a problem hiding this comment.
All this code there is quite hacky right now :D blocks_per_pov is 1 by default.
| changes.transaction.drain().into_iter().for_each(|(_, (value, count))| { | ||
| // We only care about inserts and not deletes. | ||
| if count > 0 { | ||
| db.insert(EMPTY_PREFIX, &value); |
There was a problem hiding this comment.
What is up with this EMPTY_PREFIX here? I see that internally, the value is hashed for the key. Makes sense, but when would we need the prefix here?
There was a problem hiding this comment.
We are using a HashKey, that ignores the prefix. The prefix would be important if we use a PrefixKey (this is used by changes.transaction).
In the end what is happening here is that we use the hash of value as key and EMPTY_PREFIX is ignored.
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
|
/cmd prdoc --audience node_dev |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd fmt |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This pull requests implements support for ignoring trie nodes while recording a proof. It directly includes the feature into `basic-authorship` to later make use of it in Cumulus for multi-block PoVs. The idea behind this is when you have multiple blocks per PoV that trie nodes accessed or produced by a block before (in the same `PoV`), are not required to be added to the storage proof again. So, all the blocks in one `PoV` basically share the same storage proof. This also impacts things like storage weight reclaim, because ignored trie node do not contribute a to the storage proof size (similar to when this would happen in the same block). # Example Let's say block `A` access key `X` and block `B` accesses key `X` again. As `A` already has read it, we know that it is part of the storage proof and thus, don't need to add it again to the storage proof when building `B`. The same applies for storage values produced by an earlier block (in the same PoV). These storage values are an output of the execution and thus, don't need to be added to the storage proof :) Depends on #6137. Base branch will be changed when this got merged. Part of: #6495 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
This pull requests implements support for ignoring trie nodes while recording a proof. It directly includes the feature into
basic-authorshipto later make use of it in Cumulus for multi-block PoVs.The idea behind this is when you have multiple blocks per PoV that trie nodes accessed or produced by a block before (in the same
PoV), are not required to be added to the storage proof again. So, all the blocks in onePoVbasically share the same storage proof. This also impacts things like storage weight reclaim, because ignored trie node do not contribute a to the storage proof size (similar to when this would happen in the same block).Example
Let's say block
Aaccess keyXand blockBaccesses keyXagain. AsAalready has read it, we know that it is part of the storage proof and thus, don't need to add it again to the storage proof when buildingB. The same applies for storage values produced by an earlier block (in the same PoV). These storage values are an output of the execution and thus, don't need to be added to the storage proof :)Depends on #6137. Base branch will be changed when this got merged.
Part of: #6495