Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-mmr: move offchain logic to client-side gadget #12508

Closed
Tracked by #1636
acatangiu opened this issue Oct 17, 2022 · 6 comments · Fixed by #12753
Closed
Tracked by #1636

pallet-mmr: move offchain logic to client-side gadget #12508

acatangiu opened this issue Oct 17, 2022 · 6 comments · Fixed by #12753
Assignees
Labels
I8-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint. U3-nice_to_have Issue is worth doing eventually. Z3-substantial Can be fixed by an experienced coder with a working knowledge of the codebase.

Comments

@acatangiu
Copy link
Contributor

MMR nodes are saved in off-chain db under fork-resistant keys based on frame_system::block_hash(), as such only nodes added by latest frame_system::BlockHashCount can be accessed that way.

Nodes added by blocks older than frame_system::BlockHashCount are moved by offchain_worker to "canon" (not fork resistant) location - thus being made available regardless of age.
See #11594 for more details.

PR #12498 fixed the issue that offchain_worker doesn't run for blocks imported during initial sync. So for initial syncs > frame_system::BlockHashCount (practically any node syncing from scratch), MMR nodes aren't moved to canon position and access to them is lost.
The fix was that runtime block execution saves node to both fork-resistant and canon locations:

  • initial sync nodes directly end up in the right location,
  • for the rest, we do a superfluous offchain_index::set() because it will be overwritten by offchain_worker with canon values anyway.

This results in working system, but with bloated offchain-db where MMR nodes added during initial sync are taking 2x storage than they should (they are duplicated in two locations in the offchain-db).

@acatangiu acatangiu added I8-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint. U3-nice_to_have Issue is worth doing eventually. Z3-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. labels Oct 17, 2022
@acatangiu acatangiu moved this to Draft in Parity Roadmap Oct 17, 2022
@acatangiu
Copy link
Contributor Author

Possible solution outlined here #12498 (comment):

  • change fork-resistant key to (prefix, pos, parent_hash) and hope (I need to verify) there is functionality to get all entries matching (prefix, pos) partial key,
  • keep offchain db entry last_pruned_leaf_number
  • in offchain worker attempt to prune (last_pruned_leaf_number, current - window] rather than just [current - window].
  • we can even do this in batches; put a limit on how many to clean up in each offchain_worker invocation so that we spread the load over time.

@acatangiu acatangiu moved this to Some Day Maybe in BEEFY Oct 17, 2022
@acatangiu acatangiu moved this from Some Day Maybe to Need for Polkadot 🗒 in BEEFY Oct 17, 2022
@acatangiu
Copy link
Contributor Author

Unfortunately, OFFCHAIN column uses default hash (and not btree) structure, meaning "key prefix" cannot be used for retrieving items 😢

@acatangiu
Copy link
Contributor Author

I think we need to create a client-side gadget, and move all the offchain stuff there.

PROs:

  • pallet-mmr is simplified, only cares about runtime state/storage,
  • no more offchain_worker canonicalization required; it is simplified and moved to client-side code/gadget,
  • we can actually follow finality and "properly" canonicalize offchain entries,
  • simplifies runtime api (generating proofs currently works only when --indexing-api is enabled) - generating proofs is handled by client-side gadget.

CONs:

  • extra gadget deployed client-side - pallet can maintain pruned MMR without the gadget, but cannot generate proofs without it.

@andresilva @svyatonik WDYT?

@svyatonik
Copy link
Contributor

I can't say much - @andresilva was there when it was designed that way (offchain worker) - maybe there's some strong reasoning behind that. But do you want to keep using offfchain storage for storing MMR nodes or regular DB?

@acatangiu
Copy link
Contributor Author

But do you want to keep using offfchain storage for storing MMR nodes or regular DB?

We would still use offchain column in the DB so that runtime can still add to it through indexing-api. That way, building the MMR still happens on-chain, but canonicalization and pruning of non-canon forks would be done by client-side gadget and could be driven by GRANDPA finality.

Also generating proofs RPC would not change, but instead of calling runtime API it would call some gadget API.

@acatangiu acatangiu self-assigned this Oct 28, 2022
@acatangiu
Copy link
Contributor Author

I have talked with @andresilva and we're in agreement that client-side gadget to handle all off-chain duties is the best way forward.

@acatangiu acatangiu moved this from Draft to Open in Parity Roadmap Nov 7, 2022
@acatangiu acatangiu assigned serban300 and unassigned acatangiu Nov 7, 2022
@acatangiu acatangiu moved this from Need for Polkadot 🗒 to Need for Kusama 🗒 in BEEFY Nov 7, 2022
@acatangiu acatangiu changed the title pallet-mmr: prune stale offchain db entries pallet-mmr: move offchain logic to client-side gadget Nov 10, 2022
@acatangiu acatangiu moved this from Need for Kusama 🗒 to In Progress 🛠 in BEEFY Nov 11, 2022
Repository owner moved this from In Progress 🛠 to Done ✅ in BEEFY Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I8-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint. U3-nice_to_have Issue is worth doing eventually. Z3-substantial Can be fixed by an experienced coder with a working knowledge of the codebase.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants