Disconnect peers which we do not receive VERACKs from within 60 sec #9715

TheBlueMatt commented at 10:45 PM on February 7, 2017: member

Also adds a test, which turned out to be harder than expected because python is braindead.

Disconnect peers which we do not receive VERACKs from within 60 sec 2cbd1196b7

TheBlueMatt force-pushed on Feb 7, 2017

TheBlueMatt commented at 10:59 PM on February 7, 2017: member

Note that previously it was unsupported behavior, but possible to use a connection normally without ever sending a VERACK, so this might actually have some effect on others, though we had already broken their ability to function normally with some recent net changes (post 0.13).

pstratem commented at 2:52 AM on February 8, 2017: contributor

utACK ead1c0cd17a8c696479abff58b91acc1373af179

fanquake added the label P2P on Feb 8, 2017

theuni commented at 7:08 AM on February 8, 2017: member

utACK ead1c0cd17a8c696479abff58b91acc1373af179

in qa/pull-tester/rpc-tests.py:None in ead1c0cd17 outdated

 171 | @@ -172,6 +172,7 @@
 172 |      'bip9-softforks.py',
 173 |      'p2p-feefilter.py',
 174 |      'rpcbind_test.py',
 175 | +    'p2p-timeouts.py',

paveljanik commented at 7:34 AM on February 8, 2017:

micronit: Technically speaking, this test takes at least 62 seconds, so it should be moved up a bit.

TheBlueMatt commented at 3:22 PM on February 8, 2017:

Fixed.

paveljanik commented at 7:35 AM on February 8, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/commit/ead1c0cd17a8c696479abff58b91acc1373af179

in qa/rpc-tests/p2p-timeouts.py:None in ead1c0cd17 outdated

   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import *
   9 | +from time import sleep
  10 | +
  11 | +'''
  12 | +TimeoutsTest -- test various net timeouts (only in extended tests)

paveljanik commented at 7:42 AM on February 8, 2017:

Move comment above the class itself?

TheBlueMatt commented at 3:22 PM on February 8, 2017:

Fixed.

MarcoFalke added this to the milestone 0.14.0 on Feb 8, 2017

TheBlueMatt force-pushed on Feb 8, 2017

gmaxwell commented at 4:42 PM on February 8, 2017: contributor

utACK

in qa/rpc-tests/test_framework/mininode.py:None in a5032b5b0c outdated

1653 | @@ -1652,8 +1654,9 @@ def show_debug_msg(self, msg):
1654 |          self.log.debug(msg)
1655 |  
1656 |      def handle_connect(self):
1657 | -        self.show_debug_msg("MiniNode: Connected & Listening: \n")
1658 | -        self.state = "connected"
1659 | +        if self.state is not "connected":

ryanofsky commented at 4:50 PM on February 8, 2017:

Should use != instead of is not to avoid relying on string interning (also for two other string comparisons below).

TheBlueMatt commented at 5:06 PM on February 8, 2017:

Fixed, I believe.

qa: mininode learns when a socket connects, not its first action 8aaba7a6b7

qa: Expose on-connection to mininode listeners b436f92f72

TheBlueMatt force-pushed on Feb 8, 2017

sipa commented at 9:58 PM on February 8, 2017: member

utACK the network code, i haven't verified the test code.

gmaxwell approved

gmaxwell commented at 1:33 AM on February 9, 2017: contributor

ACK (haven't reviewed/tested the test code)

laanwj commented at 6:31 AM on February 9, 2017: member

@jnewbery Can you please comment why you gave this a thumbs-down?

kallewoof commented at 8:57 AM on February 9, 2017: member

utACK

jnewbery commented at 3:38 PM on February 9, 2017: member

@laanwj - ha. :-1: is for being mean about python. I definitely :heart: tests and :+1: people writing more of them. Who knew emojis could be so ambiguous :stuck_out_tongue:

I intend to review @TheBlueMatt's test today.

(in all seriousness, we should do whatever we can to make our qa framework less braindead and make it easier to add good, solid tests quickly)

in qa/rpc-tests/p2p-timeouts.py:None in cfe274395e outdated

   5 | +
   6 | +from test_framework.mininode import *
   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import *
   9 | +from time import sleep
  10 | +

jnewbery commented at 6:02 PM on February 9, 2017:

I think we should try to use a consistent PEP8 convention for frontmatter:

shebang
copyright notice
module docstring
standard library imports
project imports

(see #9577). Can I convince you to do the same here?

TheBlueMatt commented at 10:13 PM on February 9, 2017:

OK. Done, I think?

in qa/rpc-tests/test_framework/mininode.py:None in cfe274395e outdated

1614 | @@ -1614,7 +1615,7 @@ class NodeConn(asyncore.dispatcher):
1615 |          "regtest": b"\xfa\xbf\xb5\xda",   # regtest
1616 |      }
1617 |  
1618 | -    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK):
1619 | +    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK, send_version=True):

jnewbery commented at 6:05 PM on February 9, 2017:

In my experience, test class and function interfaces often accumulate a lot of cruft as people add special-cases for individual test cases. It'd be nice if we could keep NodeConn's init() interface clean.

Would the following be a more elegant way to achieve this?

add a send_version() method to NodeConn and move the send_version code there
create a child class of NodeConn in your testcase that overrides the send_version() method

TheBlueMatt commented at 10:10 PM on February 9, 2017:

Hmm, I thought about this originlly, but hate overriding everything even more (then someone goes in and changes the function name and everything fails suddenly). Isnt this what python's named arguments is supposed to make clean?

jnewbery commented at 10:24 PM on February 9, 2017:

Well, if you change a function name and everything fails suddenly, that's a pretty good indicator that you should change it back :)

Meh - you have a preference for named arguments, I have a slight preference for overriding the function. I'm not going to fight you on it.

But I would really like you to include the module docstring and addition comments from https://github.com/jnewbery/bitcoin/commit/9457e23996bf5169dc9841eeca6f4ba010ae8654, or equivalent.

TheBlueMatt commented at 10:34 PM on February 9, 2017:

Oops, sorry, I missed that this commit was intended for this branch...I took the docs part of that commit (thanks!).

jnewbery commented at 6:07 PM on February 9, 2017: member

Test case looks good. I have a couple of style suggestions, which I've implemented here: https://github.com/jnewbery/bitcoin/commits/pr9715 . Feel free to squash that code into your commit if you agree

TheBlueMatt force-pushed on Feb 9, 2017

Add a test for P2P inactivity timeouts 66f861ade9

TheBlueMatt force-pushed on Feb 9, 2017

jnewbery commented at 11:04 PM on February 9, 2017: member

ACK test changes in 66f861ade9db108f979a9bdf8d90e8fca028e2a3

laanwj merged this on Feb 14, 2017

laanwj closed this on Feb 14, 2017

laanwj referenced this in commit b08656e343 on Feb 14, 2017

codablock referenced this in commit 44c0aabc07 on Jan 19, 2018

codablock referenced this in commit 588b8e5caf on Jan 23, 2018

andvgal referenced this in commit 523e778100 on Jan 6, 2019

CryptoCentric referenced this in commit 9d81ed38b1 on Feb 27, 2019

DrahtBot locked this on Sep 8, 2021