Outsource masp signature verification

[grarco]

Describe your changes

Closes #3312.

Swaps the signature verification call in the masp vp with a check on the triggered vps. Introduces a new masp Action to support signature verification in the affected vps since we expect them to not be triggered by the transaction itself (no changed keys).

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

v0.40.0

Checklist before merging to draft

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

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 12.14953% with 94 lines in your changes missing coverage. Please review.

Project coverage is 53.46%. Comparing base (8479d38) to head (fc63450). Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/ledger/native_vp/masp.rs	0.00%	82 Missing :warning:
crates/tx/src/action.rs	54.54%	5 Missing :warning:
crates/trans_token/src/storage.rs	0.00%	4 Missing :warning:
crates/tx_prelude/src/token.rs	0.00%	3 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3372      +/-   ##
==========================================
- Coverage   53.48%   53.46%   -0.03%     
==========================================
  Files         320      320              
  Lines      110000   110031      +31     
==========================================
- Hits        58832    58824       -8     
- Misses      51168    51207      +39     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[grarco]

Ok so I've pushed some more commits, to recap:

• In the masp vp we check that the vp of the signer has been triggered and also check that the corresponding action has been pushed (otherwise the vp might run a different validation logic, we have to inform it that it is required to validate a masp transaction)
• I've talked to @tzemanovic for the solution he proposed (multiple actions to describe what's actually happening). We concluded that for now we can go with the current solution of just pushing a single action (@murisi I think you agreed in your previous comment that this is safe and we don't need more detailed data). His suggestion though could be taken into consideration for future updated in case we want to allow for more complicated validation logic from the implicit/user vps side
• TX_TRANSFER_WASM has been updated to push the required MaspAction::MaspAuthorizer actions and trigger the vps
• The multi_transfer function has been updated to return the set of addresses whose balance has gone down
• The Transfer type has been extended to carry an additional set of masp verifiers. This is because the solution here above doesn't cover all the cases, for example, it does not cover the tamper attack that Murisi has explained before, therefore we need the transaction to trigger these extra vps too

One small annoyance, the masp vp considers required authorizers all the addresses whose balance has decreased: this includes balances that had nothing to do with the MASP in that transaction. Because of this we are forced to push actions for these addresses too: at the moment both the implicit and user vps just check for the signature which coincides with the check for a normal balance decrease, but in case the verification changed (for example in some custom vp), then this could run a validation logic which is not correct for the actual changes.

[murisi]

One small annoyance, the masp vp considers required authorizers all the addresses whose balance has decreased: this includes balances that had nothing to do with the MASP in that transaction. Because of this we are forced to push actions for these addresses too: at the moment both the implicit and user vps just check for the signature which coincides with the check for a normal balance decrease, but in case the verification changed (for example in some custom vp), then this could run a validation logic which is not correct for the actual changes.

Let me work on a separate PR to fix the MASP VP so that addresses unrelated to the MASP Transaction do not require a signature when their balances decrease. I think this should be a very minor change that does not conflict with this PR.

[murisi]

One small annoyance, the masp vp considers required authorizers all the addresses whose balance has decreased: this includes balances that had nothing to do with the MASP in that transaction. Because of this we are forced to push actions for these addresses too: at the moment both the implicit and user vps just check for the signature which coincides with the check for a normal balance decrease, but in case the verification changed (for example in some custom vp), then this could run a validation logic which is not correct for the actual changes.

Let me work on a separate PR to fix the MASP VP so that addresses unrelated to the MASP Transaction do not require a signature when their balances decrease. I think this should be a very minor change that does not conflict with this PR.

Attempted to fix this here: https://github.com/anoma/namada/pull/3516

[grarco]

The latest additions would work. But I wonder if there isn't a way to achieve outsourcing without having to augment Transfer with a vector of VP addresses... Might it be possible to collect the VP Addresses from the Authorization sections of the Tx and insert those into the verifier list instead of embedding them in the Transfer structure? Or would this be too constraining/hacky? After all the user and implicit VPs look for the signatures in these sections...

I tend to think it would be slightly hacky and I'm not sure we can derive the address from an authorization section in case of a multisig account, or can we?

[murisi]

The latest additions would work. But I wonder if there isn't a way to achieve outsourcing without having to augment Transfer with a vector of VP addresses... Might it be possible to collect the VP Addresses from the Authorization sections of the Tx and insert those into the verifier list instead of embedding them in the Transfer structure? Or would this be too constraining/hacky? After all the user and implicit VPs look for the signatures in these sections...

I tend to think it would be slightly hacky and I'm not sure we can derive the address from an authorization section in case of a multisig account, or can we?

Fair enough agreed, probably attempting this is more trouble than it's worth.

On a separate note, I suspect that 65d07fb317fa9f2f4b871de68f9ab87ecf6712dd (i.e. with all the debited accounts as authorizers but without opt_authorizers additional authorizers field) combined #3516 (which reduces the set of authorizations required for a MASP Tx) would trigger the necessary VPs for all the transfers generated by namadac to pass. Though the difference with that approach is that it would probably be impossible to successfully execute the Alice-Bertha-Christel Tx above since Bertha's address would never be inserted by the tx as a verifier. But I'm doubting that transactions like this (where assets bounce through an intermediate account like Bertha) would actually be needed in practice, so it wouldn't be much of a loss. All this to say that I think this PR would also be perfectly fine without the Adds additional authorizers for masp transactions commit. It's up to you!

[grarco]

Hey @murisi, I've pushed some last updates:

• This branch is now based on #3516
• The Transfer data has been reverted to the previous one (no extra signers)
• The transfer transaction pushes actions for the debited accounts but only if these were involved in the masp part of the transfer
• The transfer transaction fails if the vins addresses of the masp transfer have not been debited
• The masp vp now checks that only the required masp actions have been pushed