Simplify ProcessGetBlockData execution by removing send flag #13670

1. 
   fanquake commented at 2:29 pm on July 15, 2018: member

   Rebased/nits fixed version of #13250.

   Setting the send flag to false can be replaced by simply returning.

2. fanquake added the label Refactoring on Jul 15, 2018
3. fanquake added the label P2P on Jul 15, 2018
4. 
   practicalswift commented at 11:37 pm on July 15, 2018: contributor

   Concept ACK
5. in test/functional/p2p_invalid_getdata.py:10 in 4677dc25d4 outdated

    5+"""Request non-existent block.
    6+"""
    7+
    8+from test_framework.mininode import *
    9+from test_framework.test_framework import BitcoinTestFramework
   10+from test_framework.util import *
   

   ---

   ---

   

   promag commented at 9:18 pm on July 16, 2018:

   Remove, it’s unnecessary.
6. in test/functional/p2p_invalid_getdata.py:8 in 4677dc25d4 outdated

   0@@ -0,0 +1,32 @@
   1+#!/usr/bin/env python3
   2+# Copyright (c) 2018 The Bitcoin Core developers
   3+# Distributed under the MIT software license, see the accompanying
   4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5+"""Request non-existent block.
   6+"""
   7+
   8+from test_framework.mininode import *
   

   ---

   ---

   

   promag commented at 9:18 pm on July 16, 2018:

   Specify:

   0from test_framework.mininode import (CInv, P2PInterface, msg_getdata)
   

7. 
   promag commented at 9:21 pm on July 16, 2018: member

   Concept ACK, simplification is correct. Not sure about the new test.
8. 
   sipa commented at 1:27 am on July 17, 2018: member

   utACK 4677dc25d4fa3183e745d2b2f995809845148cc8. Didn’t review the test.
9. fanquake force-pushed on Jul 17, 2018
10. 
    laanwj commented at 2:53 pm on July 18, 2018: member

    I think it is somewhat subjective whether an early return is preferable to a status variable.

    ACK on the test in any case.

11. in test/functional/p2p_invalid_getdata.py:14 in 093ea052d4 outdated

     9+from test_framework.test_framework import BitcoinTestFramework
    10+
    11+# TestP2PConn: A peer we use to send messages to bitcoind, and store responses.
    12+class TestP2PConn(P2PInterface):
    13+    def __init__(self):
    14+        super().__init__()
    

    ---

    ---

    

    jamesob commented at 6:39 pm on July 18, 2018:

    Doesn’t seem like this subclass is necessary. Any reason you can’t just use P2PInterface on line 23 below?

    ---

    

    MarcoFalke commented at 7:39 pm on July 18, 2018:

    Agree. (Sorry, my previous comment didn’t apply, thus deleted)

    ---

    

    fanquake commented at 4:27 am on July 22, 2018:

    Thanks @jamesob. Test has been updated.
12. fanquake force-pushed on Jul 19, 2018
13. 
    DrahtBot commented at 5:12 pm on August 10, 2018: member
14. in test/functional/p2p_invalid_getdata.py:22 in 077df8e82c outdated

    17+        # Setup the p2p connections and start up the network thread.
    18+        self.test_node = self.nodes[0].add_p2p_connection(P2PInterface())
    19+
    20+        self.test_node.wait_for_verack()
    21+
    22+        self.test_node.send_message(msg_getdata(inv=[CInv(2, 0)]))
    

    ---

    ---

    

    sipa commented at 10:42 pm on August 10, 2018:

    What is this testing? I don’t see any assertions.

    ---

    

    MarcoFalke commented at 1:29 am on August 11, 2018:

    I think an earlier version of the code would crash when receiving such an inv. But yeah, somewhat agree that a new test file with all the overhead is a bit too much for testing so little
15. 
    sipa commented at 10:42 pm on August 10, 2018: member

    Concept ACK
16. net: Simplify ProcessGetBlockData execution by removing send flag.

    Setting the send flag to false can be replaced by simply returning.

    0cf546f71e
17. fanquake force-pushed on Aug 13, 2018
18. 
    fanquake commented at 9:13 am on August 13, 2018: member

    I’ve just dropped the test entirely here.
19. 
    jb55 commented at 4:55 am on September 6, 2018: member

    utACK 0cf546f71ebbee960fdca044e7abd100eec7b688 …

    … as long as these horrible single line if statements with multiple nested parens are cleaned up soon after

20. fanquake closed this on Oct 9, 2018

    ---

21. fanquake deleted the branch on Oct 9, 2018
22. 
    trongham commented at 2:35 pm on February 20, 2021: none

    Reviee
23. MarcoFalke referenced this in commit 3a12fdba51 on Mar 19, 2021
24. DrahtBot locked this on Aug 16, 2022

Contributors
fanquake practicalswift promag sipa laanwj jamesob MarcoFalke DrahtBot jb55 trongham

Labels
Refactoring P2P