test: Check hash in wait_for

maflcko commented at 11:45 pm on April 12, 2020: member

Like it was done in 59d6249290, the wait_for_get* helpers should be extended to take in hashes and check them.

https://github.com/bitcoin/bitcoin/blob/59d62492900f6fc6ade2a780e75863ba3b254e06/test/functional/test_framework/mininode.py#L415

Useful skills:

Basic understanding of python and how to run the Bitcoin Core functional test suite

Want to work on this issue?

The purpose of the good first issue label is to highlight which issues are suitable for a new contributor without a deep understanding of the codebase.

You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

maflcko added the label good first issue on Apr 12, 2020

fanquake added the label Tests on Apr 12, 2020

Nishikoh commented at 0:08 am on April 13, 2020: none

Could I tackle this issue?

hebasto commented at 0:12 am on April 13, 2020: member

@Nishikoh

Could I tackle this issue?

Great!

You do not need to request permission to start working on this.

robot-visions commented at 7:04 pm on April 17, 2020: contributor

Hi @Nishikoh , thanks for offering to tackle this!

Just checking, are you still interested in working on it? If so, then I’ll leave it to you :) but if not, I have some progress that I can upload.

Nishikoh commented at 7:29 pm on April 17, 2020: none

Hi @robot-visions Sorry, this work is late. Would you upload it?
Thanks.

robot-visions commented at 8:19 pm on April 17, 2020: contributor

Thanks @Nishikoh ! I uploaded some progress but it doesn’t fully resolve the issue, so please feel free to continue working on this if you’re still interested.

My first PR addresses the wait_for_getdata part. However, there are still additional changes needed to fully resolve this issue:

Update wait_for_getheaders to check the locator hash(es)
Remove calls to pop("getdata") and pop("getheaders") that are no longer needed

I need to think more about the best way to identify the correct locator hash when calling wait_for_getheaders. My initial attempt was to use the getbestblockhash RPC, but it seemed to cause too much of a slowdown. I’d also welcome any discussion / suggestions about how best to approach this.

maflcko referenced this in commit 64139803f1 on Apr 23, 2020

maflcko referenced this in commit a215c61333 on Apr 24, 2020

sidhujag referenced this in commit 83fbffb5ee on Apr 24, 2020

Nishikoh commented at 12:06 pm on April 28, 2020: none

I’m working on this issue, but I don’t have a good idea and confidence. Could you advise me on how to solve this issue?

Nishikoh commented at 12:06 pm on April 28, 2020: none

https://github.com/bitcoin/bitcoin/blob/9fb95ae8e3e4f10888a98fc99d704d97e2eb371f/test/functional/test_framework/mininode.py#L431

I’m working to modify test_function on wait_for_getheaders. I’m thinking that I will modify it like the next code.

 0    def wait_for_getheaders(self, blockhash, timeout=60):
 1        """Waits for a getheaders message.
 2
 3        Receiving any getheaders message will satisfy the predicate. the last_message["getheaders"]
 4        value must be explicitly cleared before calling this method, or this will return
 5        immediately with success. TODO: change this method to take a hash value and only
 6        return true if the correct block header has been requested."""
 7
 8        def test_function():
 9            assert self.is_connected
10            last_headers = self.last_message.get("getheaders")
11            if not last_headers:
12                return False
13            return last_headers.hashstop == blockhash
14
15        wait_until(test_function, timeout=timeout, lock=mininode_lock)

Is this idea correct?

robot-visions commented at 4:02 pm on April 28, 2020: contributor

Thanks for working on this @Nishikoh ! I think it’s a good start. To continue I would recommend:

Look at the protocol documentation for getheaders to see what the various fields (including hashstop) are for
Search for places where wait_for_getheaders is called and see what additional data you might pass in

Nishikoh commented at 9:41 am on April 29, 2020: none

Thank you for your advice @robot-visions ! I will take your advice as a reference.

Nishikoh commented at 10:21 pm on May 29, 2020: none

I posted PR. What should I do anything else?

vijaydasmp referenced this in commit c0f5e9334f on Dec 28, 2022

vijaydasmp referenced this in commit 75e1aba6f8 on Dec 30, 2022

PastaPastaPasta referenced this in commit f3687a269e on Jan 12, 2023

vijaydasmp referenced this in commit 1f32d0d6ef on Feb 18, 2023

vijaydasmp referenced this in commit 5260f27f7f on Feb 20, 2023

vijaydasmp referenced this in commit 6ba701eae6 on Feb 20, 2023

vijaydasmp referenced this in commit 6999057c35 on Feb 22, 2023

vijaydasmp referenced this in commit 5968f82886 on Feb 22, 2023

vijaydasmp referenced this in commit c27ca887f3 on Feb 22, 2023

vijaydasmp referenced this in commit 05e59d69e9 on Feb 23, 2023

vijaydasmp referenced this in commit 67eab752a6 on Feb 24, 2023

vijaydasmp referenced this in commit ba02ca49b1 on Feb 24, 2023

vijaydasmp referenced this in commit ec823356e7 on Feb 24, 2023

vijaydasmp referenced this in commit 9b3e3b387b on Feb 28, 2023

vijaydasmp referenced this in commit 11801edddf on Mar 3, 2023

PastaPastaPasta referenced this in commit 93882f3ec1 on Mar 27, 2023

achow101 closed this on Apr 25, 2024

achow101 referenced this in commit 3c88eac28e on Apr 25, 2024

test: Check hash in wait_for_get* helpers #18614

Useful skills:

Want to work on this issue?