poller: unspend expired before new spend

[jp1ac4]

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.

[jp1ac4]

Refactored the new utils function.

[jp1ac4]

Rebased on master.

[darosior]

The rationale for this ordering is to make sure a coin marked as spending in this round which then gets marked as expired does get recorded correctly.

This is because we check the spend status of both the coins marked spending in DB but also the newly spending coins:

liana/src/bitcoin/poller/looper.rs

Lines 136 to 146 in 147cc0b

	// Mark coins in a spending state whose Spend transaction was confirmed as such. Note we
	// need to take into account the freshly marked as spending coins as well, as their spend
	// may have been confirmed within the previous tip and the current one, and we may not poll
	// this chunk of the chain anymore.
	let spending_coins: Vec<(bitcoin::OutPoint, bitcoin::Txid)> = db_conn
	.list_spending_coins()
	.values()
	.map(|coin| (coin.outpoint, coin.spend_txid.expect("Coin is spending")))
	.chain(spending.iter().cloned())
	.collect();
	let (spent, expired_spending) = bit.spent_coins(spending_coins.as_slice());

It seems this change could only cause issues in the unlikely scenario a coin gets detected as spending and "expires" right before we check its spend status in spent_coins and therefore we miss it did expire. But in this case it would just be marked as such at the next polling round, that's fine.

[darosior]

I think it's a bit fragile: it's possible for a caller to provide a condition function such as it returns True above and False here during the second call. It would be more robust to change the loop above to be:

while True:
    if success() or time >= start_time + timeout:
        break
    if condition():
        raise

[jp1ac4]

That's true. Perhaps we could also do:

while True:
    if time.time() >= start_time + timeout:
        raise
    if not condition():
        raise
    if success():
        return

[jp1ac4]

I've now updated this function.

[darosior]

Concept ACK. It does make a lot of sense, i think that's a bugfix and is harmless. I'd like to sleep on it though.

[darosior]

In a followup it would be worthwile to introduce this new wait_for_while_condition_holds helper for the rescan tests.

[jp1ac4]

Updated wait_for_while_condition_holds helper function.

[darosior]

ACK cc1de1d