May as well skip in the fBlocksOnly case too?

May as well skip in the fBlocksOnly case too?

Agree, seems to be no reason to add it to AddInventoryKnown in that case.

Concept ACK

Yeah, I think that makes sense. I thought at first that maybe we'd want filterInventoryKnown to be as correct as possible even in the fBlocksOnly case, since we might still relay transactions to that peer (eg if we have a whitelisted peer and -whitelistrelay is on), but given that it's a protocol violation this seems like a silly thing to worry about.

I was thinking we could probably use a p2p regression test that exercises filterInventoryKnown, as I don't believe any of our tests would fail if it were broken?

I'll plan to update this PR at some point with a test and the change for the fBlocksOnly case.

utACK 383fc10

utACK.

I think blocks only will later be generalized to more degrees than just literally blocks only yes or no, and we probably shouldn't go around adding too much specialization in the codebase for a global blocks only mode unless it's a pretty big win. (and maybe saving the memory of ever allocating some transaction related filters in the first place might be... but insertions? not so much)

Ok I'll leave this alone then.

Given that the scope of AddInventoryKnown changes, perhaps it should be renamed to AddInventoryTxKnown, and only take a uint256 rather than a CInv? That may reduce confusion in upcoming refactors.

Lightly tested ACK. Setup: two mainnet full nodes with this patch (A publicly reachable, B -connect'ed to A) and a lightweight node C (connected to the public network). Tested block synchronization/relay of B from A, relay of transactions to from A to B, relay of newly created transactions by B and C through A. Nothing unusual.