Skip to content

Conversation

@rebroad
Copy link
Contributor

@rebroad rebroad commented Dec 23, 2016

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

Previously TXs were processed more than they needed to be even though we
already have them.
EraseOrphanTx(hash);
}
} // if accepted to memory pool
else if (fMissingInputs)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LogPrint("mempool", "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString());
}
} else {
} else { // not accepted and not missing inputs, i.e. invalid
Copy link
Contributor Author

@rebroad rebroad Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gmaxwell
Copy link
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
Copy link
Contributor Author

rebroad commented Dec 24, 2016

@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 rebroad closed this Dec 24, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants