lightingnd: wrap all JSON reply callbacks in db transactions.

It was always a bit weird they weren't, and it seems a premature
optimization to make the callbacks to this themselves.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This commit is contained in:
Rusty Russell 2023-10-24 12:11:28 +10:30
parent 524eff0e93
commit 8170aba75f
5 changed files with 40 additions and 39 deletions

View file

@ -404,10 +404,7 @@ static void sendrawtx_callback(const char *buf, const jsmntok_t *toks,
err);
}
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, success, errmsg, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
tal_free(call);
}
@ -472,9 +469,7 @@ getrawblockbyheight_callback(const char *buf, const jsmntok_t *toks,
* with NULL values. */
err = json_scan(tmpctx, buf, toks, "{result:{blockhash:null}}");
if (!err) {
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, NULL, NULL, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
goto clean;
}
@ -493,9 +488,7 @@ getrawblockbyheight_callback(const char *buf, const jsmntok_t *toks,
"getrawblockbyheight",
"bad block");
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, &blkid, blk, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
clean:
tal_free(call);
@ -574,10 +567,8 @@ static void getchaininfo_callback(const char *buf, const jsmntok_t *toks,
bitcoin_plugin_error(call->bitcoind, buf, toks, "getchaininfo",
"bad 'result' field: %s", err);
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, chain, headers, blocks, ibd,
call->first_call, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
tal_free(call);
}
@ -640,9 +631,7 @@ static void getutxout_callback(const char *buf, const jsmntok_t *toks,
err = json_scan(tmpctx, buf, toks, "{result:{script:null}}");
if (!err) {
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, NULL, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
goto clean;
}
@ -654,9 +643,7 @@ static void getutxout_callback(const char *buf, const jsmntok_t *toks,
bitcoin_plugin_error(call->bitcoind, buf, toks, "getutxout",
"bad 'result' field: %s", err);
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, &txout, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
clean:
tal_free(call);

View file

@ -921,7 +921,6 @@ static void listincoming_done(const char *buffer,
const jsmntok_t *idtok UNUSED,
struct invoice_info *info)
{
struct lightningd *ld = info->cmd->ld;
struct command_result *ret;
bool warning_mpp, warning_capacity, warning_deadends, warning_offline, warning_private_unused;
@ -934,8 +933,6 @@ static void listincoming_done(const char *buffer,
if (ret)
return;
/* We're actually outside a db transaction here: spooky! */
db_begin_transaction(ld->wallet->db);
invoice_complete(info,
false,
warning_mpp,
@ -943,7 +940,6 @@ static void listincoming_done(const char *buffer,
warning_deadends,
warning_offline,
warning_private_unused);
db_commit_transaction(ld->wallet->db);
}
/* Since this is a dev-only option, we will crash if dev-routes is not

View file

@ -17,6 +17,7 @@
#include <common/timeout.h>
#include <common/version.h>
#include <connectd/connectd_wiregen.h>
#include <db/exec.h>
#include <dirent.h>
#include <errno.h>
#include <lightningd/io_loop_with_timers.h>
@ -84,6 +85,7 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book,
p->blacklist = tal_arr(p, const char *, 0);
p->plugin_idx = 0;
p->dev_builtin_plugins_unimportant = false;
p->want_db_transaction = true;
return p;
}
@ -619,6 +621,7 @@ static const char *plugin_response_handle(struct plugin *plugin,
/* We expect the request->cb to copy if needed */
pd = plugin_detect_destruction(plugin);
request->response_cb(plugin->buffer, toks, idtok, request->response_cb_arg);
/* Note that in the case of 'plugin stop' this can free request (since
@ -639,12 +642,14 @@ static const char *plugin_response_handle(struct plugin *plugin,
* If @destroyed was set, it means the plugin called plugin stop on itself.
*/
static const char *plugin_read_json_one(struct plugin *plugin,
bool want_transaction,
bool *complete,
bool *destroyed)
{
const jsmntok_t *jrtok, *idtok;
struct plugin_destroyed *pd;
const char *err;
struct wallet *wallet = plugin->plugins->ld->wallet;
*destroyed = false;
/* Note that in the case of 'plugin stop' this can free request (since
@ -687,6 +692,11 @@ static const char *plugin_read_json_one(struct plugin *plugin,
"JSON-RPC message does not contain \"jsonrpc\" field");
}
/* We can be called extremely early, or as db hook, or for
* fake "terminated" request. */
if (want_transaction)
db_begin_transaction(wallet->db);
pd = plugin_detect_destruction(plugin);
if (!idtok) {
/* A Notification is a Request object without an "id"
@ -732,6 +742,8 @@ static const char *plugin_read_json_one(struct plugin *plugin,
*/
err = plugin_response_handle(plugin, plugin->toks, idtok);
}
if (want_transaction)
db_commit_transaction(wallet->db);
/* Corner case: rpc_command hook can destroy plugin for 'plugin
* stop'! */
@ -753,6 +765,9 @@ static struct io_plan *plugin_read_json(struct io_conn *conn,
{
bool success;
bool have_full;
/* wallet is NULL in really early code */
bool want_transaction = (plugin->plugins->want_db_transaction
&& plugin->plugins->ld->wallet != NULL);
log_io(plugin->log, LOG_IO_IN, NULL, "",
plugin->buffer + plugin->used, plugin->len_read);
@ -774,8 +789,9 @@ static struct io_plan *plugin_read_json(struct io_conn *conn,
do {
bool destroyed;
const char *err;
err =
plugin_read_json_one(plugin, &success, &destroyed);
err = plugin_read_json_one(plugin, want_transaction,
&success, &destroyed);
/* If it's destroyed, conn is already freed! */
if (destroyed)

View file

@ -108,6 +108,10 @@ struct plugins {
struct list_head plugins;
bool startup;
/* Normally we want to wrap callbacks in a db transaction, but
* not for the db hook servicing */
bool want_db_transaction;
struct logger *log;
struct lightningd *ld;

View file

@ -134,7 +134,10 @@ static void plugin_hook_killed(struct plugin_hook_call_link *link)
/* Call next will unlink, so we don't need to. This is treated
* equivalent to the plugin returning a continue-result.
*/
struct db *db = link->plugin->plugins->ld->wallet->db;
db_begin_transaction(db);
plugin_hook_callback(NULL, NULL, NULL, link->req);
db_commit_transaction(db);
} else {
/* The plugin is in the list waiting to be called, just remove
* it from the list. */
@ -159,9 +162,7 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
struct plugin_hook_request *r)
{
const jsmntok_t *resulttok;
struct db *db = r->db;
struct plugin_hook_call_link *last, *it;
bool in_transaction = false;
/* Pop the head off the call chain and continue with the next */
last = list_pop(&r->call_chain, struct plugin_hook_call_link, list);
@ -187,31 +188,22 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
r->hook->name, toks->end - toks->start,
buffer + toks->start);
db_begin_transaction(db);
if (!r->hook->deserialize_cb(r->cb_arg, buffer,
resulttok)) {
tal_free(r->cb_arg);
db_commit_transaction(db);
goto cleanup;
}
in_transaction = true;
} else {
/* plugin died */
resulttok = NULL;
}
if (!list_empty(&r->call_chain)) {
if (in_transaction)
db_commit_transaction(db);
plugin_hook_call_next(r);
return;
}
/* We optimize for the case where we already called deserialize_cb */
if (!in_transaction)
db_begin_transaction(db);
r->hook->final_cb(r->cb_arg);
db_commit_transaction(db);
cleanup:
/* We need to remove the destructors from the remaining
@ -349,7 +341,8 @@ void plugin_hook_db_sync(struct db *db)
struct jsonrpc_request *req;
struct plugin_hook_request *ph_req;
void *ret;
struct plugin **plugins;
struct plugin **plugin_arr;
struct plugins *plugins;
size_t i;
size_t num_hooks;
@ -358,11 +351,12 @@ void plugin_hook_db_sync(struct db *db)
if (num_hooks == 0)
return;
plugins = notleak(tal_arr(NULL, struct plugin *,
plugin_arr = notleak(tal_arr(NULL, struct plugin *,
num_hooks));
for (i = 0; i < num_hooks; ++i)
plugins[i] = hook->hooks[i]->plugin;
plugin_arr[i] = hook->hooks[i]->plugin;
plugins = plugin_arr[0]->plugins;
ph_req = notleak(tal(hook->hooks, struct plugin_hook_request));
ph_req->hook = hook;
ph_req->db = db;
@ -372,7 +366,7 @@ void plugin_hook_db_sync(struct db *db)
/* Create an object for this plugin. */
struct db_write_hook_req *dwh_req;
dwh_req = tal(ph_req, struct db_write_hook_req);
dwh_req->plugin = plugins[i];
dwh_req->plugin = plugin_arr[i];
dwh_req->ph_req = ph_req;
dwh_req->num_hooks = &num_hooks;
@ -393,21 +387,25 @@ void plugin_hook_db_sync(struct db *db)
json_array_end(req->stream);
jsonrpc_request_end(req);
plugin_request_send(plugins[i], req);
plugin_request_send(plugin_arr[i], req);
}
/* We don't want to try to open another transaction: we're in one! */
plugins->want_db_transaction = false;
ret = plugins_exclusive_loop(plugin_arr);
plugins->want_db_transaction = true;
/* We can be called on way out of an io_loop, which is already breaking.
* That will make this immediately return; save the break value and call
* again, then hand it onwards. */
ret = plugins_exclusive_loop(plugins);
if (ret != ph_req) {
void *ret2 = plugins_exclusive_loop(plugins);
void *ret2 = plugins_exclusive_loop(plugin_arr);
assert(ret2 == ph_req);
log_debug(plugins[0]->plugins->ld->log, "io_break: %s", __func__);
log_debug(plugins->ld->log, "io_break: %s", __func__);
io_break(ret);
}
assert(num_hooks == 0);
tal_free(plugins);
tal_free(plugin_arr);
tal_free(ph_req);
}