Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

feat: global state root 🔥 #667

Merged
merged 71 commits into from
Jun 28, 2023

Conversation

EvolveArt
Copy link
Collaborator

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Bugfix
    X Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Testing
  • Other (please describe):

What is the current behavior?

Resolves: #166

What is the new behavior?

  • Adds SCALE encoding for the StateCommitmentTree structure
  • Adds State struct in runtime which contains the two StateCommitmentTree : classes/contracts trees.
  • Upgrades a few dependencies to latest version scale-info, scale-coded and bitvec
  • Modify BlockifierStateAdapter such that it updates the state trees accordingly.
  • Implements IntermediateStateRoot to compute the global state root from the intermediate trees.

Does this introduce a breaking change?

Yes

Other information

  • A few things still need to be investigated :
  1. Is it fine to store these trees within the runtime ? Discussed with Lucas and spent a few hours thinking about # approaches but it seems unavoidable.
  2. When should the tree updates happen ? Incrementally like in this implementation or all at the same time, If we take into account the Storage Overlay (caching layer of substrate) then reading through all the state and building the trees shouldn't be that expensive ?
  3. How can we SCALE encode RefCell ? Does it even make sense or should we replace by Box ?
  4. BitVec has no MaxEncodedLen which makes sense but prevents from storing it within the runtime, so we can either create some BoundedBitVec wrapper or switch to BoundedVec<bool> for instance.

@EvolveArt EvolveArt added help wanted Extra attention is needed feature New feature labels Jun 16, 2023
@EvolveArt EvolveArt self-assigned this Jun 16, 2023
@tdelabro
Copy link
Collaborator

Does computing it in a single go produces the exact same result that tax by tx?

I can't think of any drawbacks and see a lot of pros as it allows us to skip storing all the intermediate states.

@tdelabro
Copy link
Collaborator

Ideally (in a few months) this should be computed outside of the runtime, and maybe for block b-1.

So I would advise for the solution that is as decoupled as possible from substrate logic.

@tdelabro
Copy link
Collaborator

You can disable the scale max length constraint. Either for one single storage or for the whole pallet.

It should not cause any trouble, especially if we kill the storage at the end of the block execution

@EvolveArt
Copy link
Collaborator Author

Does computing it in a single go produces the exact same result that tax by tx?

I can't think of any drawbacks and see a lot of pros as it allows us to skip storing all the intermediate states.

I mean remember what we we did originally to compute the storage we read everything from storage at each block and that was slowing down a lot.

What do we need here to compute tu global state root ?

  • read all the storage keys/values
  • read all contract classes along with their class hashes
  • read all the nonces along with their contract addresses

Having an iterative process sounds more performant than this as reads are O(log n) without cache

@tdelabro
Copy link
Collaborator

The problem was we were reading everything every tx. Now we read only what we need, but at each tx.

If we do this at the end in onfinalize rather than along the execution, it will be exactly the same amount of computing. Won't it ?
And we will spare the burden of storing/loading/making compatible the each transitive state.

@EvolveArt
Copy link
Collaborator Author

Following some discussion with @tdelabro and Mikro from Equilibrium who wrote the initial MPT implementation, I decided to replace RefCell by Box within the MPT implementation. Thus, it is SCALE encodable!

Second thing, I had to do a PR to scale-info repo in order to add a TypeInfo implementation for Rc, while the PR is not merged we can use my fork.

With these two things in place the trees are now persisted through time and we can update them iteratively, this sounds like the best approach until we adopt a client-first approach in the next few months.

crates/pallets/starknet/src/state_root.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/state_root.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/tests/mock.rs Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
@EvolveArt EvolveArt requested a review from tdelabro June 27, 2023 12:06
@EvolveArt EvolveArt force-pushed the evolve/state-root branch from b132228 to 71b22bf Compare June 27, 2023 12:53
@EvolveArt EvolveArt force-pushed the evolve/state-root branch from 71b22bf to 851db0d Compare June 27, 2023 13:02
Cargo.toml Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Show resolved Hide resolved
crates/pallets/starknet/src/lib.rs Show resolved Hide resolved
crates/primitives/starknet/src/tests/crypto.rs Outdated Show resolved Hide resolved
crates/primitives/starknet/src/tests/crypto.rs Outdated Show resolved Hide resolved
tests/tests/test-rpc/test-starknet-rpc.ts Show resolved Hide resolved
tests/tests/test-rpc/test-starknet-rpc.ts Show resolved Hide resolved
Copy link
Collaborator Author

@EvolveArt EvolveArt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@0xLucqs 0xLucqs dismissed stale reviews from tdelabro and AbdelStark June 28, 2023 12:34

I'm the boss

@EvolveArt EvolveArt merged commit fea86ad into keep-starknet-strange:main Jun 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants