Cache rejected tx to avoid rerequesting

kazcw commented at 0:52 am on July 16, 2014: contributor

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.

in src/main.cpp: in ba80f517bb outdated

66@@ -66,6 +67,8 @@ multimap<uint256, COrphanBlock*> mapOrphanBlocksByPrev;
67 map<uint256, CTransaction> mapOrphanTransactions;
68 map<uint256, set<uint256> > mapOrphanTransactionsByPrev;
69 
70+mruset<uint256> setRejectedTx = mruset<uint256>(200);

sipa commented at 8:01 pm on July 16, 2014:

Don’t use an assignment. Just:

0mruset<uint256> setRejectedTx(200);

in src/main.cpp: in afa26719bf outdated

1049@@ -1047,7 +1050,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
1050 
1051             if (RateLimitExceeded(dFreeCount, nLastFreeTime, nFreeLimit, nSize))
1052                 return state.DoS(0, error("AcceptToMemoryPool : free transaction rejected by rate limiter"),
1053-                                 REJECT_INSUFFICIENTFEE, "insufficient priority");
1054+                                 REJECT_INSUFFICIENTFEE, "insufficient priority", false);

sipa commented at 8:19 pm on July 16, 2014:

If we reject a transaction because of rate-limiting, I’d like that to be permanent. Otherwise you can keep retrying,

in src/main.cpp: in afa26719bf outdated

1005@@ -1003,7 +1006,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
1006         // are the actual inputs available?
1007         if (!view.HaveInputs(tx))
1008             return state.Invalid(error("AcceptToMemoryPool : inputs already spent"),
1009-                                 REJECT_DUPLICATE, "bad-txns-inputs-spent");
1010+                                 REJECT_DUPLICATE, "bad-txns-inputs-spent", false);

sipa commented at 8:20 pm on July 16, 2014:

If inputs are spent, they should remain spent, unless in the case of a reorganization, or transactions in a block that conflict with mempool transactions that conflict with us. Do we care?

kazcw commented at 8:48 pm on July 16, 2014: contributor

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).

laanwj commented at 7:16 am on July 17, 2014: member

I like the idea to not re-request transactions that are seen as misbehaviour.

Cache rejected tx to avoid rerequesting

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 #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.

edde2dda82

kazcw commented at 10:21 pm on July 20, 2014: contributor

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.

BitcoinPullTester commented at 10:40 pm on July 20, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4542_edde2dda82320e4660bc09cefd285310b8f1318e/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj added the label P2P on Jul 31, 2014

in src/main.cpp: in edde2dda82

67@@ -67,6 +68,8 @@ multimap<uint256, COrphanBlock*> mapOrphanBlocksByPrev;
68 map<uint256, CTransaction> mapOrphanTransactions;
69 map<uint256, set<uint256> > mapOrphanTransactionsByPrev;
70 
71+mruset<uint256> setRejectedTx(200);

rebroad commented at 8:22 am on August 29, 2014:

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?

kazcw commented at 9:39 am on August 29, 2014:

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.

laanwj commented at 8:58 am on September 25, 2014:

Having a hard upper limit is a good idea for any data structure, this avoids DoS possibilities.

TheBlueMatt commented at 8:11 pm on September 9, 2014: member

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.

rebroad commented at 0:31 am on September 29, 2014: contributor

@kazcw needs rebase.

in src/main.cpp: in edde2dda82

537@@ -535,9 +538,12 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
538 
539 
540 
541-bool IsStandardTx(const CTransaction& tx, string& reason)
542+bool IsStandardTx(const CTransaction& tx, string& reason, bool* pfPermanentRet)

Diapolo commented at 8:40 am on September 29, 2014:

Nit: Should just pass a copy of a bool IMHO, I guess a pointer is bigger on some system archs even.

laanwj commented at 11:49 am on September 29, 2014:

That’s an output parameter, passing a copy would defeat the purpose

Diapolo commented at 11:55 am on September 29, 2014:

Then leave out the const, it could be a reference ;)?

laanwj commented at 11:57 am on September 29, 2014:

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).

Diapolo commented at 12:02 pm on September 29, 2014:

I was at a totally different place in my brain while posting this. You are completely right.

laanwj commented at 4:50 pm on March 24, 2015: member

Needs rebase (if we still want this)

laanwj commented at 8:16 am on June 10, 2015: member

Closing this due to inactivity. Let me know if you start work on this again, then I’ll reopen.

laanwj closed this on Jun 10, 2015

MarcoFalke locked this on Sep 8, 2021

Cache rejected tx to avoid rerequesting #4542