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

refactor state crate #2606

Merged
merged 30 commits into from
Feb 27, 2024
Merged

refactor state crate #2606

merged 30 commits into from
Feb 27, 2024

Conversation

tzemanovic
Copy link
Member

@tzemanovic tzemanovic commented Feb 13, 2024

Describe your changes

closes #2486

High-level

  • Renamed struct State to struct InMemory and removed its field db: D together with D type param
    • Most methods on State that were using the db are moved to trait State and prefixed with db_ (e.g. State::read becomes State::db_read)
  • Removed struct WlStorage, struct TempWlStorage and trait WriteLogAndStorage
  • Added trait State generic over DB and hasher, comprised of a write log, DB and InMemory state.
  • Added struct FullAccessState (impls State, StorageRead and StorageWrite) - this is owned by shell which has full access to everything (DB, write log and InMemory state)
  • Added struct WlState (impls State, StorageRead and StorageWrite) - DB is read only, write/delete can only be done via write log
  • Added struct TempWlState (impls State, StorageRead and StorageWrite) - replaces TempWlStorage - for prepare and process proposal and dry-run txs which use a temporary write log
  • Added struct VpHostEnvState (impls State and StorageRead) - state for VP exec, everything is read-only (except for gas meter which is held in a RefCell to allow mutation in immutable methods)
  • Added struct TxHostEnvState (impls State, StorageRead and StorageWrite) - state for tx exec, can only write to write log, cannot modify in-memory state. Includes gas accounting (gas meter also in a RefCell).
  • struct TxCtx now hold reference to a generic mutable State
    • I couldn't find a way to make this work because the type has to have a 'static lifetime so anything that contains a non-static reference is a no-go. Instead, the TxCtx now has a state() method, which returns a TxHostEnvState that allows to use StorageRead + StorageWrite in host env with gas accounting included (replaces temporary implementation we've added for IBC tx workaround)
    • same deal with host_env fns, they all have be 'static to be able to use them in wasm_imports
  • struct VpCtx and native_vp::Ctx now hold a reference to a generic immutable State
    • Same as above, VpCtx has a state() method that returns VpHostEnvState
  • Replaced struct TestWlStorage by type TestState = FullAccessState<MockDB, Sha256Hasher>

Usage

  • From shell, we can restrict state access with:
    • FullAccessState::restrict_writes_to_write_log - returns &mut WlState for transactions and other state changes in a block
    • FullAccessState::read_only - returns &WlState for VPs and queries
  • trait State components access
    • State::write_log(), State::write_log_mut(), State::db(), State::in_mem() (InMemory state)
    • State::split_borrow to use all 3 at the same time
  • Most things benefit from using trait State in place of WlStorage to simplify the verbose bounds:
        DB: namada_state::DB + for<'iter> namada_state::DBIter<'iter>,
        H: StorageHasher,
    becomes
        S: State

Next steps

For sub-system modules envisioned in #2111 we'll want to replace the usage of WlStorage/WlState/State with the more abstract StorageRead + StorageWrite with additional traits that add access to individual parts of the state that are being used but not covered by StorageRead or StorageWrite (e.g. things from InMemory state - for shielded token we have trait WithConversionState that provides mutable access to conversion state (in the concrete FullAccessState this is held in struct InMemory). The only places that directly use the state will be:

  • The shell that owns the state
  • High-level ABCI handlers
  • Tx and VP environments (but not tx and VP implementations)
    These are governance, ethereum_bridge and ibc.

TODOs

  • make it build
  • replace the temporary StorageRead and StorageWrite impl for TxCtx in crates/namada/src/vm/host_env.rs with a impl provided by the state crate (must account for gas)
  • fix tests

Indicate on which release or other PRs this topic is based on

#2507 - diff https://github.com/anoma/namada/pull/2606/files/d44bf1e2c242e076ca96e23c1e27dfba663c12b2..daffb53860531bf589f00259c3e12d4e5ee3b395

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@tzemanovic tzemanovic force-pushed the tomas/refactor-state branch 5 times, most recently from 3afe3de to 2d4d95f Compare February 16, 2024 17:40
@tzemanovic tzemanovic force-pushed the tomas/refactor-state branch 3 times, most recently from 4f4803b to 4ec74dd Compare February 20, 2024 12:38
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: Patch coverage is 85.86586% with 373 lines in your changes are missing coverage. Please review.

Project coverage is 53.46%. Comparing base (c733be2) to head (daffb53).
Report is 79 commits behind head on main.

Files Patch % Lines
crates/ibc/src/actions.rs 0.00% 107 Missing ⚠️
crates/core/src/event.rs 47.87% 49 Missing ⚠️
crates/apps/src/lib/bench_utils.rs 0.00% 47 Missing ⚠️
crates/apps/src/lib/node/ledger/shell/mod.rs 78.86% 26 Missing ⚠️
...rates/apps/src/lib/node/ledger/shell/governance.rs 53.84% 24 Missing ⚠️
...tes/apps/src/lib/node/ledger/shell/testing/node.rs 0.00% 23 Missing ⚠️
...rc/protocol/transactions/ethereum_events/events.rs 88.94% 21 Missing ⚠️
crates/core/src/address.rs 75.00% 13 Missing ⚠️
crates/namada/src/ledger/mod.rs 69.44% 11 Missing ⚠️
crates/governance/src/storage/mod.rs 46.15% 7 Missing ⚠️
... and 23 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
+ Coverage   53.38%   53.46%   +0.08%     
==========================================
  Files         302      307       +5     
  Lines      103403   102898     -505     
==========================================
- Hits        55198    55012     -186     
+ Misses      48205    47886     -319     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tzemanovic added a commit that referenced this pull request Feb 23, 2024
tzemanovic added a commit that referenced this pull request Feb 23, 2024
* tomas/refactor-state:
  changelog: add #2606
  benches: update for new state API
  test/wasm: update for new state API
  tests: update for new state API
  test/PoS: update for new state API
  apps: update for new state API
  namada: update for new state API, refactor tx and VP host envs
  sdk: update for new state API
  eth_bridge: update for new state API
  ibc: update for new state API
  state: refactor everything
@tzemanovic
Copy link
Member Author

there's an issue with loading merkle tree after node restart, looking into it

crates/ibc/src/actions.rs Outdated Show resolved Hide resolved
@tzemanovic
Copy link
Member Author

there's an issue with loading merkle tree after node restart, looking into it

fixed in 1519e71

@tzemanovic
Copy link
Member Author

tzemanovic added a commit that referenced this pull request Feb 26, 2024
* tomas/refactor-state:
  changelog: add #2606
  benches: update for new state API
  test/wasm: update for new state API
  tests: update for new state API
  test/PoS: update for new state API
  apps: update for new state API
  namada: update for new state API, refactor tx and VP host envs
  sdk: update for new state API
  eth_bridge: update for new state API
  ibc: update for new state API
  state: refactor everything
@tzemanovic tzemanovic merged commit acdfd01 into main Feb 27, 2024
14 of 17 checks passed
@tzemanovic tzemanovic deleted the tomas/refactor-state branch February 27, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

state crate refactor
2 participants