Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

commands: exclude immature coins from auto-selection #1001

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Mar 11, 2024

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 jp1ac4 marked this pull request as draft March 11, 2024 20:16
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Mar 11, 2024

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

@jp1ac4 jp1ac4 force-pushed the commands-createspend-add-comment-immature-coins branch from 83908ad to 7958332 Compare March 19, 2024 11:58
@jp1ac4 jp1ac4 changed the title commands: add comment about immature coins commands: exclude immature coins from auto-selection Mar 19, 2024
@jp1ac4 jp1ac4 marked this pull request as ready for review March 19, 2024 12:07
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit to jp1ac4/liana that referenced this pull request Mar 20, 2024
jp1ac4 added a commit to jp1ac4/liana that referenced this pull request Mar 20, 2024
`is_change` is `true` for a coin if its address is derived
from our change descriptor and could in principle be used for a
coinbase transaction.

The functional test was provided by darosior in a PR comment:
wizardsardine#1001 (review)
@jp1ac4 jp1ac4 force-pushed the commands-createspend-add-comment-immature-coins branch 2 times, most recently from 0966e43 to f91e6da Compare March 20, 2024 17:48
jp1ac4 added a commit to jp1ac4/liana that referenced this pull request Mar 20, 2024
`is_change` is `true` for a coin if its address is derived
from our change descriptor and could in principle be used for a
coinbase transaction.

The functional test was provided by darosior in a PR comment:
wizardsardine#1001 (review)
@jp1ac4 jp1ac4 force-pushed the commands-createspend-add-comment-immature-coins branch from f91e6da to b9fedcf Compare March 20, 2024 18:22
jp1ac4 added 2 commits March 20, 2024 18:29
`is_change` is `true` for a coin if its address is derived
from our change descriptor and could in principle be used for a
coinbase transaction.

The functional test was provided by darosior in a PR comment:
wizardsardine#1001 (review)
An immature coin could in principle be marked as change
as this depends only on whether the address is derived from
the wallet's change descriptor.
@jp1ac4 jp1ac4 force-pushed the commands-createspend-add-comment-immature-coins branch from b9fedcf to ea5ad6e Compare March 20, 2024 18:29
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Mar 22, 2024

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.

Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jp1ac4 force-pushed the commands-createspend-add-comment-immature-coins branch 2 times, most recently from 0ecdd62 to c672212 Compare March 22, 2024 15:51
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Mar 22, 2024

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

Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jp1ac4 force-pushed the commands-createspend-add-comment-immature-coins branch from c672212 to f9bae9c Compare March 25, 2024 12:04
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Mar 25, 2024

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
Copy link
Member

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

@darosior
Copy link
Member

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

@darosior darosior merged commit 62efd33 into wizardsardine:master Mar 25, 2024
21 checks passed
@jp1ac4 jp1ac4 deleted the commands-createspend-add-comment-immature-coins branch March 25, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants