Skip to content

db fail to upgrade due rune table migration #6770

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

Closed
vincenzopalazzo opened this issue Oct 13, 2023 · 4 comments · Fixed by #6781
Closed

db fail to upgrade due rune table migration #6770

vincenzopalazzo opened this issue Oct 13, 2023 · 4 comments · Fixed by #6781
Assignees
Labels
Milestone

Comments

@vincenzopalazzo
Copy link
Collaborator

I am running the current master but I can't upgrade the db

023-10-13T08:53:43.341Z DEBUG   hsmd: pid 173246, msgfd 53
2023-10-13T08:53:43.342Z DEBUG   hsmd: capability +WIRE_HSMD_CHECK_PUBKEY
2023-10-13T08:53:43.342Z DEBUG   hsmd: capability +WIRE_HSMD_SIGN_ANY_DELAYED_PAYMENT_TO_US
2023-10-13T08:53:43.342Z DEBUG   hsmd: capability +WIRE_HSMD_SIGN_ANCHORSPEND
2023-10-13T08:53:43.342Z DEBUG   hsmd: capability +WIRE_HSMD_SIGN_HTLC_TX_MINGLE
2023-10-13T08:53:43.342Z DEBUG   hsmd: capability +WIRE_HSMD_SIGN_SPLICE_TX
2023-10-13T08:53:43.342Z INFO    lightningd: Updating database from version 234 to 236
2023-10-13T08:53:43.342Z **BROKEN** lightningd: query failed: wallet/wallet.c:5737: SELECT rune, last_used_nsec FROM runes
@vincenzopalazzo
Copy link
Collaborator Author

Ok this looks like introduced in eacf0b5

cc @ShahanaFarooqui

@ShahanaFarooqui
Copy link
Collaborator

@vincenzopalazzo The issue has already been discussed in #6700.

It happened due to my rebase and merging confusion at db version 234. As suggested by @ddustin, the fix is: #6700 (comment)

Let me know if anything else needs to be done about this or can we close it now.

@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented Oct 15, 2023

@ShahanaFarooqui Sorry but I am running 7bea339. It should contain the solution, but the bug still happens, so I understand the problem, but I do not understand what the fix is?

In the PR that you link, there is the following assertion

And fixed that in further commits :)

It does not seem to be true(?)

If you think that it is a duplicate I believe you, so please feel free to close it if I am not able to upgrade my node

@ShahanaFarooqui
Copy link
Collaborator

@vincenzopalazzo Thanks. I was able to reproduce it with current master branch. I will look into it now.

Thank you for bringing it into my attention.

ShahanaFarooqui added a commit to ShahanaFarooqui/lightning that referenced this issue Oct 16, 2023
@ShahanaFarooqui ShahanaFarooqui removed duplicate in diagnostic issue under diagnostic labels Oct 16, 2023
ShahanaFarooqui added a commit to ShahanaFarooqui/lightning that referenced this issue Oct 16, 2023
…ElementsProject#6770

While rebasing the PR for per rune restriction, I unintentionally merged the
`{SQL("ALTER TABLE runes ADD last_used_nsec BIGINT DEFAULT NULL"), NULL}` database
alteration command ahead of {NULL, migrate_runes_idfix} (commit ElementsProject@eacf0b5#diff-1abcdf1b9d822b30079d6450b790274bdfb7c7fa04baa43ad2d9bd449865d4c9R978).

`migrate_runes_idfix` was the 234th change (deployed with version 23.08.1)
and adding the `last_used_nsec` column should have been the next
(235th, added in current release) change. Due to this incorrect ordering,
nodes updating from version 23.08.1 to the master branch will not add the
`last_used_nsec` column as they should, and instead execute `migrate_runes_idfix`
again, leading to the error in issue ElementsProject#6770.

After the reordering, db_get_runes method also has to be fixed for only
selecting rune NOT last_used_nsec. Because this column was added after
`migrate_runes_idfix` calls it. I am tempted to change the method name from
`db_get_runes` to `db_migrate_runes` for more clarity on its functionality though.

Changelog-None.
rustyrussell pushed a commit that referenced this issue Oct 23, 2023
…#6770

While rebasing the PR for per rune restriction, I unintentionally merged the
`{SQL("ALTER TABLE runes ADD last_used_nsec BIGINT DEFAULT NULL"), NULL}` database
alteration command ahead of {NULL, migrate_runes_idfix} (commit eacf0b5#diff-1abcdf1b9d822b30079d6450b790274bdfb7c7fa04baa43ad2d9bd449865d4c9R978).

`migrate_runes_idfix` was the 234th change (deployed with version 23.08.1)
and adding the `last_used_nsec` column should have been the next
(235th, added in current release) change. Due to this incorrect ordering,
nodes updating from version 23.08.1 to the master branch will not add the
`last_used_nsec` column as they should, and instead execute `migrate_runes_idfix`
again, leading to the error in issue #6770.

After the reordering, db_get_runes method also has to be fixed for only
selecting rune NOT last_used_nsec. Because this column was added after
`migrate_runes_idfix` calls it. I am tempted to change the method name from
`db_get_runes` to `db_migrate_runes` for more clarity on its functionality though.

Changelog-None.
whitslack added a commit to whitslack/lightning that referenced this issue Dec 10, 2023
whitslack added a commit to whitslack/lightning that referenced this issue Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants