Masp client invalidates spent notes #2534
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grarco
force-pushed
the
grarco/invalidate-masp-notes-in-client
branch
from
8d97f3d to
8d8893b
Compare
batconjurer
reviewed
Feb 9, 2024
| @@ -113,6 +113,26 @@ pub const OUTPUT_NAME: &str = "masp-output.params"; | |||
| /// Convert circuit name | |||
| pub const CONVERT_NAME: &str = "masp-convert.params"; | |||
|
|
|||
| /// Type alias for convenience and profit | |||
grarco
force-pushed
the
grarco/invalidate-masp-notes-in-client
branch
from
8d8893b to
44b4dd8
Compare
2 tasks
batconjurer
requested changes
Feb 12, 2024
crates/sdk/src/tx.rs
Outdated
Comment on lines
2495
to
2523
| if updated_balance.source == source && updated_balance.token == args.token { | ||
| if validated_amount.amount() > updated_balance.post_balance { | ||
| if args.tx.force { | ||
| edisplay_line!( | ||
| context.io(), | ||
| "The balance of the source {} of token {} is lower than \ | ||
| the amount to be transferred. Amount to transfer is {} \ | ||
| and the balance is {}.", | ||
| source, | ||
| args.token, | ||
| context | ||
| .format_amount(&args.token, validated_amount.amount()) | ||
| .await, | ||
| context | ||
| .format_amount( | ||
| &args.token, | ||
| updated_balance.post_balance | ||
| ) | ||
| .await, | ||
| ); | ||
| } else { | ||
| return Err(Error::from(TxSubmitError::BalanceTooLow( | ||
| source.clone(), | ||
| args.token.clone(), | ||
| validated_amount.amount().to_string_native(), | ||
| updated_balance.post_balance.to_string_native(), | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
this code block is repeated a few times. Can we factor it out into a separate func?
There was a problem hiding this comment.
I've moved this logic inside check_balance_too_low_err
grarco
force-pushed
the
grarco/invalidate-masp-notes-in-client
branch
from
44b4dd8 to
2a437de
Compare
grarco
force-pushed
the
grarco/invalidate-masp-notes-in-client
branch
from
2a437de to
8f5e41b
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2534 +/- ##
==========================================
- Coverage 53.38% 53.11% -0.28%
==========================================
Files 302 303 +1
Lines 103398 103937 +539
==========================================
+ Hits 55203 55207 +4
- Misses 48195 48730 +535 ☔ View full report in Codecov by Sentry. |
batconjurer
approved these changes
Feb 13, 2024
This was referenced Feb 13, 2024
Closed
Fraccaman
pushed a commit
that referenced
this pull request
Feb 19, 2024
* origin/grarco/invalidate-masp-notes-in-client: Refactors `check_balance_too_low_err` to accept either a balance key or a balance Changelog #2534 Refactors the `ShieldedUtils` trait Fixes e2e test Updates masp proofs Invalidate spent keys when generating masp transactions Returns error if the provided gas-spending-key is not needed Improves tests of fee unshielding [chore]: Fix integration masp test fixtures I don't know [chore] Rebase on main [fix] Some e2e test fixes Moved fetch calls completely from other calls. Updated cli. Fixed tests Added changelog entry. Changed the note scanning algorithm to not require additional context.
Fraccaman
pushed a commit
that referenced
this pull request
Feb 19, 2024
* origin/grarco/invalidate-masp-notes-in-client: Refactors `check_balance_too_low_err` to accept either a balance key or a balance Changelog #2534 Refactors the `ShieldedUtils` trait Fixes e2e test Updates masp proofs Invalidate spent keys when generating masp transactions Returns error if the provided gas-spending-key is not needed Improves tests of fee unshielding [chore]: Fix integration masp test fixtures I don't know [chore] Rebase on main [fix] Some e2e test fixes Moved fetch calls completely from other calls. Updated cli. Fixed tests Added changelog entry. Changed the note scanning algorithm to not require additional context.
Fraccaman
pushed a commit
that referenced
this pull request
Feb 20, 2024
* origin/grarco/invalidate-masp-notes-in-client: Refactors `check_balance_too_low_err` to accept either a balance key or a balance Changelog #2534 Refactors the `ShieldedUtils` trait Fixes e2e test Updates masp proofs Invalidate spent keys when generating masp transactions Returns error if the provided gas-spending-key is not needed Improves tests of fee unshielding [chore]: Fix integration masp test fixtures I don't know [chore] Rebase on main [fix] Some e2e test fixes Moved fetch calls completely from other calls. Updated cli. Fixed tests Added changelog entry. Changed the note scanning algorithm to not require additional context.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Modifies the client to invalidate the spend notes after their usage, to allow the construction of consecutive valid masp transactions (mandatory requirement for fee unshielding and masp tx with the same source key).
The
ShieldedContextcan now be in two states:ConfirmedorSpeculative. AConfirmedstate is one coming from asyncoperation, meaning that the data in the context comes from transaction that have been accepted by the protocol. When callinggen_shielded_transfer, instead, the context invalidates any spend notes used and transitions to aSpeculativestate (i.e. not that is not guaranteed to che coherent with the protocol). The speculative state is stored in a separate file and is always reload instead of the confirmed one when present. When callingfetchtheConfirmedstate is reloaded from disk to discard any possible speculative state and the fetch operates on this. Finally, when saving the newly synced confirmed state to storage, the file of the speculative state is removed if found.Improved previous integration tests to be more strict and added new ones.
Indicate on which release or other PRs this topic is based on
#2458 rebased on v0.31.3
Checklist before merging to
draft