test: Makes wait_for_getdata delete data on checks, plus allows to check the getdata message type
#29748
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
|
95472f4 to
256dc97
Compare
AngusP
left a comment
There was a problem hiding this comment.
ACK 256dc9745241b247262cc69beaf9bc57786b4d55 (caveat I'm pretty new to this code)
256dc97 to
df90ea6
Compare
AngusP
left a comment
There was a problem hiding this comment.
ACK df90ea6c73412736608ffc6074b73da980ad25a5
|
Concept ACK, |
Unfortunately, that's not the case given how the tests using The only way of working around this would be making PS: Or maybe rework this PR to be drop + cleanups |
wait_for_getdata delete data on checks, plus allows to check the getdata message type
Covered that in 067b009 by deleting the data on checks (pop instead of get) plus allowing to check the inv type. For the latter, I've done it in a way that all items need to be of the same type. We could address this so every single inv has its own type, but for that we would be required to provide a list of invs to |
AngusP
left a comment
There was a problem hiding this comment.
reACK 067b009bbf47f7bc5adeb6b500042f7c44bfb03f
There was a problem hiding this comment.
tACK 067b009
Make successful, functional tests successful
I believe this is a good refactor introducing check_last_getdata and modifying wait_for_getdata that will reduce some level of code duplication within tests.
Regarding this comment, I tried this refactor and the tests pass.
check_last_getdata uses the exact same logic as the internal function for wait_for_getdata. It feels like there should be a way of defining it as a lambda function, and getting rid of the internal function. I haven't been able to figure out how though.
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 684f029d58..a28b8860e4 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -430,6 +430,15 @@ class P2PConnection(asyncio.Protocol):
logger.debug(log_message)
+def assert_last_data(last_data, hash_list, datatype):
+ if not last_data:
+ return False
+ if datatype is None:
+ return [x.hash for x in last_data.inv] == hash_list
+ else:
+ return [(x.hash, x.type) for x in last_data.inv] == [(h, datatype) for h in hash_list]
+
+
class P2PInterface(P2PConnection):
"""A high-level P2P interface class for communicating with a Bitcoin node.
@@ -603,9 +612,7 @@ class P2PInterface(P2PConnection):
def check_last_getdata(self, hash_list):
with p2p_lock:
last_data = self.last_message.get("getdata")
- if last_data is None:
- return False
- return [x.hash for x in last_data.inv] == hash_list
+ return assert_last_data(last_data, hash_list, None)
def wait_for_tx(self, txid, *, timeout=60):
def test_function():
@@ -645,12 +652,7 @@ class P2PInterface(P2PConnection):
The object hashes in the inventory vector must match the provided hash_list."""
def test_function():
last_data = self.last_message.pop("getdata", None)
- if not last_data:
- return False
- if datatype is None:
- return [x.hash for x in last_data.inv] == hash_list
- else:
- return [(x.hash, x.type) for x in last_data.inv] == [(h, datatype) for h in hash_list]
+ return assert_last_data(last_data, hash_list, datatype)
Thanks! I think I may have been locking the |
4cbd066 to
bc844fd
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
bitcoin#18690 introduced a way of checking if a collection of hashes was present in the last received getdata message. However, bits of manual checking were left in some tests.
…age types if provided Currently, `wait_for_getdata` checks data in `last_message`, but it leaves is as is. This results in multiple tests having to manually pop data from the collection is multiple getdata messages need to be checked in order to make sure that we are not asserting the oldest message. This was partially solved by checking the hash of what was being waited for, but the issue remains if the same tx/block is sent multiple times during the test. In some of this scenarios, we were also manually checking the getdata message type manually which is not possible anymore if we pop data on checking, so `wait_for_getdata` is extended to support checking the type when we care about it
bc844fd to
e233ef1
Compare
|
Rebased to fix merge conflicts after #29736 |
|
tACK e233ef1 |
|
This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs. Closing due to lack of interest. |
#18690 introduced a way of checking if a collection of hashes was present in the last received getdata message. However, bits of manual checking were left in some tests.
This changes the behavior of
wait_for_getdataso data is deleted on checks, plus allows checking for thegetdatainv types so we don't have to do it manually (when needed)