go/runtime/txpool: Remove rechecked transactions from seen cache #5542
Conversation
In case a transaction is rejected because it fails a re-check pass, it should also be removed from the seen cache as it may be resubmitted later when it could become valid.
kostko
force-pushed
the
kostko/fix/rt-txpool-seencache-remove
branch
from
150e62c to
d2b2d6f
Compare
kostko
requested review from
peterjgilbert,
pro-wh,
ptrus and
peternose
as code owners
| t.seenCache.Remove(h) | ||
| } | ||
|
|
||
| t.HandleTxsUsed(hashes) |
There was a problem hiding this comment.
A bit confusing... Are rejected transactions "processed in a block" (description of HandleTxsUsed)?
There was a problem hiding this comment.
No, but the logic is the same (e.g. they should be removed from queue). Maybe the naming is a bit awkward.
There was a problem hiding this comment.
Looks good.
- Why is this better than submitting a new transaction?
- Do we simplify DoS attacks with these changes?
- Is it possible that rejected transactions will travel from node to node and never be removed from the network?
- Do we need to limit the number of retries?
| @@ -83,6 +83,11 @@ type TransactionPool interface { | |||
| // ClearProposedBatch clears the proposal queue. | |||
| ClearProposedBatch() | |||
|
|
|||
| // RejectTxs indicates that given transaction hashes have been rejected during block processing. | |||
| // Queues that can remove those transactions will do so. Additionally, these transactions will | |||
| // be removed from the already seen cache as they can potentially become valid in the future. | |||
There was a problem hiding this comment.
The final two sentences might be too detailed for an interface as they delve into implementation specifics. But so are some other comments.
Because the same transaction may become valid in the future. The future may also be due to the async nature of the network where some nodes may be slightly behind others.
Just in the sense that there may be more load on transaction checks (which are very fast). But someone can already generate slightly different transactions and spam those. Also note that it is already the case that transactions which are rejected immediately are not added to the already seen cache (for the same reason). This just makes rechecks consistent.
Nodes only forward transactions that pass checks. Also the p2p layer has its own deduplication logic.
You mean like having a separate cache that would count the number of times a tx hash was rejected and after a certain amount just immediately reject without passing any checks? We could consider this if it becomes a problem in practice. |
This makes sense.
If he takes this action now, the impact will be more significant, as the rejected transactions will accumulate in the network. However, according to your answer, these transactions will not be forwarded so I guess this is not a problem.
Yes.
Ok. |
In case a transaction is rejected because it fails a re-check pass, it should also be removed from the seen cache as it may be resubmitted later when it could become valid.