-
Notifications
You must be signed in to change notification settings - Fork 961
Downgrade tool #8702
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
base: master
Are you sure you want to change the base?
Downgrade tool #8702
Conversation
This will allow a downgrade tool to attempt to downgrade a node to the previous version. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
06f7a80 to
d4efc16
Compare
This will make it easier to share with the downgrade tool. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When installed, the name is `lightning-hsmtool`. We actually copy `tools/hsmtool` to `tools/lightning-hsmtool` but that's a silly step which we should get rid of. So: 1. Make sure our documentation always refers to it as lightning-hsmtool. 2. Make sure our tests invoke it as `lightning-hsmtool`. 3. Rename the C file. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And leave $(TOOLS) as user-visible tools. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
5f6d819 to
04e2c73
Compare
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: tools: `lightningd-downgrade` can downgrade your database from v25.12 to v25.09 if something goes wrong.
In particular, document when downgrades are not possible. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
04e2c73 to
b4be1b1
Compare
wallet/db.c
Outdated
| scid ? fmt_short_channel_id(tmpctx, *scid) : "no short channel id"); | ||
| } | ||
|
|
||
| /* For sqlite3 needs "2021-03-12 (3.35.0)" or above */ |
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.
Is there a way to enforce this requirement? Or is it an okay assumption that if they're running >25.9 version they have updated their sqlite version 🤔
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.
It will simply fail. Won't downgrade, will give an error message. It would be nice to fix, but that is a really old sqlite...
wallet/db.c
Outdated
| /* Simply drop table. */ | ||
| "DROP TABLE network_events"}, | ||
| {NULL, migrate_fail_pending_payments_without_htlcs, | ||
| /* Failing pending payments is idempotent, so no revert needed */ |
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.
What happens if you downgrade and then upgrade here?
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.
From wikipedia:
"Idempotence is the property of certain operations in mathematics and computer science whereby they can be applied multiple times without changing the result beyond the initial application."
i.e. if you run this more than once, the second time won't do anything (it will have converted them the first time).
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.
dang I've been wikipedia-ed 😭
wallet/db.c
Outdated
| struct migration { | ||
| const char *sql; | ||
| void (*func)(struct lightningd *ld, struct db *db); | ||
| const char *revertsql; |
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.
Not exactly sure where to add this (or if this comment is to premature I've only looked at this commit so far) but I'm guessing we also need a way to clean up the datastore between v25.09 and v25.12?
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.
Excellent point. This is true, if a plugin changed its datastore format, it would need to do this.
I looked through, and indeed, node bias support in v25.12 for askrene. If this is used, the plugin will error out on downgrade! Will add fix and test...
| void fillin_missing_lease_satoshi(struct lightningd *ld, | ||
| struct db *db); | ||
| void fillin_missing_lease_satoshi(struct lightningd *ld, |
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.
This was there in the deleted code in db.c as well but there's a duplication here.
| void migrate_remove_chain_moves_duplicates(struct lightningd *ld, struct db *db); | ||
| void migrate_from_account_db(struct lightningd *ld, struct db *db); | ||
| void migrate_remove_chain_moves_duplicates(struct lightningd *ld, struct db *db); |
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.
There's a duplication here as well.
|
|
||
| const struct db_migration *get_db_migrations(size_t *num); | ||
|
|
||
| /* All the functions provided by wallet.c */ |
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.
This comment might need to be changed to migrations.c
| # Any mention of BOLT# must be followed by an exact quote, modulo whitespace. | ||
| bolt-check/%: % bolt-precheck tools/check-bolt | ||
| @if [ -d .tmp.lightningrfc ]; then tools/check-bolt $(CHECK_BOLT_PREFIX) .tmp.lightningrfc $<; else echo "Not checking BOLTs: BOLTDIR $(BOLTDIR) does not exist" >&2; fi | ||
| bolt-check/%: % bolt-precheck devtools/check-bolt |
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.
This commit looks good, but why are we doing this?
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.
devtools are for tools run by developers only.
| tools/headerversions.o: ccan/config.h | ||
| tools/lightning-hsmtool: tools/lightning-hsmtool.o libcommon.a | ||
| tools/lightning-hsmtool: tools/lightning-hsmtool.o | ||
| tools/lightning-downgrade.o: CFLAGS:=$(CFLAGS) -DCLN_PREV_VERSION=$(CLN_PREV_VERSION) |
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.
Does this work okay without quotes?
| tools/lightning-downgrade.o \ | ||
| wallet/migrations.o \ | ||
| $(DB_OBJS) \ | ||
| $(WALLET_DB_QUERIES:.c=.o) \ | ||
| tools/lightning-downgrade.o |
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.
This is duplicated
| $(info Building version $(VERSION)) | ||
|
|
||
| # Next release. | ||
| CLN_NEXT_VERSION := v25.12 |
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.
Is this something manually updated or updated by one of the build scripts? I'm guessing it's okay for the previous release version to always stay as 25.09 because we will always try to make our releases downgradable from here on out?
| -------- | ||
|
|
||
| ```bash | ||
| lightning-downgrade method [ARGUMENTS]... |
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.
What is method? It doesn't seem like the main function accepts any arguments?
| static const struct db_version db_versions[] = { | ||
| { "v25.09", 276, false }, | ||
| /* When we implement v25.12 downgrade: { "v25.12", 280, ???}, */ | ||
| }; |
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.
Do we need to add a warning in the docs to indicate that the gossip store is deleted? Also, when we have a new release we will have to manually update the release version and the migration version, is that okay to do? Should we add a point in the release checklist to do so?
Fairly easy, for this release. The real problem was writing the tests!