Simplify ProcessGetBlockData execution by removing send flag #13670

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:pstratem_rebased_processgetblockdata changing 1 files +11 −11

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.
fanquake added the label Refactoring on Jul 15, 2018
fanquake added the label P2P on Jul 15, 2018
practicalswift commented at 11:37 PM on July 15, 2018: contributor

Concept ACK

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.

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:

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

promag commented at 9:21 PM on July 16, 2018: member

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

utACK 4677dc25d4fa3183e745d2b2f995809845148cc8. Didn't review the test.
fanquake force-pushed on Jul 17, 2018
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.
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.
fanquake force-pushed on Jul 19, 2018
DrahtBot commented at 5:12 PM on August 10, 2018: member

No more conflicts as of last run.
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
sipa commented at 10:42 PM on August 10, 2018: member

Concept ACK
net: Simplify ProcessGetBlockData execution by removing send flag.
Setting the send flag to false can be replaced by simply returning.
0cf546f71e
fanquake force-pushed on Aug 13, 2018
fanquake commented at 9:13 AM on August 13, 2018: member

I've just dropped the test entirely here.
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
fanquake closed this on Oct 9, 2018
fanquake deleted the branch on Oct 9, 2018
trongham commented at 2:35 PM on February 20, 2021: none

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