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

MASP VP refactor #2452

Merged
merged 10 commits into from
Jan 30, 2024
Merged

MASP VP refactor #2452

merged 10 commits into from
Jan 30, 2024

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Jan 26, 2024

Describe your changes

This PR modifies the masp VP to validate the changed keys in storage instead of validating the Transfer object. It also updates the client to retrieve the data from the changed keys instead of the tx data since this doesn't get validated from the vp. Unfortunately it wasn't possible to completely remove the dependency on Transfer since we still need it for one thing, retrieve the masp Transaction object whose commitment is included in the tx data. Because of this, the masp VP only allows for a single value transfer when a transparent address is involved (whereas the other VPs technically support multiple transfers).

In addition, this PR also makes the operations in the masp VP checked. It has to be noted that operations on I128Sum underneath use check operations even though, in case of an over/under-flow, the application crashes. This solution is more desirable than the default behavior (wrapping around) but in our case it would be better to not crash and just discard the transaction. This type comes from the masp library so either we open an issue there to ask for a better support of checked operations or we rewrite the logic ourselves.

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

v0.30.2

Checklist before merging to draft

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

grarco added a commit that referenced this pull request Jan 26, 2024
@grarco grarco force-pushed the grarco/masp-vp-refactor branch from c1e43f6 to 70698c5 Compare January 26, 2024 16:07
@grarco grarco force-pushed the grarco/masp-vp-refactor branch from 70698c5 to 8ae3465 Compare January 26, 2024 16:48
@grarco grarco force-pushed the grarco/masp-vp-refactor branch from b01007a to 8dcdea2 Compare January 29, 2024 10:47
@grarco grarco marked this pull request as ready for review January 29, 2024 11:27
@grarco grarco requested review from murisi and cwgoes January 29, 2024 11:28
Fraccaman
Fraccaman previously approved these changes Jan 29, 2024
@Fraccaman Fraccaman mentioned this pull request Jan 29, 2024
Fraccaman pushed a commit that referenced this pull request Jan 29, 2024
* origin/grarco/masp-vp-refactor:
  Fixes validation of minted balance key in masp vp
  Changelog #2452
  Fmt
  Fixes benchmarks
  Refactors error in masp vp
  Updates SDK to retrieve changed keys instead of `Transfer` for masp
  Removes wrong comment from masp vp
  Masp vp validation based on changed keys
@cwgoes
Copy link
Collaborator

cwgoes commented Jan 29, 2024

In addition, this PR also makes the operations in the masp VP checked. It has to be noted that operations on I128Sum underneath use check operations even though, in case of an over/under-flow, the application crashes. This solution is more desirable than the default behavior (wrapping around) but in our case it would be better to not crash and just discard the transaction. This type comes from the masp library so either we open an issue there to ask for a better support of checked operations or we rewrite the logic ourselves.

This sounds like a potential DoS vector to me - what if someone crafts a transaction just to cause an over/under-flow and crash the ledger? This may be difficult to do, but it's much better defense-in-depth to just reject the transaction when an over/under-flow happens instead of halting. Perhaps you can ask @murisi for help with the library?

brentstone added a commit that referenced this pull request Jan 29, 2024
* origin/grarco/masp-vp-refactor:
  Checked operations in masp vp
  Fixes validation of minted balance key in masp vp
  Changelog #2452
  Fmt
  Fixes benchmarks
  Refactors error in masp vp
  Updates SDK to retrieve changed keys instead of `Transfer` for masp
  Removes wrong comment from masp vp
  Masp vp validation based on changed keys
brentstone added a commit that referenced this pull request Jan 29, 2024
* origin/grarco/masp-vp-refactor:
  Checked operations in masp vp
  Fixes validation of minted balance key in masp vp
  Changelog #2452
  Fmt
  Fixes benchmarks
  Refactors error in masp vp
  Updates SDK to retrieve changed keys instead of `Transfer` for masp
  Removes wrong comment from masp vp
  Masp vp validation based on changed keys
@brentstone brentstone merged commit ad44e2b into main Jan 30, 2024
13 of 15 checks passed
@brentstone brentstone deleted the grarco/masp-vp-refactor branch January 30, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants