-
Notifications
You must be signed in to change notification settings - Fork 67
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
commands: exclude immature coins from auto-selection #1001
Conversation
I'll add a test that immature coins are not included for auto-selection. |
83908ad
to
7958332
Compare
There was a problem hiding this 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."
This test is taken from darosior's comment in wizardsardine#1001 (review).
`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)
0966e43
to
f91e6da
Compare
`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)
f91e6da
to
b9fedcf
Compare
`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.
b9fedcf
to
ea5ad6e
Compare
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 |
There was a problem hiding this 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.
0ecdd62
to
c672212
Compare
I've now added a unit test for the v3 to v4 DB migration. |
There was a problem hiding this 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.
c672212
to
f9bae9c
Compare
I've updated the coins to use distinct values for each one (amount, vout, derivation index) and added a new I've also added some comments to clarify the approach taken and the final state of the coins. |
My bad, i just had a quick skim through the code and overlooked you confirmed them using the connection. |
ACK f9bae9c -- this is very nice. Again, great catch. And thanks for adding a more extensive unit test. |
As a follow-up to #873, this
adds a comment to make clear that immature coins are not included as candidates for auto-selectionexcludes 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.