test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until #19134

  15 |  from test_framework.util import (
  16 |      assert_equal,
  17 |      disconnect_nodes,
  18 | -    connect_nodes,
  19 | -    wait_until,
  20 | +    connect_nodes

MarcoFalke commented at 4:19 PM on June 1, 2020:

Any reason to remove the trailing comma in this line? (Same in the other changes)

dboures commented at 5:15 PM on June 1, 2020:

No reason, I have since added back the trailing commas.

  67 | +        node0.wait_until(lambda: self.nodes[0].getblockcount() == 1000)
  68 |          stale_block_hash = self.nodes[0].getblockhash(1000)
  69 |  
  70 |          self.nodes[1].generate(1001)
  71 | -        wait_until(lambda: self.nodes[1].getblockcount() == 2000)
  72 | +        node1.wait_until(lambda: self.nodes[1].getblockcount() == 2000)

MarcoFalke commented at 4:23 PM on June 1, 2020:

node1 is a mininode. self.nodes[1] is a TestNode.

It might be best to add a def wait_until(self, ...): to the test framework.

        self.wait_until(lambda: self.nodes[1].getblockcount() == 2000)

  82 | -    def wait_for_block_announcement(self, block_hash, timeout=30):
  83 | +    def wait_for_block_announcement(self, block_hash):
  84 |          def received_hash():
  85 |              return (block_hash in self.announced_blockhashes)
  86 | -        wait_until(received_hash, timeout=timeout, lock=mininode_lock)
  87 | +        self.wait_until(received_hash,)

adamjonas commented at 2:30 PM on June 11, 2020:

Remove , if no other arguments being passed in.

  97 |          # Check that get*info() shows the versionbits unknown rules warning
  98 |          assert WARN_UNKNOWN_RULES_ACTIVE in node.getmininginfo()["warnings"]
  99 |          assert WARN_UNKNOWN_RULES_ACTIVE in node.getnetworkinfo()["warnings"]
 100 |          # Check that the alert file shows the versionbits unknown rules warning
 101 | -        wait_until(lambda: self.versionbits_in_alert_file(), timeout=60)
 102 | +        self.wait_until(lambda: self.versionbits_in_alert_file(), timeout=60)

adamjonas commented at 2:38 PM on June 11, 2020:

You've defined the default as 60 seconds, so you can drop here and other places where you specify 60.

  72 | @@ -73,23 +73,23 @@ def send_header_for_blocks(self, new_blocks):
  73 |      def request_headers_and_sync(self, locator, hashstop=0):
  74 |          self.clear_block_announcement()
  75 |          self.get_headers(locator, hashstop)
  76 | -        wait_until(self.received_block_announcement, timeout=30, lock=mininode_lock)
  77 | +        self.wait_until(self.received_block_announcement)

adamjonas commented at 2:39 PM on June 11, 2020:

Also re the default timeout, are you intending to change these non-60 timeouts throughout?

dboures commented at 7:57 PM on June 20, 2020:

No I was not. Thank you for the careful review, it's very helpful as I am still learning!