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

runes: Use next id from runes table not runes_uniqueid from vars #6715

Merged

Conversation

ShahanaFarooqui
Copy link
Collaborator

@ShahanaFarooqui ShahanaFarooqui commented Sep 23, 2023

Changelog-Fixed: runes: use runes table id instead runes_uniqueid from vars because it returns incorrect unique id if rune/s migrated from datastore.

Issue (#6696):
The issue occurs when user saves the rune (created by commando-rune) in the datastore with v23.02 and then upgrades the DB with v23.08. This upgrade calls migrate_datastore_commando_runes function to migrate runes from datastore to runes table. But it does not increment runes_uniqueid of vars table. So when user creates a new rune, CLN gets old next_unique_id and fails with UNIQUE_ID constraint error.
This issue effects Umbrel's users and users who saved their runes from older version in the datastore.

Solution 1:

  • Add another migration function to recalculate runes_uniqueid and update vars table.
  • Update runes_uniqueid in vars table in migrate_datastore_commando_runes for users who are yet to update.

Solution 2:

  • Do not use runes_uniqueid, instead calculate the next_unique_id from runes table.
  • id can be auto generated for runes insert but we still need to maintain it in runes because:
    - It is also required by rune_derive_start
    - counter starts from 1 (not 0) if not explicitly passed to the query.

Implemented solution 2 as it is clean and future proof.

@ShahanaFarooqui ShahanaFarooqui added this to the v23.11 milestone Sep 23, 2023
@ShahanaFarooqui ShahanaFarooqui linked an issue Sep 23, 2023 that may be closed by this pull request
@rustyrussell
Copy link
Contributor

According to the changelog lines and lack of tests, this doesn't change anything? :)

The original purpose of the var was that we can have missing runes. Perhaps that's a premature optimization, though. If so, you should delete the now-unused variable.

Fixes: ElementsProject#6696

Changelog-Fixed: rune: use runes table `id` instead `runes_uniqueid` from `vars` because it returns incorrect unique id if rune/s migrated from datastore.
@ShahanaFarooqui
Copy link
Collaborator Author

According to the changelog lines and lack of tests, this doesn't change anything? :)

Updated changelog from None to Fixed. Also copied bug details from the original issue to describe the problem.

The original purpose of the var was that we can have missing runes. Perhaps that's a premature optimization, though. If so, you should delete the now-unused variable.

Deleted unused variable.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 63fc378

@rustyrussell rustyrussell merged commit 9561094 into ElementsProject:master Oct 2, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] createrune command crashes lightningd
2 participants