Misc cleanups (extracted from autoclean rework) #5577
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's foolish to ban passing NULL, 0 to memcpy, memset et al, but it's been done. At high level of optimization, GCC assumes this doesn't happen, and yep, assumes "if (ctx)" inside tal_free() must be true. So when a psbt is NULL, and psbt_get_bytes returns NULL, a crash ensues: ``` lightningd: FATAL SIGNAL 6 (version v0.12.0rc2-6-g47efa5d-modded) 0x5557dfc42fef send_backtrace common/daemon.c:33 0x5557dfc42fef crashdump common/daemon.c:46 0x7fe93ef5851f ??? ./signal/../sysdeps/unix/sysv/linux/x86_64/libc_sigaction.c:0 0x7fe93efaca7c __pthread_kill_implementation ./nptl/pthread_kill.c:44 0x7fe93efaca7c __pthread_kill_internal ./nptl/pthread_kill.c:78 0x7fe93efaca7c __GI___pthread_kill ./nptl/pthread_kill.c:89 0x7fe93ef58475 __GI_raise ../sysdeps/posix/raise.c:26 0x7fe93ef3e7f2 __GI_abort ./stdlib/abort.c:79 0x5557dfbb0c28 call_error ccan/ccan/tal/tal.c:93 0x5557dfbb0c34 check_bounds ccan/ccan/tal/tal.c:165 0x5557dfbb0c34 to_tal_hdr ccan/ccan/tal/tal.c:178 0x5557dfc7a1d3 tal_free ccan/ccan/tal/tal.c:482 0x5557dfc609d3 tal_free ccan/ccan/tal/tal.c:477 0x5557dfc609d3 towire_wally_psbt bitcoin/psbt.c:743 0x5557dfbc5dfc towire_dualopend_got_offer_reply openingd/dualopend_wiregen.c:358 0x5557dfbc5dfc openchannel2_hook_cb lightningd/dual_open_control.c:671 0x5557dfc22f4f plugin_hook_callback lightningd/plugin_hook.c:210 0x5557dfc1dfbe plugin_response_handle lightningd/plugin.c:591 0x5557dfc1dfbe plugin_read_json_one lightningd/plugin.c:702 0x5557dfc1dfbe plugin_read_json lightningd/plugin.c:747 0x5557dfc71756 next_plan ccan/ccan/io/io.c:59 0x5557dfc775d5 io_ready ccan/ccan/io/io.c:417 0x5557dfc775d5 io_loop ccan/ccan/io/poll.c:453 0x5557dfbdb1ce io_loop ccan/ccan/io/poll.c:380 0x5557dfbdb1ce io_loop_with_timers lightningd/io_loop_with_timers.c:22 0x5557dfbb37d1 main lightningd/lightningd.c:1195 0x7fe93ef3fd8f __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 0x7fe93ef3fe3f __libc_start_main_impl ../csu/libc-start.c:392 0x5557dfbb6e84 ??? ???:0 0xffffffffffffffff ??? ???:0 ``` Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This broke with COPTFLAGS="-flto -O3", and so I took a look (it complains more than normal because main isn't there). We should never be running update-mocks except on programs expected to compile: in this case, that's tools/test/run-test-wire.c. Remove the code which tries to run this, which also means non-developers won't be running update-mocks! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…constraints. Get stricter with recognizing real column defs. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We can also remove the listpeers closer hack, which was removed from the schema already. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
You often don't care about the reply, so this is quite convenient. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have to scatter this everywhere in our schemas, as there's no way to make it the default :( Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
valgrind noticed that this was uninitialized when I tried a complex migration. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
misc-cleanups
branch
from
8e3f4ac
to
4f9eee4
Compare
rustyrussell
force-pushed
the
misc-cleanups
branch
3 times, most recently
from
c5a889d
to
64a017d
Compare
Because it used internal routines, it didn't pass operations through the db hook! So make it use the generic routines, with the twist that they are not translated. And when we use this in a migration hook, we're actually in a transaction. This, in turn, introduces an issue: we need to be outside a transaction to "PRAGMA foreign_keys = OFF", but completing the transaction when there is a db hook actually enters the io loop, freeing the tmpctx! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Be more graceful in shutting down: this should fix the issue where bookkeeper gets upset that its commands are rejected during shutdown, and generally make things more graceful. 1. Stop any new RPC connections. 2. Stop any per-peer daemons (channeld, etc). 3. Shut down plugins. 4. Stop all existing RPC connections. 5. Stop global daemons. 6. Free up peer, chanen HTLC datastructures. 7. Close database. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Changed: Plugins: RPC operations are now still available during shutdown.
rustyrussell
force-pushed
the
misc-cleanups
branch
from
64a017d
to
e7fe59b
Compare
cdecker
reviewed
Sep 12, 2022
| @@ -950,10 +950,10 @@ struct db *db_setup(const tal_t *ctx, struct lightningd *ld, | |||
| db->report_changes_fn = plugin_hook_db_sync; | |||
|
|
|||
| db_begin_transaction(db); | |||
| db->data_version = db_data_version_get(db); | |||
There was a problem hiding this comment.
Got confused about why this works. The DB version and the data version are orthogonal to each other :-)
cdecker
approved these changes
Sep 12, 2022
Comment on lines
+506
to
+517
| db_commit_transaction(db); | ||
|
|
||
| /* But core insists we're "in a transaction" for all ops, so fake it */ | ||
| db->in_transaction = "Not really"; | ||
| /* Turn off foreign keys first. */ | ||
| sqlite3_prepare_v2(wrapper->conn, "PRAGMA foreign_keys = OFF;", -1, &stmt, NULL); | ||
| if (sqlite3_step(stmt) != SQLITE_DONE) | ||
| goto sqlite_stmt_err; | ||
| sqlite3_finalize(stmt); | ||
| db_prepare_for_changes(db); | ||
| db_exec_prepared_v2(take(db_prepare_untranslated(db, | ||
| "PRAGMA foreign_keys = OFF;"))); | ||
| db_report_changes(db, NULL, 0); | ||
| db->in_transaction = NULL; | ||
|
|
||
| db_begin_transaction(db); |
There was a problem hiding this comment.
That's quite dodgy, as it can leave partial migrations in place if the query itself fails. But I don't have a better solution tbh. I wish sqlite3 had working ALTER TABLE support in an old-enough version so we could use it...
niftynei
reviewed
Sep 12, 2022
| @@ -933,11 +933,6 @@ parse_request(struct json_connection *jcon, const jsmntok_t tok[]) | |||
| json_tok_full(jcon->buffer, method)); | |||
| } | |||
|
|
|||
| if (jcon->ld->state == LD_STATE_SHUTDOWN) { | |||
| return command_fail(c, LIGHTNINGD_SHUTDOWN, | |||
| "lightningd is shutting down"); | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were enough things I found which weren't directly related to mean it was worth creating a generic "cleanup" PR.