net: Remove forcerelay of rejected txs #17985

1. 
   MarcoFalke commented at 9:04 PM on January 22, 2020: member

   This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:

   • While RelayTransaction enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See https://github.com/bitcoin/bitcoin/blob/4a072330763b3ff2d1b5c5b8d30a9517873ac6de/src/net_processing.cpp#L3862-L3866

   • Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (mapRelay)

   The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574

   This feature was (intentionally or accidentally) removed in 4d8993b3469915d8c9ba4cd3b918f16782edf0de, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.

2. MarcoFalke added the label P2P on Jan 22, 2020
3. 
   DrahtBot commented at 12:40 AM on January 23, 2020: member

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

4. fanquake requested review from sdaftuar on Jan 23, 2020
5. 
   luke-jr commented at 2:53 AM on January 29, 2020: member

   While RelayTransaction enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool.

   Does that mean #17303 is redundant?

6. 
   MarcoFalke commented at 6:32 PM on January 30, 2020: member

   @luke-jr No, #17303 is different and not redundant. That other one is about transactions that have been in the mempool, but a are no longer. For example they could have been mined.

7. 
   MarcoFalke commented at 6:33 PM on January 30, 2020: member

   A test for forcerelay has been merged here 3b69310beb172b2b56fbfc38c45898a1b627ac54. The test should still pass after the changes here.

8. 
   laanwj commented at 4:52 PM on February 10, 2020: member

   code review ACK fab08e0caa029aaaa683ad5a17b23f2dc0d66c56

9. 
   ajtowns commented at 9:00 AM on February 11, 2020: member

   ACK fab08e0caa029aaaa683ad5a17b23f2dc0d66c56

   Might be good to keep a LogPrintf("Not relaying tx %s as not present in mempool") error though?

10. MarcoFalke force-pushed on Feb 11, 2020
11. 
    MarcoFalke commented at 12:27 PM on February 11, 2020: member

    Thanks for the review. Added the logprint back. Also, I've rebased this to be able to extend the existing test case

12. net: Remove forcerelay of rejected txs facb71576c
13. MarcoFalke force-pushed on Feb 11, 2020
14. 
    MarcoFalke commented at 3:46 PM on February 11, 2020: member

    Force pushed to fixup the documentation

15. 
    hebasto commented at 5:27 PM on February 17, 2020: member

    Concept ACK

16. hebasto approved
17. 
    hebasto commented at 7:45 PM on February 17, 2020: member

    ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.

18. laanwj merged this on Feb 26, 2020
19. laanwj closed this on Feb 26, 2020

    ---

20. MarcoFalke deleted the branch on Feb 26, 2020
21. sidhujag referenced this in commit 7bd71c6d3c on Feb 26, 2020
22. sidhujag referenced this in commit c9a8767cb9 on Nov 10, 2020
23. Fabcien referenced this in commit 1ee3e4bda4 on Aug 30, 2021
24. DrahtBot locked this on Feb 15, 2022

Review Requested
sdaftuar