test: Makes `wait_for_getdata` delete data on checks, plus allows to check the getdata message type #29748

sr-gi commented at 11:38 AM on March 27, 2024: member

#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_getdata so data is deleted on checks, plus allows checking for the getdata inv types so we don't have to do it manually (when needed)

DrahtBot commented at 11:39 AM on March 27, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	AngusP
Concept ACK	naiyoma
Stale ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Mar 27, 2024

sr-gi commented at 11:40 AM on March 27, 2024: member

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.

sr-gi force-pushed on Mar 27, 2024

AngusP approved

AngusP commented at 10:27 PM on March 29, 2024: contributor

ACK 256dc9745241b247262cc69beaf9bc57786b4d55 (caveat I'm pretty new to this code)

DrahtBot added the label Needs rebase on Apr 2, 2024

sr-gi force-pushed on Apr 4, 2024

DrahtBot removed the label Needs rebase on Apr 4, 2024

AngusP approved

AngusP commented at 10:44 AM on April 7, 2024: contributor

ACK df90ea6c73412736608ffc6074b73da980ad25a5

naiyoma commented at 7:35 PM on April 7, 2024: contributor

Concept ACK, nit: Could you also take a look at announce_tx_and_wait_for_getdata in p2p_segwit.py for a similar cleanup?

sr-gi commented at 11:59 AM on April 8, 2024: member

Concept ACK, nit: Could you also take a look at announce_tx_and_wait_for_getdata in p2p_segwit.py for a similar cleanup?

Unfortunately, that's not the case given how the tests using announce_tx_and_wait_for_getdata are built. That is, the same transaction may be sent multiple times using announce_tx_and_wait_for_getdata, so you need to potentially pop the old one to make sure the assert is not using old data.

The only way of working around this would be making wait_for_getdata pop the data instead of just reading it (similarly to how #29736 is approached) but that may need its own PR.

PS: Or maybe rework this PR to be drop + cleanups

sr-gi renamed this:
~~test: Cleans some manual checks/drops when using wait_for_getdata~~
test: Makes `wait_for_getdata` delete data on checks, plus allows to check the getdata message type
on Apr 8, 2024

sr-gi commented at 2:32 PM on April 8, 2024: member

Concept ACK, nit: Could you also take a look at announce_tx_and_wait_for_getdata in p2p_segwit.py for a similar cleanup?

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 wait_for_getdata, instead of a list of hashes. If this is something we want to have, it would certainly need its own PR given it'd require a way bigger change.

AngusP approved

AngusP commented at 3:57 PM on April 8, 2024: contributor

reACK 067b009bbf47f7bc5adeb6b500042f7c44bfb03f

rkrux approved

rkrux commented at 9:02 AM on April 10, 2024: contributor

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)

sr-gi commented at 1:53 PM on April 10, 2024: member

Regarding this comment, I tried this refactor and the tests pass.

Thanks! I think I may have been locking the p2p_lock twice which made test timeout. I amended the last comment with a simplification based on your diff

sr-gi force-pushed on Apr 10, 2024

DrahtBot added the label CI failed on Apr 10, 2024

DrahtBot commented at 2:03 PM on April 10, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/23663215852</sub>

in test/functional/test_framework/p2p.py:604 in bc844fd8a7 outdated

 599 | @@ -600,6 +600,19 @@ def test_function():
 600 |  
 601 |      # Message receiving helper methods
 602 |  
 603 | +    def _check_last_getdata(self, hash_list, datatype=None):
 604 | +        last_data = self.last_message.pop("getdata", None)

rkrux commented at 2:18 PM on April 10, 2024:

Note: This will pop from last message via check_last_getdata() call as well, which is different from previous commits. That's why I accepted last_data as an argument in my suggestion. Is popping in check_last_getdata also fine?

sr-gi commented at 2:22 PM on April 10, 2024:

Yeah, I noticed that on your suggestion, but I don't think we do reuse the value in any test (same as with wait_for_*)

DrahtBot removed the label CI failed on Apr 10, 2024

test: Cleans some manual checks/drops when using wait_for_getdata

https://github.com/bitcoin/bitcoin/pull/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.

7a82739cb7

test: makes wait_for_getdata pops data on checks, plus check the message 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

e233ef1836

sr-gi force-pushed on Apr 25, 2024

sr-gi commented at 6:32 PM on April 25, 2024: member

Rebased to fix merge conflicts after #29736

AngusP commented at 6:59 PM on April 29, 2024: contributor

tACK e233ef18364a5758adcb83eea53bdf92aa1bf551

DrahtBot requested review from rkrux on Apr 29, 2024

achow101 commented at 2:47 PM on October 15, 2024: member

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.

achow101 closed this on Oct 15, 2024

bitcoin locked this on Oct 15, 2025