Skip to content
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

sp-state-machine: Read value from backend when writing the first time #6230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Oct 24, 2024

This is a first step into fixing #6020. The underlying problem for parachains is that when calling storage_root, values that are written, will need to be looked up in the trie. This is required to insert the value or to remove it. The problem is that this lookup in storage_root increases the storage proof size, but storage reclaim for example can not track these lookups. This means that with storage reclaim it is possible to include more transactions than what the block should be allowed to include.
This pull request reads a key from the backend the first time it is written. This results in taking into account what storage_root is doing at the end of a block. However, this may still skips reading one extra node (depending on the trie structure). Skipping only one node is far less problematic than skipping the entire lookup. A future pull request will improve this further to take the entire operation of storage_root into account.

For chains that are not recording a storage proof, they will still do this extra backend read. However, this read will end up in the cache. At the end of the block the key will be read again as part of storage_root, but served from the cache. So, there is no downside of this.

This is a first step into fixing #6020.
The underlying problem for parachains is that when calling `storage_root`, values that
are written, will need to be looked up in the trie. This is required to insert the value
or to remove it. The problem is that this lookup in `storage_root` increases the storage
proof size, but storage reclaim for example can not track these lookups. This means that
with storage reclaim it is possible to include more transactions than what the block should
be allowed to include.
This pull request reads a key from the backend the first time it is written. This results in
taking into account what `storage_root` is doing at the end of a block. However, this may
still skips reading one extra `node` (depending on the trie structure). Skipping only one `node`
is far less problematic than skipping the entire lookup. A future pull request will improve this further
to take the entire operation of `storage_root` into account.

For chains that are not recording a storage proof, they will still do this extra backend read. However,
this read will end up in the cache. At the end of the block the key will be read again as part of
`storage_root`, but served from the cache. So, there is no downside of this.
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Oct 24, 2024
@bkchr bkchr requested a review from skunert October 24, 2024 22:04
@skunert
Copy link
Contributor

skunert commented Oct 25, 2024

Won't this break backwards compatibility? If storage reclaim now "sees" more read acesses than before, it will arrive at a different reclaim amount. All chains that have the reclaim enabled would arrive at a different state (i.e. BlockWeight) after importing old blocks with this change. Even though the final proof should be the same.

Edit: Same with the PVF, producing nodes would arrive at a different BlockWeight while the state machine in the PVF would use the old logic, failing to validate new blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants