Message Pool Rudimentary Spam Protection Measures #3313
Conversation
gas limit is checked against block gas limit in ValidForBlockInclusion
to make it clear to readers that this failure is soft; we might be out of sync.
vyzo
changed the title
|
Follow up: we also want to rate limit messages per actor to some reasonable number. |
vyzo
force-pushed
the
feat/mpool-balance-tracking
branch
from
2b858dc to
038e83b
Compare
vyzo
changed the title
There was a problem hiding this comment.
This is a very solid step in the right direction, the general idea makes sense to me but still I'd need someone more familiarized with mpool internals to review this. There may be many corner cases we may not be anticipating with this "balance simulator" (illustrative but probably not useful example: what if I transfer funds to the actor in the same round that it wants to submit the message?).
On the implementation side (but again with very little insight here), I'm not totally sold on the way this new requiredFunds state is maintained across calls, there seem to be many callers that now need to be aware of it and maintain it (which sounds a bit error prone to me).
|
thanks for the review @schomatis!
Well, the state is maintained in the |
|
One potential follow up, probably for another pr because I want to think about it, is to turn the Ignores into Rejections if a peer is sending us too much stuff that gets Ignored. |
| if m.Message.GasLimit > build.BlockGasLimit { | ||
| return xerrors.Errorf("given message has too high of a gas limit") | ||
| } | ||
|
|
There was a problem hiding this comment.
this is checked by ValidForBlockInclusion, so it was redundant here and I removed it.
| @@ -319,7 +348,7 @@ func (mp *MessagePool) checkMessage(m *types.SignedMessage) error { | |||
| return xerrors.Errorf("mpool message too large (%dB): %w", m.Size(), ErrMessageTooBig) | |||
| } | |||
|
|
|||
| // Perform syntaxtic validation, minGas=0 as we check if correctly in select messages | |||
| // Perform syntactic validation, minGas=0 as we check the actual mingas before we add it | |||
| if err := m.Message.ValidForBlockInclusion(0); err != nil { | |||
There was a problem hiding this comment.
were do we do that check now?
There was a problem hiding this comment.
we do it in both add and push.
There was a problem hiding this comment.
and we check with the actual gas in the add logic, so it happens in all paths.
| func (ms *msgSet) rm(nonce uint64) { | ||
| m, has := ms.msgs[nonce] | ||
| if has { | ||
| ms.requiredFunds.Sub(ms.requiredFunds, m.Message.RequiredFunds().Int) |
There was a problem hiding this comment.
Oh wait, we're tracking funds required for this message chain here, so this means we're removing the message from the chain, which lowers the required funds for that chain. SGTM
essential for correctness in the revert case
|
I fixed a correctness issue by adding a flag in |
This adds tracking of required funds for pending messages so that we can stop spam attacks.
This also allows us to finally enable some score contribution from the messages topic.