Disconnect peers which we do not receive VERACKs from within 60 sec#9715
Disconnect peers which we do not receive VERACKs from within 60 sec#9715laanwj merged 4 commits intobitcoin:masterfrom
Conversation
TheBlueMatt
force-pushed
the
2017-02-disconnect-no-verack
branch
from
32d849b to
ead1c0c
Compare
|
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). |
|
utACK ead1c0cd17a8c696479abff58b91acc1373af179 |
|
utACK ead1c0cd17a8c696479abff58b91acc1373af179 |
qa/pull-tester/rpc-tests.py
Outdated
There was a problem hiding this comment.
micronit: Technically speaking, this test takes at least 62 seconds, so it should be moved up a bit.
|
ACK ead1c0c |
qa/rpc-tests/p2p-timeouts.py
Outdated
There was a problem hiding this comment.
Move comment above the class itself?
TheBlueMatt
force-pushed
the
2017-02-disconnect-no-verack
branch
from
ead1c0c to
520a033
Compare
|
utACK |
There was a problem hiding this comment.
Should use != instead of is not to avoid relying on string interning (also for two other string comparisons below).
There was a problem hiding this comment.
Fixed, I believe.
TheBlueMatt
force-pushed
the
2017-02-disconnect-no-verack
branch
from
520a033 to
cfe2743
Compare
|
utACK the network code, i haven't verified the test code. |
|
@jnewbery Can you please comment why you gave this a thumbs-down? |
|
@laanwj - ha. 👎 is for being mean about python. I definitely ❤️ tests and 👍 people writing more of them. Who knew emojis could be so ambiguous 😛 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) |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
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
| from test_framework.test_framework import BitcoinTestFramework | ||
| from test_framework.util import * | ||
| from time import sleep | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
OK. Done, I think?
| } | ||
|
|
||
| def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK): | ||
| def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK, send_version=True): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 jnewbery@9457e23, or equivalent.
There was a problem hiding this comment.
Oops, sorry, I missed that this commit was intended for this branch...I took the docs part of that commit (thanks!).
TheBlueMatt
force-pushed
the
2017-02-disconnect-no-verack
branch
from
cfe2743 to
fb140d2
Compare
TheBlueMatt
force-pushed
the
2017-02-disconnect-no-verack
branch
from
fb140d2 to
66f861a
Compare
|
ACK test changes in 66f861a |
…thin 60 sec 66f861a Add a test for P2P inactivity timeouts (Matt Corallo) b436f92 qa: Expose on-connection to mininode listeners (Matt Corallo) 8aaba7a qa: mininode learns when a socket connects, not its first action (Matt Corallo) 2cbd119 Disconnect peers which we do not receive VERACKs from within 60 sec (Matt Corallo)
…from within 60 sec 66f861a Add a test for P2P inactivity timeouts (Matt Corallo) b436f92 qa: Expose on-connection to mininode listeners (Matt Corallo) 8aaba7a qa: mininode learns when a socket connects, not its first action (Matt Corallo) 2cbd119 Disconnect peers which we do not receive VERACKs from within 60 sec (Matt Corallo)
…from within 60 sec 66f861a Add a test for P2P inactivity timeouts (Matt Corallo) b436f92 qa: Expose on-connection to mininode listeners (Matt Corallo) 8aaba7a qa: mininode learns when a socket connects, not its first action (Matt Corallo) 2cbd119 Disconnect peers which we do not receive VERACKs from within 60 sec (Matt Corallo)
12 participants
Also adds a test, which turned out to be harder than expected because python is braindead.