Improve error messages returned by VP evaluation #2940

sug0 · 2024-03-21T18:04:04Z

Describe your changes

Closes #2746.

This PR changes the following:

VPs went from returning a result of booleans and errors to a result of unit and errors. On success, a unit is returned, and in the case of a tx validation failure, a descriptive error message is returned. This should improve both DX and UX.
A couple of new abstractions have been implemented for booleans and results, to handle erroring out from false or Ok(false).
The VP sentinel was removed in favor of downcasting storage errors to out-of-gas errors or signature verification failure errors.

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

#2852

Diff for review: https://github.com/anoma/namada/compare/tiago/vps-use-storage-err..tiago/wasm-errs-ux

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

sug0 · 2024-03-21T18:43:34Z

~~vp_stack_limiter failing on x86_64 linux :|~~

codecov · 2024-03-22T10:48:15Z

Codecov Report

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

Project coverage is 53.13%. Comparing base (5e0b162) to head (620d3d8).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/ledger/governance/mod.rs	0.00%	456 Missing ⚠️
crates/namada/src/ledger/native_vp/masp.rs	0.00%	155 Missing ⚠️
crates/namada/src/ledger/pgf/mod.rs	0.00%	104 Missing ⚠️
crates/vp_prelude/src/lib.rs	35.77%	79 Missing ⚠️
crates/namada/src/ledger/native_vp/mod.rs	36.00%	48 Missing ⚠️
crates/namada/src/ledger/pos/vp.rs	0.00%	42 Missing ⚠️
crates/namada/src/ledger/native_vp/multitoken.rs	62.68%	25 Missing ⚠️
...ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs	83.46%	21 Missing ⚠️
crates/namada/src/ledger/native_vp/parameters.rs	27.27%	16 Missing ⚠️
crates/namada/src/vm/host_env.rs	83.15%	16 Missing ⚠️
... and 14 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2940      +/-   ##
==========================================
- Coverage   53.44%   53.13%   -0.31%     
==========================================
  Files         310      311       +1     
  Lines      101574   102095     +521     
==========================================
- Hits        54288    54251      -37     
- Misses      47286    47844     +558

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

.changelog/unreleased/improvements/2940-wasm-errs-ux.md

crates/namada/src/ledger/governance/mod.rs

tzemanovic · 2024-03-26T22:18:35Z

ah, the make test-wasm are broken in vp_implicit and vp_user

tzemanovic · 2024-03-26T22:51:22Z

ah, the make test-wasm are broken in vp_implicit and vp_user

This was due to the change in host_env fn vp_verify_tx_section_signature that now returns an Err instead of HostEnvResult on invalid sig and because the native_host_fn! test helpers unwrap results of host env fns it leads to panics in tests - this is very hard to do differently because the function has to be extern "C"

fixed in 6cad262 - the tests now have to catch panics instead

This change permits error messages to be encoded in the `Err` branch of `Result` types, to facilitate debugging in case of software problems, and to improve the user experience in Namada.

* tiago/wasm-errs-ux: Changelog for #2940 Generalize VP status codes Add `bitflags` dep Poison tx gas meter on gas overflow Prevent gas overflows from being ignored during VP execution Remove explicit out-of-gas checks during VP execution Rebuild wasms for tests Fix wasm unit tests Return a result of unit from VPs rather than of bools Extend bools and results with new helper methods

* origin/tomas/tx-actions: changelog: add #2934 wasm/vp_implicit+vp_user: re-order to read the main entrypoint first IBC: insert NUT token as verifier in IBC transfer wasm/vp_implicit+vp_user: update using tx action vp_prelude: re-export tx mod multitoken: replace write_log specialization with standard VP rules pgf: add tx actions to check authorization gov: add tx actions to check authorization write_log: skip temp storage keys in changed keys set native_vp/PoS: update to use actions from txs tx_prelude/pos: push tx actions to indicate to VP namada/vm/host_env: add `tx_read_temp` tx,vp_prelude,native_vp: impl tx action Read/Write traits tx: add actions to indicate them from tx to VPs core/address: add `InternalAddress::TempStorage` Changelog for #2940 Remove explicit out-of-gas checks during VP execution Rebuild wasms for tests Return a result of unit from VPs rather than of bools

* origin/tiago/wasm-errs-ux: Changelog for #2940 Generalize VP status codes Add `bitflags` dep Poison tx gas meter on gas overflow Prevent gas overflows from being ignored during VP execution Remove explicit out-of-gas checks during VP execution Rebuild wasms for tests Fix wasm unit tests Return a result of unit from VPs rather than of bools Extend bools and results with new helper methods # Conflicts: # Cargo.lock # crates/apps/src/lib/node/ledger/shell/finalize_block.rs # crates/benches/vps.rs # crates/macros/src/lib.rs # crates/namada/src/ledger/governance/mod.rs # crates/namada/src/ledger/native_vp/ethereum_bridge/vp.rs # crates/namada/src/ledger/native_vp/ibc/mod.rs # crates/namada/src/ledger/native_vp/masp.rs # crates/namada/src/ledger/native_vp/multitoken.rs # crates/namada/src/ledger/protocol/mod.rs # crates/namada/src/ledger/vp_host_fns.rs # crates/namada/src/vm/wasm/run.rs # crates/tests/src/e2e/ledger_tests.rs # crates/tests/src/vm_host_env/ibc.rs # wasm/Cargo.lock # wasm/wasm_source/src/vp_user.rs # wasm_for_tests/tx_fail.wasm # wasm_for_tests/tx_memory_limit.wasm # wasm_for_tests/tx_mint_tokens.wasm # wasm_for_tests/tx_no_op.wasm # wasm_for_tests/tx_proposal_code.wasm # wasm_for_tests/tx_proposal_ibc_token_inflation.wasm # wasm_for_tests/tx_proposal_masp_reward.wasm # wasm_for_tests/tx_read_storage_key.wasm # wasm_for_tests/tx_write.wasm # wasm_for_tests/tx_write_storage_key.wasm # wasm_for_tests/vp_always_false.wasm # wasm_for_tests/vp_always_true.wasm # wasm_for_tests/vp_eval.wasm # wasm_for_tests/vp_memory_limit.wasm # wasm_for_tests/vp_read_storage_key.wasm # wasm_for_tests/wasm_source/Cargo.lock

* origin/tomas/tx-actions: changelog: add #2934 wasm/vp_implicit+vp_user: re-order to read the main entrypoint first IBC: insert NUT token as verifier in IBC transfer wasm/vp_implicit+vp_user: update using tx action vp_prelude: re-export tx mod multitoken: replace write_log specialization with standard VP rules pgf: add tx actions to check authorization gov: add tx actions to check authorization write_log: skip temp storage keys in changed keys set native_vp/PoS: update to use actions from txs tx_prelude/pos: push tx actions to indicate to VP namada/vm/host_env: add `tx_read_temp` tx,vp_prelude,native_vp: impl tx action Read/Write traits tx: add actions to indicate them from tx to VPs core/address: add `InternalAddress::TempStorage` Changelog for #2940 Remove explicit out-of-gas checks during VP execution Rebuild wasms for tests Return a result of unit from VPs rather than of bools

* origin/tomas/tx-actions: changelog: add #2934 wasm/vp_implicit+vp_user: re-order to read the main entrypoint first IBC: insert NUT token as verifier in IBC transfer wasm/vp_implicit+vp_user: update using tx action vp_prelude: re-export tx mod multitoken: replace write_log specialization with standard VP rules pgf: add tx actions to check authorization gov: add tx actions to check authorization write_log: skip temp storage keys in changed keys set native_vp/PoS: update to use actions from txs tx_prelude/pos: push tx actions to indicate to VP namada/vm/host_env: add `tx_read_temp` tx,vp_prelude,native_vp: impl tx action Read/Write traits tx: add actions to indicate them from tx to VPs core/address: add `InternalAddress::TempStorage` Changelog for #2940 Remove explicit out-of-gas checks during VP execution Rebuild wasms for tests Return a result of unit from VPs rather than of bools # Conflicts: # crates/benches/native_vps.rs # crates/benches/vps.rs # crates/core/src/validity_predicate.rs # crates/ibc/src/actions.rs # crates/ibc/src/context/token_transfer.rs # crates/ibc/src/context/transfer_mod.rs # crates/ibc/src/lib.rs # crates/macros/src/lib.rs # crates/namada/src/ledger/governance/mod.rs # crates/namada/src/ledger/native_vp/ethereum_bridge/vp.rs # crates/namada/src/ledger/native_vp/ibc/mod.rs # crates/namada/src/ledger/native_vp/masp.rs # crates/namada/src/ledger/native_vp/multitoken.rs # crates/namada/src/ledger/pgf/mod.rs # crates/namada/src/ledger/pos/vp.rs # crates/namada/src/ledger/protocol/mod.rs # crates/namada/src/ledger/vp_host_fns.rs # crates/namada/src/vm/host_env.rs # crates/namada/src/vm/wasm/run.rs # crates/state/src/write_log.rs # crates/tests/src/e2e/ledger_tests.rs # crates/tests/src/vm_host_env/ibc.rs # crates/tests/src/vm_host_env/mod.rs # crates/tx_prelude/src/ibc.rs # wasm/wasm_source/src/tx_resign_steward.rs # wasm/wasm_source/src/tx_update_steward_commission.rs # wasm/wasm_source/src/vp_implicit.rs # wasm/wasm_source/src/vp_user.rs # wasm_for_tests/tx_fail.wasm # wasm_for_tests/tx_memory_limit.wasm # wasm_for_tests/tx_mint_tokens.wasm # wasm_for_tests/tx_no_op.wasm # wasm_for_tests/tx_proposal_code.wasm # wasm_for_tests/tx_proposal_ibc_token_inflation.wasm # wasm_for_tests/tx_proposal_masp_reward.wasm # wasm_for_tests/tx_read_storage_key.wasm # wasm_for_tests/tx_write.wasm # wasm_for_tests/tx_write_storage_key.wasm # wasm_for_tests/vp_always_false.wasm # wasm_for_tests/vp_always_true.wasm # wasm_for_tests/vp_eval.wasm # wasm_for_tests/vp_memory_limit.wasm # wasm_for_tests/vp_read_storage_key.wasm

sug0 added 6 commits March 22, 2024 13:37

Add a simple allocated storage err

d602f7d

Remove eyre from Ethereum Bridge param errs

9708463

Remove eyre from Ethereum Bridge VP

440c76d

Remove eyre from NUT VP

3bcfe7b

Remove eyre from Ethereum Bridge Pool VP

29e44be

Changelog for #2852

18b2f75

sug0 force-pushed the tiago/wasm-errs-ux branch 2 times, most recently from 7b18a19 to 883e787 Compare March 25, 2024 10:43

sug0 changed the title ~~Tiago/wasm errs ux~~ Improve error messages returned by VP evaluation Mar 25, 2024

sug0 added a commit that referenced this pull request Mar 25, 2024

Changelog for #2940

7df7d49

sug0 added enhancement New feature or request ledger client breaking-change labels Mar 25, 2024

sug0 requested review from tzemanovic, batconjurer and grarco March 25, 2024 10:51

sug0 marked this pull request as ready for review March 25, 2024 10:51

sug0 mentioned this pull request Mar 25, 2024

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

Closed

tzemanovic reviewed Mar 25, 2024

View reviewed changes

.changelog/unreleased/improvements/2940-wasm-errs-ux.md Outdated Show resolved Hide resolved

sug0 marked this pull request as draft March 25, 2024 12:58

tzemanovic reviewed Mar 25, 2024

View reviewed changes

crates/namada/src/ledger/governance/mod.rs Outdated Show resolved Hide resolved

Extend bools and results with new helper methods

c5e8a3b

sug0 added a commit that referenced this pull request Mar 25, 2024

Changelog for #2940

0eaeb5a

sug0 force-pushed the tiago/wasm-errs-ux branch from 7df7d49 to 0eaeb5a Compare March 25, 2024 13:22

sug0 added a commit that referenced this pull request Mar 25, 2024

Changelog for #2940

35b5aee

sug0 force-pushed the tiago/wasm-errs-ux branch from 78fff10 to 35b5aee Compare March 25, 2024 13:24

sug0 marked this pull request as ready for review March 25, 2024 13:25

tzemanovic mentioned this pull request Mar 26, 2024

VP update using actions enum written by txs to a temp storage #2934

Merged

8 tasks

sug0 mentioned this pull request Mar 26, 2024

Improve error messages returned by TX execution #2958

Merged

2 tasks

sug0 added the UX label Mar 26, 2024

sug0 and others added 5 commits March 27, 2024 09:02

Return a result of unit from VPs rather than of bools

70e75a6

This change permits error messages to be encoded in the `Err` branch of `Result` types, to facilitate debugging in case of software problems, and to improve the user experience in Namada.

Fix wasm unit tests

93b4b69

Rebuild wasms for tests

c9567e9

Remove explicit out-of-gas checks during VP execution

55c4c8a

Prevent gas overflows from being ignored during VP execution

18f1203

sug0 added a commit that referenced this pull request Mar 27, 2024

Changelog for #2940

cf4466d

sug0 force-pushed the tiago/wasm-errs-ux branch from 6cad262 to cf4466d Compare March 27, 2024 09:26

sug0 added 4 commits March 27, 2024 09:40

Poison tx gas meter on gas overflow

7f38baa

Add bitflags dep

056e6ec

Generalize VP status codes

bc8fec5

Changelog for #2940

620d3d8

sug0 force-pushed the tiago/wasm-errs-ux branch from cf4466d to 620d3d8 Compare March 27, 2024 11:06

sug0 requested a review from grarco March 27, 2024 11:06

sug0 mentioned this pull request Mar 27, 2024

Remove tx error sentinel #2962

Open

tzemanovic approved these changes Apr 1, 2024

View reviewed changes

tzemanovic added a commit that referenced this pull request Apr 8, 2024

fixup! Merge branch 'tiago/wasm-errs-ux' (#2940)

d52c924

tzemanovic added the merged to draft label Apr 8, 2024

brentstone mentioned this pull request Apr 9, 2024

Issue: Inconsistencies in Transaction Processing and Error Reporting #2659

Closed

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

tzemanovic deleted the tiago/wasm-errs-ux branch April 12, 2024 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error messages returned by VP evaluation #2940

Improve error messages returned by VP evaluation #2940

sug0 commented Mar 21, 2024 •

edited by brentstone

sug0 commented Mar 21, 2024 •

edited

codecov bot commented Mar 22, 2024 •

edited

tzemanovic commented Mar 26, 2024

tzemanovic commented Mar 26, 2024

Improve error messages returned by VP evaluation #2940

Improve error messages returned by VP evaluation #2940

Conversation

sug0 commented Mar 21, 2024 • edited by brentstone

Describe your changes

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

Checklist before merging to draft

sug0 commented Mar 21, 2024 • edited

codecov bot commented Mar 22, 2024 • edited

Codecov Report

tzemanovic commented Mar 26, 2024

tzemanovic commented Mar 26, 2024

sug0 commented Mar 21, 2024 •

edited by brentstone

Checklist before merging to `draft`

sug0 commented Mar 21, 2024 •

edited

codecov bot commented Mar 22, 2024 •

edited