AnteHandler spam prevention

[jaekwon]

Problem Definition

AnteHandler currently potentially requires multiple account lookups and multiple signature verifications (especially with the upcoming threshold signature addition to tendermint/crypto)

This issue is a proposal for limiting the spam potential by enabling feature of "oversigning"

Proposal

A "oversigned" StdTx is one where the fee payer signs over all the remaining signatures for the remaining Msgs, thus allowing CheckTx to only have to check 1 account and 1 non-multi pubkey before rejecting the transaction. If there is any problem with the rest of the msgs/signatures, then the fee payer pays still pays the fee, so the DoS problem is mitigated.

The purpose of this change is to prevent CheckTx from having to load multiple accounts or check multiple signatures (e.g. in a composite multisig) before rejecting a transaction, to prevent spam issues.

Structures

StdTx := {Oversigned bool, Msgs*, Fee, *Signatures, Memo}
StdSignDoc := {AccountNum, ChainID, Fee, Memo, *Msgs, Sequence}
OversignedStdSignDoc := {AccountNum, ChainID, Fee, Memo, *Msgs, Sequence, *Signature}
FeePayMsg := {AccAddress}

• Changes:
  -> StdTx starts with new Oversigned bool field
  -> OversignedSignDoc embeds StdSignDoc but also includes *Signatures
  -> Add PubKey.IsComposite() in tendermint/crypto

• Goals
  -> Any non-oversigned StdTx can be oversigned by any third-party account.
  -> Be as restrictive as possible so that it can be upgraded in a backwards compatible way as we make AnteHandler more intelligent.

• We want to oversign, if:
  -> If len(StdTx.Signatures) > 1, or:
  -> If the signing account (only 1 due to above exception) is a composite pubkey

• If we want to oversign:
  -> Set Oversigned=True in StdTx, otherwise is False (not amino binary encoded if false so efficient for non-oversigned txs)
  -> An oversigned StdTx is invalid if there is only 1 Msg or 1 Signature.

• FeePayMsg:
  -> If the fee payer is not otherwise involved in the msgs, then the first msg should be a FeePayMsg which just denotes the account address of the fee payer.
  -> The FeePayMsg should not be present in any msg except the first.
  -> The FeePayMsg should be removed from the head of Msgs before creating OversignedStdSignDoc, thus allowing the fee payer to be determined after everyone else has already signed.
  -> In an oversigned StdTx, even if the fee payer is in with StdTx.Msgs[1:].Map(GetSigners), its signature is still required (e.g. it would have two different signatures in StdTx.Signatures... the first, and somewhere else). This is to prevent the fee payer (e.g. as a service) from accidentally committing to a transaction that it otherwise didn't intend to sign. (We can upgrade to optimize in the future)
  -> A StdTx with only 1 FeePayMsg is still valid, in this case the only effect is that a fee is paid for a memo.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[ValarDragon]

We don't really need this in consensus. We could have this option to have a separate fee payer designated always available. Then each validator has a config for their local CheckTx for which they reject all multisigs of that size, unless they have a single fee payer account.

This gives more granularity, since this is a matter of ensuring relay nodes / validators don't waste computation. Its likely harder to implement, so I think your proposal will be easier for prelaunch.

[alexanderbez]

Do we absolutely need to have the notion of "Oversigned" embedded in the actual tx type? Can we just have methods on the StdTx itself (e.g. StdTx#IsMultiSigned, StdTx#ValidMultiSigned, etc..)? Or does having this new metadata persisted as part of the actual tx necessary?

[jaekwon]

Do we absolutely need to have the notion of "Oversigned" embedded in the actual tx type? Can we just have methods on the StdTx itself (e.g. StdTx#IsMultiSigned, StdTx#ValidMultiSigned, etc..)? Or does having this new metadata persisted as part of the actual tx necessary?

Probably, whatever our rules for "needs oversigning" is, it can easily change. It's not much more expensive to have it there either...

We don't really need this in consensus. We could have this option to have a separate fee payer designated always available. Then each validator has a config for their local CheckTx for which they reject all multisigs of that size, unless they have a single fee payer account.

This gives more granularity, since this is a matter of ensuring relay nodes / validators don't waste computation. Its likely harder to implement, so I think your proposal will be easier for prelaunch.

The single fee payer needs to sign the transaction, and this specifies how with minimal code changes. I agree that it isn't absolutely necessary in consensus, but having it simplifies UX... you don't have to wonder whether to do one or the other depending on whether the next proposers will support it not etc. For the same reason it would be better to have a self-adjusting fee determined by consensus itself... though we might not launch with it due to time constraints.

[ValarDragon]

The single fee payer needs to sign the transaction, and this specifies how with minimal code changes. I agree that it isn't absolutely necessary in consensus, but having it simplifies UX... you don't have to wonder whether to do one or the other depending on whether the next proposers will support it not etc.

Totally agree with doing this because of minimal code changes. I'm not sure if I think this would be the ideal though, as this of idea doesn't really scale well to wanting ring signatures on chain. (Having a known fee-payer breaks anonymity) As well, this does cause unnecessary signatures on the honest multisignatures, causing an increase in their gas cost / fee.

For reference, I'm totally onboard with doing this prelaunch, I just don't think its the ideal long term solution.

For the same reason it would be better to have a self-adjusting fee determined by consensus itself... though we might not launch with it due to time constraints.

I am skeptical of that for different reasons, but I think we can discuss in a separate issue if we want to consider self-adjusting fees as the ideal long-term solution.

[cwgoes]

Can we just have a flat maximum number of signers? Seems like a much simpler & workable fix.

[ValarDragon]

I really like sunny's suggestion yesterday, just add an if conditional here to say that the first "feepayer"'s pubkey can't be a multisig. That is much simpler, as we support recursive multisigs. (Its more expressive than a weighted multisig :))

[cwgoes]

Let's set a hard max. How about 5 or 7 signatures? (cc @ebuchman)

[cwgoes]

ref #2779

[ValarDragon]

Wait why are we setting a hardmax. Just type check the fee payer's pubkey type, and ensure its not a multisig.

[cwgoes]

Wait why are we setting a hardmax. Just type check the fee payer's pubkey type, and ensure its not a multisig.

Because this needs to happen in combination with #2772 (comment), so the fee won't be paid.

[cwgoes]

Also see #2781.

[ValarDragon]

Because this needs to happen in combination with #2772 (comment), so the fee won't be paid.

These are entirely distinct issues. Checking the fee payer isn't a multisig can be done in conjunction with the fix for #2772. It is a single type check.

For gas handling, what needs to happen is a "numSigners" field on the multisignature, but only on that.

[cwgoes]

These are entirely distinct issues.

I don't think we can rely on the fee disincentivizing multisig-spam because the fee won't be paid in DeliverTx if the ante handler fails, and the proposer can still include transactions which fail CheckTx.

[ValarDragon]

The point is that if the fee payer's signature passes, the fee will by paid regardless, and that will be over the entire gas amount of the entire tx, multisig included. The multisig passing is irrelevant to to the fee being paid.

[cwgoes]

The point is that if the fee payer's signature passes, the fee will by paid regardless, and that will be over the entire gas amount of the entire tx, multisig included. The multisig passing is irrelevant to to the fee being paid.

My point is that the fee won't be paid, that's what we're changing with the ante handler in DeliverTx - see this PR - #2781.

[ValarDragon]

Thats wrong though. If the fee payer's signature is correct, the fee should be paid, independently of the correctness of the other signatures. That PR should be adapted then. It shouldn't be a hard fix, only deduct fees after fee payer signature, don't wait for any other signatures.

[alexanderbez]

@ValarDragon suggestions on how to support such flexibility?

[ValarDragon]

I edited the above post

It shouldn't be a hard fix, only deduct fees after fee payer signature, don't wait for any other signatures.

[alexanderbez]

Ok, I think we should still have the PR as-is (#2781), but in addition, also modify ante-handler as you're suggesting. Any opposition to this @cwgoes ?

[cwgoes]

Thats wrong though. If the fee payer's signature is correct, the fee should be paid, independently of the correctness of the other signatures. That PR should be adapted then. It shouldn't be a hard fix, only deduct fees after fee payer signature, don't wait for any other signatures.

Is this really worth giving up the invariant that a proposer cannot include transactions which would fail checktx to cause state changes? Limiting the size of multisigs seems like a simpler solution, and a limit of 5 or 7 is unlikely to problematize real-world usecases.

[ValarDragon]

I think this is independently necessary, and was the original intent of the fee payer.

This is part of why I was originally opposed to the multi tx approach though. Given that we have it, I think we should do this.

[cwgoes]

I'm not convinced this is worth it. What if, for example, I screw up a multisig tx (for which someone else already signed the fee for), send it to CheckTx (fails), then fix the bug and send a second - now a proposer can include both and charge fees for each. Capping multisig size is sufficient to prevent DoS.

[cwgoes]

At minimum, if we do this, we need to not only charge the fee but also increment the sequence number for the fee payer account. That prevents the above (I still don't think this is worth it compared to hard-capping multisig size) - but then the UX is worse, the fee payer has to now sign a second transaction.

[ValarDragon]

Only the fee payer could create the second tx, because the signatures would have been invalidated. (Since the signature is over all the data)

[cwgoes]

Do you agree that this will be a UX problem? Anyone with a partially-signed multisig tx could cause the tx to be submitted, deduct a fee and increment the fee payer's sequence, but ultimately fail.

[cwgoes]

Worse, it creates a frontrunning attack - the proposer could always do ^^.

[alexanderbez]

Ok, I think this warrants a further discussion with the team as there are pros/cons to both. I say for now we leave #2781 as-is (no ante-handler updates) and may enforce a multisig limit (for now) in an independent PR (which I think was the original intent)

[ValarDragon]

The main gossip network shouldn't support this sort of stuff, so the proposer shouldn't be able to do so.

But thats a good point. Perhaps we require the fee payer to sign over the other signatures.

[cwgoes]

The main gossip network shouldn't support this sort of stuff, so the proposer shouldn't be able to do so.

No gossip is required - the proposer who has a (fully signed, valid) multsig-tx can just alter one of the signatures, causing it to fail but still deduct the fee - they have no incentive to actually run the transaction (not just a griefing attack, it's cheaper for them and they still get the fee).

Having the fee payer sign over the other signatures is a possible solution, although it's quite a bit more complex and inefficient in the honest-party case.

[cwgoes]

Conclusion: limit total number of signatures per transaction to 7 per @ebuchman.