commands: exclude immature coins from auto-selection

[jp1ac4]

As a follow-up to #873, this adds a comment to make clear that immature coins are not included as candidates for auto-selection excludes immature coins from auto-selection.

An unconfirmed coin could be both immature and marked as change, as the latter only depends on whether the address is derived from the wallet's change descriptor. I've also removed a corresponding assertion that may not always hold.

[jp1ac4]

I'll add a test that immature coins are not included for auto-selection.

[darosior]

Great catch. You missed the SQL constraint there is (basically "change XOR immature" too). I don't remember what i was thinking but there must be a reason i had both this assertion and the SQL constraint. It was introduced in #578. We should carefully make sure there is no piece of logic relying on this. This late in the release cycle i would suggest we make the poller actually respect the constraint (never try to create a coin which is both change and immature) instead of removing the constraint. We could then discuss in an issue whether it's safe to get rid of the constraint.

Here is a functional test which triggers the assertion on master and the SQL constraint failure on this branch:

diff --git a/tests/test_misc.py b/tests/test_misc.py
index f4a81c5..5c4b90a 100644
--- a/tests/test_misc.py
+++ b/tests/test_misc.py
@@ -239,6 +239,19 @@ def test_coinbase_deposit(lianad, bitcoind):
         and coin["spend_info"] is not None
     )
 
+    # We must also properly detect coinbase deposits to a change address. We used to have
+    # an assertion that a coin cannot both be change and a coinbase deposit. Since change
+    # is determined by the address... Technically we can.
+    change_desc = lianad.multi_desc.singlepath_descriptors()[1]
+    change_addr = bitcoind.rpc.deriveaddresses(str(change_desc), [0, 0])[0]
+    bitcoind.rpc.generatetoaddress(1, change_addr)
+    wait_for(lambda: any(c["is_immature"] for c in lianad.rpc.listcoins()["coins"]))
+    coin = next(c for c in lianad.rpc.listcoins()["coins"] if c["is_immature"])
+    bitcoind.generate_block(100)
+    wait_for_sync()
+    coin = next(c for c in lianad.rpc.listcoins()["coins"] if c["outpoint"] == coin["outpoint"])
+    assert not coin["is_immature"] and coin["block_height"] is not None
+
 
 @pytest.mark.skipif(
     OLD_LIANAD_PATH is None or USE_TAPROOT, reason="Need the old lianad binary to create the datadir."

[jp1ac4]

Thanks for spotting the SQL constraint that I missed and providing the functional test.

As discussed, I've now removed this constraint so that a coin can be both change and immature.

I've also added a DB migration script. I could perhaps perform a check during the migration that SELECT * FROM coins INTERSECT SELECT * FROM coins_new has the same number of rows as coins.

[darosior]

Looks good to me. Could you create an exhaustive unit test for the migration? Have coins in various states, migrate the DB and make sure everything stayed the same. You can take inspiration from the v0_to_v2_migration test. If you make this a v3 to v4 you shouldn't even have to carve out helpers since we didn't change the columns.

[jp1ac4]

I've now added a unit test for the v3 to v4 DB migration.

[darosior]

I think we can exercise more states when migrating from v3 to v4. Sorry to nit, i'm just always a bit wary to touch the database.

[jp1ac4]

I've updated the coins to use distinct values for each one (amount, vout, derivation index) and added a new coin_e.

I've also added some comments to clarify the approach taken and the final state of the coins.

[darosior]

My bad, i just had a quick skim through the code and overlooked you confirmed them using the connection.

[darosior]

ACK f9bae9c -- this is very nice. Again, great catch. And thanks for adding a more extensive unit test.