From 723c16072a4438589f3bd2008c56c2f36ea06aec Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 15 Dec 2020 10:13:42 +1030 Subject: [PATCH] cleanups: feedback from Christian Decker review. 1. Hoist 7200 constant into the bolt12 heade2. 2. Make preimage the last createinvoice arg, so we could make it optional. 3. Check the validity of the preimage in createinvoice. 4. Always output used flag in listoffers. 5. Rename wallet offer iterators to offer_id iterators. 6. Fix paramter typos. 7. Rename `local_offer_id` parameter to `localofferid`. 8. Add reference constraints on local_offer_id db fields. 9. Remove cut/paste comment. 10. Clarify source of fatal() messages in wallet. Signed-off-by: Rusty Russell --- common/bolt12.h | 10 ++++++++++ devtools/bolt12-cli.c | 5 +++-- doc/lightning-createinvoice.7 | 12 ++++++------ doc/lightning-createinvoice.7.md | 7 +++---- lightningd/invoice.c | 15 +++++++++++++-- lightningd/offer.c | 7 +++---- lightningd/pay.c | 8 ++++---- plugins/libplugin-pay.c | 2 +- plugins/pay.c | 4 ++-- tests/test_pay.py | 2 +- tests/test_plugin.py | 2 +- wallet/db.c | 4 ++-- wallet/db_postgres_sqlgen.c | 10 +++++----- wallet/db_sqlite3_sqlgen.c | 10 +++++----- wallet/statements_gettextgen.po | 16 ++++++++-------- wallet/wallet.c | 17 +++++++++-------- wallet/wallet.h | 16 ++++++++-------- 17 files changed, 84 insertions(+), 63 deletions(-) diff --git a/common/bolt12.h b/common/bolt12.h index 47dd95125..5176f76b0 100644 --- a/common/bolt12.h +++ b/common/bolt12.h @@ -5,6 +5,16 @@ struct feature_set; +/* BOLT-offers #12: + * - if `relative_expiry` is present: + * - MUST reject the invoice if the current time since 1970-01-01 UTC + * is greater than `timestamp` plus `seconds_from_timestamp`. + * - otherwise: + * - MUST reject the invoice if the current time since 1970-01-01 UTC + * is greater than `timestamp` plus 7200. + */ +#define BOLT12_DEFAULT_REL_EXPIRY 7200 + /** * offer_encode - encode this complete bolt12 offer TLV into text. */ diff --git a/devtools/bolt12-cli.c b/devtools/bolt12-cli.c index b6ab3efdf..a006943e6 100644 --- a/devtools/bolt12-cli.c +++ b/devtools/bolt12-cli.c @@ -405,8 +405,9 @@ static void print_relative_expiry(u64 *timestamp, u32 *relative) * is greater than `timestamp` plus 7200. */ if (!relative) - printf("relative_expiry: %u (%s) (default)\n", 7200, - fmt_time(tmpctx, *timestamp + 7200)); + printf("relative_expiry: %u (%s) (default)\n", + BOLT12_DEFAULT_REL_EXPIRY, + fmt_time(tmpctx, *timestamp + BOLT12_DEFAULT_REL_EXPIRY)); else printf("relative_expiry: %u (%s)\n", *relative, fmt_time(tmpctx, *timestamp + *relative)); diff --git a/doc/lightning-createinvoice.7 b/doc/lightning-createinvoice.7 index 4b53482ad..5a209df84 100644 --- a/doc/lightning-createinvoice.7 +++ b/doc/lightning-createinvoice.7 @@ -3,7 +3,7 @@ lightning-createinvoice - Low-level invoice creation .SH SYNOPSIS -\fBcreateinvoice\fR \fIinvstring\fR \fIpreimage\fR \fIlabel\fR +\fBcreateinvoice\fR \fIinvstring\fR \fIlabel\fR \fIpreimage\fR .SH DESCRIPTION @@ -15,15 +15,15 @@ The \fIinvstring\fR parameter is of bolt11 form, but without the final signature appended\. Minimal sanity checks are done\. -The \fIpreimage\fR is the preimage to supply upon successful payment of -the invoice\. - - The \fIlabel\fR must be a unique string or number (which is treated as a string, so "01" is different from "1"); it is never revealed to other nodes on the lightning network, but it can be used to query the status of this invoice\. + +The \fIpreimage\fR is the preimage to supply upon successful payment of +the invoice\. + .SH RETURN VALUE On success, an invoice object is returned, as per \fBlistinvoices\fR(7)\. @@ -57,4 +57,4 @@ Rusty Russell \fI is mainly responsible\. Main web site: \fIhttps://github.com/ElementsProject/lightning\fR -\" SHA256STAMP:1b6598e430d416615867dc09604574c62a8ecb47c51a370711da75359ff80ef3 +\" SHA256STAMP:e9abfb1d376a1a5103cb4da2ce7d9d79d0ad6e23683ab1d68423ea4ea4a99f2f diff --git a/doc/lightning-createinvoice.7.md b/doc/lightning-createinvoice.7.md index a3c9d1f59..314a66776 100644 --- a/doc/lightning-createinvoice.7.md +++ b/doc/lightning-createinvoice.7.md @@ -4,7 +4,7 @@ lightning-createinvoice -- Low-level invoice creation SYNOPSIS -------- -**createinvoice** *invstring* *preimage* *label* +**createinvoice** *invstring* *label* *preimage* DESCRIPTION ----------- @@ -15,14 +15,13 @@ database. The *invstring* parameter is of bolt11 form, but without the final signature appended. Minimal sanity checks are done. -The *preimage* is the preimage to supply upon successful payment of -the invoice. - The *label* must be a unique string or number (which is treated as a string, so "01" is different from "1"); it is never revealed to other nodes on the lightning network, but it can be used to query the status of this invoice. +The *preimage* is the preimage to supply upon successful payment of +the invoice. RETURN VALUE ------------ diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 0c1d1f994..0098b6f73 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -1515,8 +1515,8 @@ static struct command_result *json_createinvoice(struct command *cmd, if (!param(cmd, buffer, params, p_req("invstring", param_string, &invstring), - p_req("preimage", param_preimage, &preimage), p_req("label", param_label, &label), + p_req("preimage", param_preimage, &preimage), NULL)) return command_param_failed(); @@ -1537,6 +1537,10 @@ static struct command_result *json_createinvoice(struct command *cmd, return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Missing expiry in invoice"); + if (!sha256_eq(&payment_hash, &b11->payment_hash)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Incorrect preimage"); + if (!wallet_invoice_create(cmd->ld->wallet, &invoice, b11->msat, @@ -1588,7 +1592,14 @@ static struct command_result *json_createinvoice(struct command *cmd, if (inv->relative_expiry) expiry = *inv->relative_expiry; else - expiry = 7200; + expiry = BOLT12_DEFAULT_REL_EXPIRY; + + if (!inv->payment_hash) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Missing payment_hash in invoice"); + if (!sha256_eq(&payment_hash, inv->payment_hash)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Incorrect preimage"); if (!inv->description) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, diff --git a/lightningd/offer.c b/lightningd/offer.c index e4890783b..ca3843918 100644 --- a/lightningd/offer.c +++ b/lightningd/offer.c @@ -20,8 +20,7 @@ static void json_populate_offer(struct json_stream *response, json_add_bool(response, "active", offer_status_active(status)); json_add_bool(response, "single_use", offer_status_single(status)); json_add_string(response, "bolt12", b12); - if (status == OFFER_USED) - json_add_bool(response, "used", true); + json_add_bool(response, "used", status == OFFER_USED); if (label) json_add_escaped_string(response, "label", label); } @@ -143,9 +142,9 @@ static struct command_result *json_listoffers(struct command *cmd, struct db_stmt *stmt; struct sha256 id; - for (stmt = wallet_offer_first(cmd->ld->wallet, &id); + for (stmt = wallet_offer_id_first(cmd->ld->wallet, &id); stmt; - stmt = wallet_offer_next(cmd->ld->wallet, stmt, &id)) { + stmt = wallet_offer_id_next(cmd->ld->wallet, stmt, &id)) { b12 = wallet_offer_find(tmpctx, wallet, &id, &label, &status); if (offer_status_active(status) >= *active_only) { diff --git a/lightningd/pay.c b/lightningd/pay.c index 17cdfd0b9..d8d4f887e 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1292,7 +1292,7 @@ static struct command_result *json_sendonion(struct command *cmd, p_opt("label", param_escaped_string, &label), p_opt("shared_secrets", param_secrets_array, &path_secrets), p_opt_def("partid", param_u64, &partid, 0), - /* FIXME: paramter should be invstring now */ + /* FIXME: parameter should be invstring now */ p_opt("bolt11", param_string, &invstring), p_opt_def("msatoshi", param_msat, &msat, AMOUNT_MSAT(0)), p_opt("destination", param_node_id, &destination), @@ -1447,12 +1447,12 @@ static struct command_result *json_sendpay(struct command *cmd, p_req("payment_hash", param_sha256, &rhash), p_opt("label", param_escaped_string, &label), p_opt("msatoshi", param_msat, &msat), - /* FIXME: paramter should be invstring now */ + /* FIXME: parameter should be invstring now */ p_opt("bolt11", param_string, &invstring), p_opt("payment_secret", param_secret, &payment_secret), p_opt_def("partid", param_u64, &partid, 0), #if EXPERIMENTAL_FEATURES - p_opt("local_offer_id", param_sha256, &local_offer_id), + p_opt("localofferid", param_sha256, &local_offer_id), #endif NULL)) return command_param_failed(); @@ -1561,7 +1561,7 @@ static struct command_result *json_listsendpays(struct command *cmd, const char *invstring; if (!param(cmd, buffer, params, - /* FIXME: paramter should be invstring now */ + /* FIXME: parameter should be invstring now */ p_opt("bolt11", param_string, &invstring), p_opt("payment_hash", param_sha256, &rhash), NULL)) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 0eecfa5c1..1bd9f9b1c 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1453,7 +1453,7 @@ static struct command_result *payment_createonion_success(struct command *cmd, json_add_node_id(req->js, "destination", p->destination); if (p->local_offer_id) - json_add_sha256(req->js, "local_offer_id", p->local_offer_id); + json_add_sha256(req->js, "localofferid", p->local_offer_id); send_outreq(p->plugin, req); return command_still_pending(cmd); diff --git a/plugins/pay.c b/plugins/pay.c index 885770da2..bc1004983 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1994,7 +1994,7 @@ static struct command_result *json_paymod(struct command *cmd, maxdelay_default), p_opt_def("exemptfee", param_msat, &exemptfee, AMOUNT_MSAT(5000)), #if EXPERIMENTAL_FEATURES - p_opt("local_offer_id", param_sha256, &local_offer_id), + p_opt("localofferid", param_sha256, &local_offer_id), #endif #if DEVELOPER p_opt_def("use_shadow", param_bool, &use_shadow, true), @@ -2085,7 +2085,7 @@ static struct command_result *json_paymod(struct command *cmd, if (b12->relative_expiry) invexpiry = *b12->timestamp + *b12->relative_expiry; else - invexpiry = *b12->timestamp + 7200; + invexpiry = *b12->timestamp + BOLT12_DEFAULT_REL_EXPIRY; p->local_offer_id = tal_steal(p, local_offer_id); #endif /* EXPERIMENTAL_FEATURES */ } else diff --git a/tests/test_pay.py b/tests/test_pay.py index 4f9b81bb8..3b1912f90 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1883,7 +1883,7 @@ def test_setchannelfee_usage(node_factory, bitcoind): wait_for(lambda: [c['base_fee_millisatoshi'] for c in l1.rpc.listchannels(scid)['channels']] == [DEF_BASE, 1337]) wait_for(lambda: [c['fee_per_millionth'] for c in l1.rpc.listchannels(scid)['channels']] == [DEF_PPM, 137]) - # also test with named and missing paramters + # also test with named and missing parameters result = l1.rpc.setchannelfee(ppm=42, id=scid) assert(result['base'] == DEF_BASE) assert(result['ppm'] == 42) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 18e134215..0953ce47f 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -388,7 +388,7 @@ def test_pay_plugin(node_factory): msg = 'pay bolt11 [msatoshi] [label] [riskfactor] [maxfeepercent] '\ '[retry_for] [maxdelay] [exemptfee]' if EXPERIMENTAL_FEATURES: - msg += ' [local_offer_id]' + msg += ' [localofferid]' if DEVELOPER: msg += ' [use_shadow]' assert only_one(l1.rpc.help('pay')['help'])['command'] == msg diff --git a/wallet/db.c b/wallet/db.c index 4b891990c..d9843b9f2 100644 --- a/wallet/db.c +++ b/wallet/db.c @@ -665,9 +665,9 @@ static struct migration dbmigrations[] = { ", PRIMARY KEY (offer_id)" ");"), NULL}, /* A reference into our own offers table, if it was made from one */ - {SQL("ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL;"), NULL}, + {SQL("ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);"), NULL}, /* A reference into our own offers table, if it was made from one */ - {SQL("ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL;"), NULL}, + {SQL("ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);"), NULL}, }; /* Leak tracking. */ diff --git a/wallet/db_postgres_sqlgen.c b/wallet/db_postgres_sqlgen.c index 8ce9b6f18..b288e5007 100644 --- a/wallet/db_postgres_sqlgen.c +++ b/wallet/db_postgres_sqlgen.c @@ -855,14 +855,14 @@ struct db_query db_postgres_queries[] = { .readonly = false, }, { - .name = "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL;", - .query = "ALTER TABLE invoices ADD COLUMN local_offer_id BYTEA DEFAULT NULL;", + .name = "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);", + .query = "ALTER TABLE invoices ADD COLUMN local_offer_id BYTEA DEFAULT NULL REFERENCES offers(offer_id);", .placeholders = 0, .readonly = false, }, { - .name = "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL;", - .query = "ALTER TABLE payments ADD COLUMN local_offer_id BYTEA DEFAULT NULL;", + .name = "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);", + .query = "ALTER TABLE payments ADD COLUMN local_offer_id BYTEA DEFAULT NULL REFERENCES offers(offer_id);", .placeholders = 0, .readonly = false, }, @@ -1762,4 +1762,4 @@ struct db_query db_postgres_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_POSTGRES */ -// SHA256STAMP:774d5116e102d98351730072b4aa0b48d27b05c364680fc406f8b12bf4c7e293 +// SHA256STAMP:26bbd985dc45de765f06b8c2644ecd76c098532ff71a33c8f71608890631d726 diff --git a/wallet/db_sqlite3_sqlgen.c b/wallet/db_sqlite3_sqlgen.c index 2b83c78c5..d171d7fba 100644 --- a/wallet/db_sqlite3_sqlgen.c +++ b/wallet/db_sqlite3_sqlgen.c @@ -855,14 +855,14 @@ struct db_query db_sqlite3_queries[] = { .readonly = false, }, { - .name = "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL;", - .query = "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL;", + .name = "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);", + .query = "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);", .placeholders = 0, .readonly = false, }, { - .name = "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL;", - .query = "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL;", + .name = "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);", + .query = "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);", .placeholders = 0, .readonly = false, }, @@ -1762,4 +1762,4 @@ struct db_query db_sqlite3_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_SQLITE3 */ -// SHA256STAMP:774d5116e102d98351730072b4aa0b48d27b05c364680fc406f8b12bf4c7e293 +// SHA256STAMP:26bbd985dc45de765f06b8c2644ecd76c098532ff71a33c8f71608890631d726 diff --git a/wallet/statements_gettextgen.po b/wallet/statements_gettextgen.po index 2750c17fc..f6ee36515 100644 --- a/wallet/statements_gettextgen.po +++ b/wallet/statements_gettextgen.po @@ -563,11 +563,11 @@ msgid "CREATE TABLE offers ( offer_id BLOB, bolt12 TEXT, label TEXT, status INT msgstr "" #: wallet/db.c:668 -msgid "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL;" +msgid "ALTER TABLE invoices ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);" msgstr "" #: wallet/db.c:670 -msgid "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL;" +msgid "ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);" msgstr "" #: wallet/db.c:897 @@ -1130,23 +1130,23 @@ msgstr "" msgid "INSERT INTO offers ( offer_id, bolt12, label, status) VALUES (?, ?, ?, ?);" msgstr "" -#: wallet/wallet.c:4084 +#: wallet/wallet.c:4083 msgid "SELECT bolt12, label, status FROM offers WHERE offer_id = ?;" msgstr "" -#: wallet/wallet.c:4112 +#: wallet/wallet.c:4111 msgid "SELECT offer_id FROM offers;" msgstr "" -#: wallet/wallet.c:4138 +#: wallet/wallet.c:4137 msgid "UPDATE offers SET status=? WHERE offer_id = ?;" msgstr "" -#: wallet/wallet.c:4149 +#: wallet/wallet.c:4148 msgid "UPDATE invoices SET state=? WHERE state=? AND local_offer_id = ?;" msgstr "" -#: wallet/wallet.c:4177 +#: wallet/wallet.c:4176 msgid "SELECT status FROM offers WHERE offer_id = ?;" msgstr "" @@ -1161,4 +1161,4 @@ msgstr "" #: wallet/test/run-wallet.c:1378 msgid "INSERT INTO channels (id) VALUES (1);" msgstr "" -# SHA256STAMP:50622dd1a8b0f2fe71efa1c1d175f7ad130f3042db84e0c9547f849a328e8f5d +# SHA256STAMP:1e8fd669b4322c65584b19aa1613b74ac1daa8d80a5c02e0dc1381e2b9e08f9e diff --git a/wallet/wallet.c b/wallet/wallet.c index ac6abc056..bae86f1ec 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -4080,7 +4080,6 @@ char *wallet_offer_find(const tal_t *ctx, struct db_stmt *stmt; char *bolt12; - /* Test if already exists. */ stmt = db_prepare_v2(w->db, SQL("SELECT bolt12, label, status" " FROM offers" " WHERE offer_id = ?;")); @@ -4105,19 +4104,19 @@ char *wallet_offer_find(const tal_t *ctx, return bolt12; } -struct db_stmt *wallet_offer_first(struct wallet *w, struct sha256 *offer_id) +struct db_stmt *wallet_offer_id_first(struct wallet *w, struct sha256 *offer_id) { struct db_stmt *stmt; stmt = db_prepare_v2(w->db, SQL("SELECT offer_id FROM offers;")); db_query_prepared(stmt); - return wallet_offer_next(w, stmt, offer_id); + return wallet_offer_id_next(w, stmt, offer_id); } -struct db_stmt *wallet_offer_next(struct wallet *w, - struct db_stmt *stmt, - struct sha256 *offer_id) +struct db_stmt *wallet_offer_id_next(struct wallet *w, + struct db_stmt *stmt, + struct sha256 *offer_id) { if (!db_step(stmt)) return tal_free(stmt); @@ -4180,14 +4179,16 @@ void wallet_offer_mark_used(struct db *db, const struct sha256 *offer_id) db_bind_sha256(stmt, 0, offer_id); db_query_prepared(stmt); if (!db_step(stmt)) - fatal("Unknown stats offer_id %s", + fatal("%s: unknown offer_id %s", + __func__, type_to_string(tmpctx, struct sha256, offer_id)); status = offer_status_in_db(db_column_int(stmt, 0)); tal_free(stmt); if (!offer_status_active(status)) - fatal("offer_id %s status %i", + fatal("%s: offer_id %s not active: status %i", + __func__, type_to_string(tmpctx, struct sha256, offer_id), status); diff --git a/wallet/wallet.h b/wallet/wallet.h index cfbced10f..56ae19144 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -1428,23 +1428,23 @@ char *wallet_offer_find(const tal_t *ctx, * @offer_id: the first offer id (if returns non-NULL) * * Returns pointer to hand as @stmt to wallet_offer_next(), or NULL. - * If you choose not to call wallet_offer_next() you must free it! + * If you choose not to call wallet_offer_id_next() you must free it! */ -struct db_stmt *wallet_offer_first(struct wallet *w, - struct sha256 *offer_id); +struct db_stmt *wallet_offer_id_first(struct wallet *w, + struct sha256 *offer_id); /** * Iterate through all the offers. * @w: the wallet - * @stmt: return from wallet_offer_first() or previous wallet_offer_next() + * @stmt: return from wallet_offer_id_first() or previous wallet_offer_id_next() * @offer_id: the next offer id (if returns non-NULL) * * Returns NULL once we're out of offers. If you choose not to call - * wallet_offer_next() again you must free return. + * wallet_offer_id_next() again you must free return. */ -struct db_stmt *wallet_offer_next(struct wallet *w, - struct db_stmt *stmt, - struct sha256 *offer_id); +struct db_stmt *wallet_offer_id_next(struct wallet *w, + struct db_stmt *stmt, + struct sha256 *offer_id); /** * Disable an offer in the database.