Fix temporary values read #2735

grarco · 2024-02-27T10:29:59Z

Describe your changes

Closes #2683.

Fixes the has_key method provided to the vp env to not return an error on temporary write values (the read method was already fine).

Adds a new read_persistent method on WriteLog that ignore temporary writes and modifies the read and has_key implementation of impl_storage_read to call this new method.
Modifies tx_iter_next to skip temporary writes.
Refactors fetch_or_compile to call state.read (which should include possible updated in the write log of the wasm codes).

Updates write_temp in WriteLog to return an error when trying to overwrite a StorageModification::Write with a StorageModification::Temp.
Updates iter_prefix_post in WriteLog to iterate also on the precommit bucket.
Updates write, write_temp and delete in WriteLog to take the precommit bucket into account.

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

v0.32.0

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

codecov · 2024-02-27T10:55:31Z

Codecov Report

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

Project coverage is 53.49%. Comparing base (5e0b162) to head (f588ec5).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/vm/wasm/run.rs	57.14%	12 Missing ⚠️
crates/namada/src/vm/host_env.rs	57.14%	6 Missing ⚠️
crates/state/src/write_log.rs	94.23%	6 Missing ⚠️
crates/namada/src/ledger/vp_host_fns.rs	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2735      +/-   ##
==========================================
+ Coverage   53.44%   53.49%   +0.05%     
==========================================
  Files         310      310              
  Lines      101574   101623      +49     
==========================================
+ Hits        54288    54368      +80     
+ Misses      47286    47255      -31

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

grarco · 2024-02-28T14:58:07Z

One thing I noticed is that here the type system doesn't really help us: the block_write_log will (and should) never contain a StorageModification::Temp because we filter them out, but since we are not using a different type this isn't enforced leading to some places in the code were we might match on a variant that doesn't really exist. We could think about coming up with a new type here

namada/crates/state/src/write_log.rs

Line 84 in 78519dd

pub(crate) block_write_log: HashMap<storage::Key, StorageModification>,

grarco · 2024-02-28T15:02:49Z

crates/namada/src/ledger/vp_host_fns.rs

@@ -188,7 +188,9 @@ where
            Ok(false)
        }
        Some(&write_log::StorageModification::InitAccount { .. }) => Ok(true),
-        Some(&write_log::StorageModification::Temp { .. }) => Ok(true),
+        Some(&write_log::StorageModification::Temp { .. }) => {


The API we provide to the VPs is different to the one we have in protocol or expose to transactions. For VPs we report an error whereas in protocol and for transactions we simply ignore temporary writes and try to look for other storage modifications or directly from storage. I tend to like the latter better but I can understand that returning an error might be more explicit. Should we try to unify the api and apply the same logic everywhere (either errors or read-through?)

I agree that the latter is better and I'm for unifying this in VPs. If a VP allows temp values, it has to read them explicitly so ignoring them for non-temp reads is fine

crates/state/src/write_log.rs

…efix_post`

…t into account

tzemanovic · 2024-03-20T12:12:48Z

rebased on 0.32.0

* origin/grarco/fix-vps-api: Changelog #2735 `read_persistent` returns a `PersistentStorageModification` Unit tests write log temp writes In write log `write`, `write_temp` and `delete` now take the precommit into account vp `read_temp` ignores non-temp values instead of error Unifies read interface of vps to that of txs and protocol Refactors `read` and `has_key` implementations in `impl_storage_read` Adds a new `read_persistent` method on `WriteLog`. Refactors `iter_prefix_post` Host fn iterator does not return temporary values Fixes check on address existence Fixes retrieval of wasm code in `fetch_or_compile` Ingores temporary writes in `impl_storage_read` macro Prevents a write log temp write after a write `has_key` host fns return error on temp write

* origin/grarco/fix-vps-api: Changelog #2735 `read_persistent` returns a `PersistentStorageModification` Unit tests write log temp writes In write log `write`, `write_temp` and `delete` now take the precommit into account vp `read_temp` ignores non-temp values instead of error Unifies read interface of vps to that of txs and protocol Refactors `read` and `has_key` implementations in `impl_storage_read` Adds a new `read_persistent` method on `WriteLog`. Refactors `iter_prefix_post` Host fn iterator does not return temporary values Fixes check on address existence Fixes retrieval of wasm code in `fetch_or_compile` Ingores temporary writes in `impl_storage_read` macro Prevents a write log temp write after a write `has_key` host fns return error on temp write # Conflicts: # crates/namada/src/vm/wasm/run.rs

grarco force-pushed the grarco/fix-vps-api branch from d32eb42 to 2cf6b23 Compare February 28, 2024 09:39

grarco commented Feb 28, 2024

View reviewed changes

grarco added a commit that referenced this pull request Feb 28, 2024

Changelog #2735

aa89818

grarco requested a review from tzemanovic February 28, 2024 15:06

grarco marked this pull request as ready for review February 28, 2024 16:19

grarco added the breaking-change label Feb 29, 2024

grarco force-pushed the grarco/fix-vps-api branch from aa89818 to 22f3c56 Compare February 29, 2024 14:54

grarco added a commit that referenced this pull request Feb 29, 2024

Changelog #2735

22f3c56

tzemanovic reviewed Mar 7, 2024

View reviewed changes

crates/state/src/write_log.rs Outdated Show resolved Hide resolved

grarco mentioned this pull request Mar 18, 2024

Type safety for block_write_log #2915

Closed

tzemanovic approved these changes Mar 20, 2024

View reviewed changes

tzemanovic added the ready for draft label Mar 20, 2024

tzemanovic mentioned this pull request Mar 20, 2024

Draft [breaking, mainnet] - released as 0.33.0 #2912

Closed

grarco added 14 commits March 20, 2024 12:06

has_key host fns return error on temp write

c1f9351

Prevents a write log temp write after a write

ee20066

Ingores temporary writes in impl_storage_read macro

8879eef

Fixes retrieval of wasm code in fetch_or_compile

31ea1a9

Fixes check on address existence

e0022be

Host fn iterator does not return temporary values

5ac9586

Adds a new read_persistent method on WriteLog. Refactors `iter_pr…

d1deea5

…efix_post`

Refactors read and has_key implementations in impl_storage_read

cf90067

Unifies read interface of vps to that of txs and protocol

55d7e96

vp read_temp ignores non-temp values instead of error

cbd1250

In write log write, write_temp and delete now take the precommi…

883b4a7

…t into account

Unit tests write log temp writes

14e1404

read_persistent returns a PersistentStorageModification

b3a593d

Changelog #2735

f588ec5

tzemanovic force-pushed the grarco/fix-vps-api branch from a0f479a to f588ec5 Compare March 20, 2024 12:11

tzemanovic added the merged to draft label Apr 1, 2024

tzemanovic merged commit d6a9c77 into main Apr 12, 2024
16 of 19 checks passed

tzemanovic deleted the grarco/fix-vps-api branch April 12, 2024 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix temporary values read #2735

Fix temporary values read #2735

grarco commented Feb 27, 2024 •

edited by tzemanovic

codecov bot commented Feb 27, 2024 •

edited

grarco commented Feb 28, 2024 •

edited

grarco Feb 28, 2024 •

edited

tzemanovic Mar 5, 2024

tzemanovic commented Mar 20, 2024

Fix temporary values read #2735

Fix temporary values read #2735

Conversation

grarco commented Feb 27, 2024 • edited by tzemanovic

Describe your changes

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

Checklist before merging to draft

codecov bot commented Feb 27, 2024 • edited

Codecov Report

grarco commented Feb 28, 2024 • edited

grarco Feb 28, 2024 • edited

Choose a reason for hiding this comment

tzemanovic Mar 5, 2024

Choose a reason for hiding this comment

tzemanovic commented Mar 20, 2024

grarco commented Feb 27, 2024 •

edited by tzemanovic

Checklist before merging to `draft`

codecov bot commented Feb 27, 2024 •

edited

grarco commented Feb 28, 2024 •

edited

grarco Feb 28, 2024 •

edited