Skip processing of TXs that we already have. #9414

pull rebroad wants to merge 1 commits into bitcoin:master from rebroad:DoNotProcessNewTxsMoreThanOnce changing 1 files +7 −4

rebroad commented at 4:54 PM on December 23, 2016: contributor

Previously TXs were processed more than they needed to be even though we already have them.

Skip processing of TXs that we already have.

Previously TXs were processed more than they needed to be even though we
already have them.

715b64abe2

in src/net_processing.cpp:None in 715b64abe2

1684 | @@ -1682,9 +1685,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
1685 |  
1686 |              BOOST_FOREACH(uint256 hash, vEraseQueue)
1687 |                  EraseOrphanTx(hash);
1688 | -        }
1689 | +        } // if accepted to memory pool
1690 |          else if (fMissingInputs)

rebroad commented at 4:55 PM on December 23, 2016:

for fMissingInputs to be true AlreadyHave() had to be false

in src/net_processing.cpp:None in 715b64abe2

1711 | @@ -1709,7 +1712,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
1712 |              } else {
1713 |                  LogPrint("mempool", "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString());
1714 |              }
1715 | -        } else {
1716 | +        } else { // not accepted and not missing inputs, i.e. invalid

rebroad commented at 4:56 PM on December 23, 2016:

whereas here AlreadyHave() could have been true still, so for whitelisted nodes, the same tx could be relayed multiple times

gmaxwell commented at 7:48 PM on December 23, 2016:

That is the intended and advertised purpose of the whitelisting, read the comment in the else block!

gmaxwell commented at 5:45 PM on December 23, 2016: contributor

&& is shortcutting so this doesn't change it to skip AcceptToMemoryPool. It does however change it so that it does not run the else clause which is needed for the forced relaying for whitelisted peers (which needs to run already for transactions we already have).

So I think the only real behavior change this makes is to break the functionality.
rebroad commented at 5:11 AM on December 24, 2016: contributor

@gmaxwell Ah, yes, my apologies I has missed the whitelist intention there. It was primarily noticing that (!tx.HasWitness() && !state.CorruptionPossible()) is bring run against txs we already have in the mempool that made me wonder if there was a logic oversight here, but perhaps the overhead from running this checks again is not significant.

I am not aware yet of why we would want to relay txs (possibly multiple times) to whitelisted nodes that we already have in our mempool though - is this requirement documented somewhere please?
rebroad closed this on Dec 24, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

rebroad

gmaxwell