Only use AddInventoryKnown for transactions #7960

1. 
   sdaftuar commented at 6:19 pm on April 27, 2016: member

   filterInventoryKnown is only used when relaying transactions, so stop adding block hashes to the filter.

   Also updates a comment that is no longer correct.

2. Only use AddInventoryKnown for transactions

   filterInventoryKnown is only used when relaying transactions,
   so stop adding block hashes to the filter.

   383fc10ebb
3. MarcoFalke added the label P2P on Apr 27, 2016
4. 
   theuni commented at 7:55 pm on April 27, 2016: member

   May as well skip in the fBlocksOnly case too?
5. 
   laanwj commented at 12:05 pm on April 28, 2016: member

   May as well skip in the fBlocksOnly case too?

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

6. 
   laanwj commented at 12:05 pm on April 28, 2016: member

   Concept ACK
7. 
   sdaftuar commented at 5:13 pm on April 28, 2016: member

   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.

8. 
   sipa commented at 5:02 pm on May 5, 2016: member

   utACK 383fc10ebb8e9ffe88c1b9e2ea706c76e5a6343e
9. 
   gmaxwell commented at 5:34 am on May 17, 2016: contributor

   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)

10. 
    sdaftuar commented at 4:59 pm on May 18, 2016: member

    Ok I’ll leave this alone then.
11. 
    sipa commented at 2:27 pm on May 25, 2016: member

    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.
12. 
    sipa commented at 5:32 pm on May 31, 2016: member

    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.
13. sipa merged this on Jun 1, 2016
14. sipa closed this on Jun 1, 2016

    ---

15. sipa referenced this in commit 6a22373771 on Jun 1, 2016
16. codablock referenced this in commit aaa254471e on Sep 16, 2017
17. codablock referenced this in commit 32461ab2e0 on Sep 19, 2017
18. codablock referenced this in commit bad08eadf8 on Dec 22, 2017
19. codablock referenced this in commit c7937c202e on Dec 27, 2017
20. andvgal referenced this in commit 5d8a6a44ae on Jan 6, 2019
21. andvgal referenced this in commit c49bbcc03a on Jan 6, 2019
22. zkbot referenced this in commit 19a8c45f42 on Aug 13, 2021
23. zkbot referenced this in commit 8e9f44cbe2 on Aug 13, 2021
24. zkbot referenced this in commit cf9a0799b4 on Aug 17, 2021
25. MarcoFalke locked this on Sep 8, 2021

Contributors
sdaftuar theuni laanwj sipa gmaxwell

Labels
P2P