refactor: Make all validation error actions explicit

[shrenujbansal]

Related Issues

Proposed Changes

• Make all validation error actions explicit in the switch check
• Move ErrNotEnoughFunds to return pubsub IGNORE since this can be possible from an innocent actor if a set of transactions gets executed out of order leaving no funds for one of them
• Add a comment when adding new errors to also include the error in the switch check

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[Stebalien]

I'm not sure if any of these should be "reject" as they can happen during forks/syncing.

[shrenujbansal]

To be clear, this is exactly the current behavior. Below are the errors we mark as reject:
ErrMessageTooBig
ErrMessageValueTooHigh
ErrNotEnoughFunds
ErrInvalidToAddr

Are you saying none of these should be marked as reject? @Stebalien

[Stebalien]

Yeah, I know. I'm saying that they only "too big" should likely be an error. But we should check this with @arajasek first.

[arajasek]

I think they're all fine to treat as REJECT, except ErrNotEnoughFunds.

ErrMessageValueTooHigh is trying to send more than 2B FIL, while ErrInvalidToAddr is just checking for non-nil recipient, both of which seem fine to REJECT.

Agreed that ErrNotEnoughFunds should be an IGNORE, though.

[Stebalien]

Ah, I see. Sg.

[Stebalien]

Whether or not there are enough available funds depends on the tipset against which the message is checked, other messages in the message pool, etc.

[shrenujbansal]

I see, so you could have funds available when the txn was created but subsequently created txns which also used up the funds could have been executed first leaving insufficient funds for the first one. Is that right?

[magik6k]

Pretty sure that we want ErrNotEnoughFunds to be reject, otherwise I can just spam the network with RBF messages from an address with barely any fil

[arajasek]

@magik6k But none of those messages would actually get added to anyone's mempools (assuming you don't have the FIL for it). You'll just spam and get ignored?

Sounds like you're maybe saying ErrRBFTooLowPremium should be a reject? Which I...might agree with...maybe?

[magik6k]

Hmm, I guess if we ignore, those messages won't get very far anyways, so it either doesn't really matter, or ignore is leaning on a slightly safer side when it comes to pubsub scoring.

[arajasek]

I'm not sure I agree with the idea of this PR. I do think we want to be rejecting by default, and only carving out ignores for very specific benign cases.

In general, its better practice to be more specific for erroneous cases, especially where an error results in a significant penalty

Unfortunately, a lowered peer score is the smaller penalty here. What you really don't want is to be not punishing peers that spam you invalid messages (eg. aren't validly signed, aren't actually messages but garbage data, etc.)

[shrenujbansal]

Unfortunately, a lowered peer score is the smaller penalty here. What you really don't want is to be not punishing peers that spam you invalid messages (eg. aren't validly signed, aren't actually messages but garbage data, etc.)

Could you elaborate on this? Why is a lowered peer score the smaller penalty when we've noticed especially in this recent case, that it has drastic implications for a peer not actually doing anything wrong?
If we decide to ignore a newly added error, it would mean minimal change to the peer score and not adding that msg to the mpool, so why would that be considered a bigger penalty?

Bringing the erroneous cases to be stated explicitly has benefits as can be witnessed in this very PR discussion. Previously, most people weren't even aware of whats being rejected
If however, there are security implications, I would say a good middle ground would be to define a static map where each error has the validation action (ignore/reject) stated explicitly
Thoughts? @arajasek

[arajasek]

Could you elaborate on this? Why is a lowered peer score the smaller penalty when we've noticed especially in this recent case, that it has drastic implications for a peer not actually doing anything wrong? If we decide to ignore a newly added error, it would mean minimal change to the peer score and not adding that msg to the mpool, so why would that be considered a bigger penalty?

Because otherwise I (an attacker) can send you an endless stream of bad messages, and have you spend all your time validating (invalid) signatures / reading state / generally wasting time. I think you need to be built to respond to bad actors, assume errors are malicious, and only carve out very specific exceptions.

Bringing the erroneous cases to be stated explicitly has benefits as can be witnessed in this very PR discussion. Previously, most people weren't even aware of whats being rejected If however, there are security implications, I would say a good middle ground would be to define a static map where each error has the validation action (ignore/reject) stated explicitly Thoughts? @arajasek

I'm not sure how much a static map achieves, because you'll still have some "default" action you take for errors that aren't defined in that map. I do think this winds up being a binary decision -- either REJECT all errors, with a carveout for specific typed IGNOREs, or IGNORE all errors, with a carveout for specific typed REJECTs.

[shrenujbansal]

Because otherwise I (an attacker) can send you an endless stream of bad messages, and have you spend all your time validating (invalid) signatures / reading state / generally wasting time. I think you need to be built to respond to bad actors, assume errors are malicious, and only carve out very specific exceptions

This makes sense. However, here #11016 (comment) you mentioned that for an ErrNotEnoughFunds error we could set it as IGNORE because even if someone spams you it doesn't add to the mpool
Couldn't an attacker send you an endless stream of msgs where the account doesn't have enough funds and you'd spend all your time validating bad msgs then?
Also, what's to preclude this from happening with the other types of errors we mark as IGNORE today?

[jennijuju]

No tests?

[shrenujbansal]

No tests?

There's no new logic here. Just a comment and making the errors more explicit

[arajasek]

LGTM