Merge #965: poller: unspend expired before new spend

cc1de1d6d6710f1426a957806661e7f3461a7cb5 poller: unspend coins before spending new (jp1ac4)
1e7653e08a3778446ff677bb147df68b734a31fd tests: add function to wait while condition holds (jp1ac4)

Pull request description:

  This change ensures that the spend txid of a coin is updated directly to another value in case a conflicting spend is detected.

  The previous order caused the spend txid to first be cleared, which would misleadingly make the coin appear as confirmed
  rather than spending.

  I've added a new utils function for the functional tests that is a slight generalisation of `wait_for` with an additional condition that must always be met while waiting.

  `wait_for` now calls this new function with the condition being one that is always true.

ACKs for top commit:
  darosior:
    ACK cc1de1d6d6710f1426a957806661e7f3461a7cb5

Tree-SHA512: e3f00804a63b0e94bc1b2cbee03cac63dd6e2555ca6d301589b356b2baf8e0cf27362e1dda44018d1d8282e300b187079fcf61f5d2754263b9e8b08cd34be06e

src/bitcoin/poller/looper.rs

@@ -245,8 +245,8 @@ fn updates(
     db_conn.new_unspent_coins(&updated_coins.received);
     db_conn.remove_coins(&updated_coins.expired);
     db_conn.confirm_coins(&updated_coins.confirmed);
-    db_conn.spend_coins(&updated_coins.spending);
     db_conn.unspend_coins(&updated_coins.expired_spending);
+    db_conn.spend_coins(&updated_coins.spending);
     db_conn.confirm_spend(&updated_coins.spent);
     if latest_tip != current_tip {
         db_conn.update_tip(&latest_tip);

tests/test_chain.py

@@ -3,6 +3,7 @@ import copy
 from fixtures import *
 from test_framework.utils import (
     wait_for,
+    wait_for_while_condition_holds,
     get_txid,
     spend_coins,
     RpcError,
@@ -407,7 +408,13 @@ def test_conflicting_unconfirmed_spend_txs(lianad, bitcoind):
             return False
         return coin["spend_info"]["txid"] == txid.hex()
 
-    wait_for(lambda: is_spent_by(lianad, spent_coin["outpoint"], txid_b))
+    wait_for_while_condition_holds(
+        lambda: is_spent_by(lianad, spent_coin["outpoint"], txid_b),
+        lambda: lianad.rpc.listcoins([], [spent_coin["outpoint"]])["coins"][0][
+            "spend_info"
+        ]
+        is not None,  # The spend txid changes directly from txid_a to txid_b
+    )
 
 
 def test_spend_replacement(lianad, bitcoind):
@@ -454,11 +461,15 @@ def test_spend_replacement(lianad, bitcoind):
     # newly marked as spending, the second one's spend_txid should be updated and
     # the first one's spend txid should be dropped.
     second_txid = sign_and_broadcast_psbt(lianad, second_psbt)
-    wait_for(
+    wait_for_while_condition_holds(
         lambda: all(
             c["spend_info"] is not None and c["spend_info"]["txid"] == second_txid
             for c in lianad.rpc.listcoins([], second_outpoints)["coins"]
-        )
+        ),
+        lambda: lianad.rpc.listcoins([], [coins[1]["outpoint"]])["coins"][0][
+            "spend_info"
+        ]
+        is not None,  # The spend txid of coin from first spend is updated directly
     )
     wait_for(
         lambda: lianad.rpc.listcoins([], [first_outpoints[0]])["coins"][0]["spend_info"]
@@ -467,11 +478,15 @@ def test_spend_replacement(lianad, bitcoind):
 
     # Now RBF the second transaction with a send-to-self, just because.
     third_txid = sign_and_broadcast_psbt(lianad, third_psbt)
-    wait_for(
+    wait_for_while_condition_holds(
         lambda: all(
             c["spend_info"] is not None and c["spend_info"]["txid"] == third_txid
             for c in lianad.rpc.listcoins([], second_outpoints)["coins"]
-        )
+        ),
+        lambda: all(
+            c["spend_info"] is not None
+            for c in lianad.rpc.listcoins([], second_outpoints)["coins"]
+        ),  # The spend txid of all coins are updated directly
     )
     assert (
         lianad.rpc.listcoins([], [first_outpoints[0]])["coins"][0]["spend_info"] is None

tests/test_framework/utils.py

@@ -35,17 +35,33 @@ def wait_for(success, timeout=TIMEOUT, debug_fn=None):
     debug_fn is logged at each call to success, it can be useful for debugging
     when tests fail.
     """
+    wait_for_while_condition_holds(success, lambda: True, timeout, debug_fn)
+
+
+def wait_for_while_condition_holds(success, condition, timeout=TIMEOUT, debug_fn=None):
+    """
+    Run success() either until it returns True, or until the timeout is reached,
+    as long as condition() holds.
+    debug_fn is logged at each call to success, it can be useful for debugging
+    when tests fail.
+    """
     start_time = time.time()
     interval = 0.25
-    while not success() and time.time() < start_time + timeout:
+    while True:
+        if time.time() >= start_time + timeout:
+            raise ValueError("Error waiting for {}", success)
+        if not condition():
+            raise ValueError(
+                "Condition {} not met while waiting for {}", condition, success
+            )
+        if success():
+            return
         if debug_fn is not None:
             logging.info(debug_fn())
         time.sleep(interval)
         interval *= 2
         if interval > 5:
             interval = 5
-    if time.time() > start_time + timeout:
-        raise ValueError("Error waiting for {}", success)
 
 
 def get_txid(hex_tx):