In D9469#214913, @deadalnix wrote:

I completely fail to see what this patch achieve. The added code seems to be at best redundant with existing code, at worse introduce bugs (if not today, then later as redundancy gets out of sync).

Currently we only store good proofs in the PeerManager, but the plan is also to store orphan proofs. We need a way to add a proof, have the PeerManager verify it and tell us if it was succesfully added. getPeerId(proof) will only tell us if the proof is good, not if we can store the proof as an orphan.

In D9469#214959, @deadalnix wrote:

Why couldn't fetchOrCreatePeer put the proof in the orphan set based on validation? Why is a new API needed? I strongly suspect that as you actually solve problems that you haven you'll discover that this is not needed.

It is true that the sorting of good vs orphan proofs can be done in fetchOrCreatePeers. But the problem that is currently not solved by the existing API is to get a feedback on why a Peer couldn't be created, to decide whether to put the proof in the recentRejects filter. If I call getPeerId and it returns NO_PEER, I don't know if the proof is permanently bad.
This is why I initially wanted the recentRejects to be handled by the PeerManager in D9423

An alternative to adding a new method is to forward the ProofValidationState all the way up the stack (e.g. PeerId PeerManager::getPeerId(const Proof &proof, ProofValidationState &state)). I did not go down that route because it has the drawback of making the Processor or the network layer have to inspect the reason for the rejection, I think it would be better for this to stay internal to the PeerManager.

Ok, so the problem that you want to solve is knowing if the proof is rejected or just orphaned. The solution can come in many forms. For instance:
1/ Separate validation that doesn't depend from the state (and is therefore def invalid) and validation that is state dependent, put the stateless check outside the peer manager and handle the result there. This doesn't require any changes here.
2/ Do the check here and add a way to export the result, which this patch really doesn't achieve.

Either way, you'll not figure out which is best by trying to guess here. you'll figure out by actually building what's using it. It starts in the network layer.

OK I will try to attack this from the other angle, starting from the network. I have a good idea of the general picture, but the tricky part for me it to split it into smaller units that make sense on their own.

In D9469#214984, @deadalnix wrote:

2/ Do the check here and add a way to export the result, which this patch really doesn't achieve.

This was the plan. The check is already done here. All we need is adding the orphanpool and plug is into addProofand getProof. After that, if addProof returns True, we know that the proof is not bad fore sure, because it has been added either as peers or as an orphan pool.

Solution 1 has the drawback to let many "definitely bad" proofs not be filtered, because most state dependent checks also tell us that the proof is def bad. Only HEIGHT_MISMATCH and MISSING_UTXO can be orphans.