Cache rejected tx to avoid rerequesting #4542
Conversation
src/main.cpp
Outdated
There was a problem hiding this comment.
Don't use an assignment. Just:
mruset<uint256> setRejectedTx(200);
|
My concern with remembering some rejections that currently may change is that it offers an easy avenue to blind a particular node to a double-spend: just send it the tx to be hidden at a time when it will reject-cache that tx, and that tx will effectively be invisible to the node until/unless it's included in a block. If this is not considered an important risk, I think we could also reject premature coinbase spends permanently, and remove the whole rejection IsPermanent mechanism (treating all rejections as permanent would simplify things a bit). |
|
I like the idea to not re-request transactions that are seen as misbehaviour. |
Previously AlreadyHave would only prevent rerequesting transactions accepted to the mempool or identified as orphans; if a transaction failed validation, it would keep requesting it and validating it every time it received an inv. This change adds a small mruset to remember some failed transactions. The main benefit of this change is that if a bug like bitcoin#3190 causes a peer to periodically send invs for bogus transactions, we don't repeatedly getdata and validate the same transactions until we DoS-ban the peer. This requires a new distinction between permanent failures (that are independent of chain state) and temporary failures; the few potentially temporary rejections are not cached.
|
If we do start persisting rejections that would otherwise not be permanent under certain circumstances, it seems like we'd ought to implement a way to still keep track of them for purposes of double-spend monitoring. I'm going to include just the minimal behavioral changes to fix the original issue in this PR, caching only rejections that definitely would have been rejected again if they were received again. Any additional changes using the rejection cache could be addressed separately. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4542_edde2dda82320e4660bc09cefd285310b8f1318e/ for binaries and test log. |
There was a problem hiding this comment.
What does the 200 do here? Limit it to remembering 200 TXs? Why 200? Would it make more sense to remember for a certain period rather than a fixed number?
There was a problem hiding this comment.
200 is arbitrary. I figured it would be large enough to handle the rate at which invalid txes would occur under "normal" circumstances like the wallet tx empty-vin bug. I don't think reasonable behavior is very dependent on the set size/expiration policy so long as it's in the right ballpark; it seems like returns diminish quickly beyond the cache being big enough to handle the bulk of unnecessary transaction-rerequesting. An expiry time would be slightly more complex to deal with than a maximum number of entries, and since I don't see the choice of policies making any important functional difference I just went with the simplest.
There was a problem hiding this comment.
Having a hard upper limit is a good idea for any data structure, this avoids DoS possibilities.
|
I dont think the network needs to concern itself with double spends. They've proven to be a PITA to try to relay and probably shouldnt be relayed as standard transactions, so I'd say ignore them for this pull. |
|
@kazcw needs rebase. |
There was a problem hiding this comment.
Nit: Should just pass a copy of a bool IMHO, I guess a pointer is bigger on some system archs even.
There was a problem hiding this comment.
That's an output parameter, passing a copy would defeat the purpose
There was a problem hiding this comment.
Then leave out the const, it could be a reference ;)?
There was a problem hiding this comment.
What const? And no, it can't be a reference, because it can be 0 if you're not interested in the output (see the function).
There was a problem hiding this comment.
I was at a totally different place in my brain while posting this. You are completely right.
|
Needs rebase (if we still want this) |
|
Closing this due to inactivity. Let me know if you start work on this again, then I'll reopen. |
7 participants
AlreadyHave only prevents rerequesting transactions accepted to the mempool or identified as orphans; if a transaction failed validation, it keeps requesting it and validating it every time it receives an inv. This change adds a small mruset to remember some failed transactions.
This requires a new distinction between permanent failures (that are independent of chain state) and temporary failures; the few potentially temporary rejections are not cached.
I wrote this because my node keeps banning old peers affected by #3190, when they're only resending the failed transactions because we keep getdataing them. A small cache of invalid transactions has low overhead in code complexity and runtime complexity, and prevents a lot of unnecessary banning for #3190 and any possible future bugs with similar effects.