Eliminates MASP VP's requirement for all debited accounts to sign Tx

[murisi]

Describe your changes

This PR attempts to eliminate the MASP VP's requirement (mentioned in #3372 (comment)) for all debited accounts to sign a Tx. This is done by requiring only those accounts whose balances are modified by the inner Transaction and whose balances decrease to sign the transaction. The justification for this change is that in the case of accounts not modified by the inner Transaction, other VPs already validate all changes pertaining to the given address.

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

Namada 0.40.0

Checklist before merging to draft

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

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 53.47%. Comparing base (8479d38) to head (17b9fcf).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/ledger/native_vp/masp.rs	0.00%	28 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3516      +/-   ##
==========================================
- Coverage   53.48%   53.47%   -0.02%     
==========================================
  Files         320      320              
  Lines      110000   110000              
==========================================
- Hits        58832    58819      -13     
- Misses      51168    51181      +13     

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

[grarco]

Is this still required? I think with the new logic we don't need this: in fact, this might require some additional authorizers which are not needed

https://github.com/anoma/namada/blob/17b9fcfa9c85f5a115646120f6595e8cf28c38a7/crates/namada/src/ledger/native_vp/masp.rs#L975-L977

[murisi]

Is this still required? I think with the new logic we don't need this: in fact, this might require some additional authorizers which are not needed

https://github.com/anoma/namada/blob/17b9fcfa9c85f5a115646120f6595e8cf28c38a7/crates/namada/src/ledger/native_vp/masp.rs#L975-L977

I am not sure if the VP is as correct if we remove this insertion code. It might be, but I'm not sure yet. How are you reasoning about this? My thinking is as follows: so currently the overall logic is to validate a Tx by splitting it into two parts: the Transaction, and Tx - Transaction and validating those separately/independently - if both parts are correct then so is the whole. The signature verifications in this question are a part of validating the Transaction part - essentially we're making sure that all the transparent input addresses (let's say that an Albert is amongst them) in the Transaction have signed (directly or indirectly) over the entire Transaction object. Are there conditions under which we can avoid doing this check? I guess if the overall state change being validated by the MASP VP resulted in Albert's balance decreasing, then maybe we could avoid checking for signatures since Albert's VP is triggered to check the balance change anyway. But it seems the assumption here would be that Albert's VP (whether it's user, implicit, or something else) would always run logic to check that there's a signature (directly or indirectly) over the Transaction object. Am I missing something here?

What issues come up when integrating this PR with outsourcing? My thinking is that the above insertion is only triggered when a Transaction has transparent inputs (say Albert is amongst them). This means that either the overall state change being validated by the MASP VP debits Albert's account, or the Transaction has transparent outputs going to Albert, or the overall Tx contains some other credits to Albert that cancels out the transparent input. I want to think that the latter two cases are rare and can always be simplified (by netting) so that Albert is exclusively either being debited or credited in a Tx, but not both. In the former case (where the overall state change is a debit to Albert), I would think that your logic that automatically includes all debited accounts in the set of MASP authorizers (here https://github.com/anoma/namada/blob/cf9942a9ec4c9d965b052cedea190abf5d2331d2/wasm/tx_transfer/src/lib.rs#L36) would ensure that MaspAuthorizer actions are pushed and verifiers inserted for each transparent input in a Transaction. But I might also be missing something here... Also, thanks again for integrating these 2 PRs, let me know if I can assist in any other way.

[grarco]

Is this still required? I think with the new logic we don't need this: in fact, this might require some additional authorizers which are not needed
https://github.com/anoma/namada/blob/17b9fcfa9c85f5a115646120f6595e8cf28c38a7/crates/namada/src/ledger/native_vp/masp.rs#L975-L977

I am not sure if the VP is as correct if we remove this insertion code. It might be, but I'm not sure yet. How are you reasoning about this? My thinking is as follows: so currently the overall logic is to validate a Tx by splitting it into two parts: the Transaction, and Tx - Transaction and validating those separately/independently - if both parts are correct then so is the whole. The signature verifications in this question are a part of validating the Transaction part - essentially we're making sure that all the transparent input addresses (let's say that an Albert is amongst them) in the Transaction have signed (directly or indirectly) over the entire Transaction object. Are there conditions under which we can avoid doing this check? I guess if the overall state change being validated by the MASP VP resulted in Albert's balance decreasing, then maybe we could avoid checking for signatures since Albert's VP is triggered to check the balance change anyway. But it seems the assumption here would be that Albert's VP (whether it's user, implicit, or something else) would always run logic to check that there's a signature (directly or indirectly) over the Transaction object. Am I missing something here?

What issues come up when integrating this PR with outsourcing? My thinking is that the above insertion is only triggered when a Transaction has transparent inputs (say Albert is amongst them). This means that either the overall state change being validated by the MASP VP debits Albert's account, or the Transaction has transparent outputs going to Albert, or the overall Tx contains some other credits to Albert that cancels out the transparent input. I want to think that the latter two cases are rare and can always be simplified (by netting) so that Albert is exclusively either being debited or credited in a Tx, but not both. In the former case (where the overall state change is a debit to Albert), I would think that your logic that automatically includes all debited accounts in the set of MASP authorizers (here

https://github.com/anoma/namada/blob/cf9942a9ec4c9d965b052cedea190abf5d2331d2/wasm/tx_transfer/src/lib.rs#L36

) would ensure that MaspAuthorizer actions are pushed and verifiers inserted for each transparent input in a Transaction. But I might also be missing something here... Also, thanks again for integrating these 2 PRs, let me know if I can assist in any other way.

I believe your reasoning is correct, never mind. What happened was that in the rebase branch I've removed the commit that returns the debited accounts from multi_transfer because I thought we didn't need it since we don't want to push actions for all the debited accounts but only for those that have also been touched by the masp Transaction in a way that doesn't match the storage modification. I believe this is always the case in our tests (indeed if I remove the call to signer.insert the tests pass). In these cases we don't need the actions because the debit in the masp Transaction exactly matches the debit in the storage changes so the source's VP will be triggered anyway (this is what I meant with "additional authorizers" in my original comment). In our transactions we never use intermediaries so the masp actions should never be required.

Thinking twice about it though, as you say, I'm not sure this is always the case. Actually by running a simple calculation on a case where the shielding is redirected (someone shields on behalf of the original sender) the checks don't run for neither the original sender nor the new one if we remove that line. Now I'm not sure this is needed cause I can't see anyone tampering with a transaction to shield tokens on behalf of someone else and also we usually only requires extra validation for debit but I think keeping it is a safe fallback (also, we still want to prevent any sort of tampering)