electrum: fix update of spend_txid for spending coins

[jp1ac4]

If the spend txid of a coin changes, we should update with the new txid.

This would affect a spending coin for which the spending transaction changes and the new spend transaction is confirmed in a single poll.

[jp1ac4]

I think if the spending coin is already in the DB, the spending_coins: Vec<OutPoint, Txid> passed to spent_coins would contain one entry with the old spend_txid and one with the new. So it's possible that we would still insert the correct spend_txid into the DB without this fix, depending on the order of iteration.

[darosior]

A regression test would demonstrate whether the fix is necessary. 🤷

[jp1ac4]

A regression test would demonstrate whether the fix is necessary. 🤷

Given that we iterate over the spending coins from the DB first and then the updated spending info, I think we will always use the correct value without this change as we'll first insert the wrong value and then the right value. I wasn't able to write a functional test to trigger this error.

EDIT: Related to this, we should probably modify how the two iterators are chained together so that we only include spending coins from the DB if they're not part of the other, up-to-date iterator. We shouldn't be passing the same outpoint more than once to spent_coins().

[jp1ac4]

I think we should still include this change in the future so that we don't rely on the order of iteration.

EDIT: As a test, I swapped the order of iteration and confirmed that my functional test fails without this PR and passes with it.

EDIT: Note that the replacement tx needs to be broadcast externally to the wallet in order to trigger this failure. Otherwise, the poller runs when broadcasting and the new spend_txid is detected while the coin is still spending.

[jp1ac4]

I've pushed the functional test that would fail without this PR if the order of iteration of spending coins were reversed.

[darosior]

It would still see it when it's included in the mempool.

[jp1ac4]

I should add to this comment that I stop and start lianad while the replacement is broadcast and confirmed to ensure that it isn't seen by Liana in the mempool.

[darosior]

EDIT: Related to this, we should probably modify how the two iterators are chained together so that we only include spending coins from the DB if they're not part of the other, up-to-date iterator. We shouldn't be passing the same outpoint more than once to spent_coins().

I agree, looks like this code is only accidentally correct.