Convert descriptions anchor validation #2248
Conversation
…ate tree from the tx
grarco
force-pushed
the
grarco/convert-description-validation
branch
from
cf53ba9 to
463bfc8
Compare
| .expect("Cannot obtain a storage key"); | ||
| wl_storage.write( | ||
| &anchor_key, | ||
| crate::types::hash::Hash( |
There was a problem hiding this comment.
We can abstract all of this out into a hash function on the tree type, perhaps?
There was a problem hiding this comment.
Ah the FrozenCommitmentTree is imported from the masp crate but we can probably refactor this part of the code in other ways
| let current_tx_key = Key::from(masp_addr.to_db_key()) | ||
| .push(&(TX_KEY_PREFIX.to_owned() + ¤t_tx_idx.to_string())) | ||
| .expect("Cannot obtain a storage key"); | ||
| // Save the Transfer object and its location within the blockchain |
There was a problem hiding this comment.
Do we really need to store the entire transaction data? Seems wasteful. Also, can't this be queried from Tendermint?
There was a problem hiding this comment.
We don't need this data in protocol but we need it for the clients to scan transactions and generate a valid internal state to produce future transactions. I agree with you that this could bloat the storage pretty quick (together with the nullifier set), but I think there are a few concerns with querying this data from comet:
- I'm not sure we can assume that the blocks will be always available (in case of a chain restart for example if we decided to prune history)
- Requesting blocks from full nodes could take a long time/bandwidth
- The time to process the blocks filtering out only masp transactions could be long
Still we might prefer this solution over writing to storage. Wdyt? Alternatively we could think about a way to prune transactions from storage when they are not useful anymore (e.g. all the output descriptions have been spent)
There was a problem hiding this comment.
We should be able to use the indexer to pre-filter for MASP transaction data so that clients need only download and scan that. It's true that we need to keep around data after chain upgrades, but this doesn't seem too difficult.
| let head_tx_key = Key::from(masp_addr.to_db_key()) | ||
| .push(&HEAD_TX_KEY.to_owned()) | ||
| .expect("Cannot obtain a storage key"); | ||
| let current_tx_idx: u64 = |
There was a problem hiding this comment.
If we are going to write transactions to storage like this, all of this incrementing current tx index logic should be abstracted into a common function.
* origin/grarco/convert-description-validation: Stricter checks on sapling bundle components Changelog #2248 Fixes masp key validation Fixes conversion anchor handling Removes redundant masp dependency from bench crate Improves masp vp keys verification Masp VP verifies the anchors of convert descriptions `update_allowed_conversions` to publish the updated convert anchor
Describe your changes
Closes #1373.
Modifies
update_allowed_conversionsto update the anchor in storage at every epoch.Updates the masp vp to check the validity of the convert descriptions' anchors and improves how the masp vp validates the changed keys.
Also updates the VP to run stricter checks on the sapling bundle composition (addressing #2244 (comment))
Indicate on which release or other PRs this topic is based on
#2244
Checklist before merging to
draft