Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
178892b
db: add partid, total_msat fields to payment entries.
rustyrussell Dec 11, 2019
ad4ed97
db: add partid field to htlc_out.
rustyrussell Dec 11, 2019
2a03434
htlcs: remove origin_htlc_id from htlc_out.
rustyrussell Dec 11, 2019
c654478
lightningd: share more code between sendpay and sendonion.
rustyrussell Dec 11, 2019
04a46f0
lightningd: change amount-in-flight check to be more nuanced.
rustyrussell Dec 11, 2019
c61227b
sendpay/sendonion: add optional partid arg, finesse msatoshi argument.
rustyrussell Dec 11, 2019
15fa972
configure: make partid payments only available with EXPERIMENTAL_FEAT…
rustyrussell Dec 11, 2019
3bc4636
waitsendpay: add partid arg.
rustyrussell Dec 11, 2019
94d3897
pytest: Add tests to make sure received onion is as expected.
rustyrussell Dec 11, 2019
2e4416e
doc: update experimental bolt version quotes.
rustyrussell Dec 11, 2019
d94ae31
lightningd: cleanup redundant args from handle_localpay
rustyrussell Dec 11, 2019
3c6e33a
lightningd: split invoice check into separate function.
rustyrussell Dec 11, 2019
555b217
lightningd: implement htlc sets.
rustyrussell Dec 11, 2019
73bf9e0
lightningd: wrap htlc replay in a database transaction.
rustyrussell Dec 11, 2019
1839483
lightningd: sew in htlc set.
rustyrussell Dec 11, 2019
8cee375
plugins: listpays ignores pre-0.7.0 or manual sendpay payments w/ no …
rustyrussell Dec 11, 2019
84a2753
plugins: listpays will now consolidate multi-part payments.
rustyrussell Dec 11, 2019
c6bbb41
common: offer option_basic_mpp for EXPERIMENTAL_FEATURES.
rustyrussell Dec 11, 2019
cbfc84f
pytest: add more multi-part-payment tests.
rustyrussell Dec 11, 2019
2b4ca09
lightningd: require payment_secret for MPP.
rustyrussell Dec 11, 2019
207ae69
lightningd: fix spurious "more than twice final" error.
rustyrussell Dec 12, 2019
e6edb76
lightningd: fix failure message in waitsendpay with multi-part payments.
rustyrussell Dec 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/bolt11.c
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ static char *decode_n(struct bolt11 *b11,
return NULL;
}

/* BOLT-412c260b537be95b105ae2062463f1d992024ca7 #11:
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #11:
*
* * `s` (16): `data_length` 52. This 256-bit secret prevents
* forwarding nodes from probing the payment recipient.
Expand All @@ -317,7 +317,7 @@ static char *decode_s(struct bolt11 *b11,
return unknown_field(b11, hu5, data, data_len, 's',
data_length);

/* BOLT-412c260b537be95b105ae2062463f1d992024ca7 #11:
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #11:
*
* A reader... MUST skip over unknown fields, OR an `f` field
* with unknown `version`, OR `p`, `h`, `s` or `n` fields that do
Expand Down
1 change: 1 addition & 0 deletions common/features.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ static const u32 our_features[] = {
#if EXPERIMENTAL_FEATURES
OPTIONAL_FEATURE(OPT_VAR_ONION),
OPTIONAL_FEATURE(OPT_PAYMENT_SECRET),
OPTIONAL_FEATURE(OPT_BASIC_MPP),
#endif
OPTIONAL_FEATURE(OPT_GOSSIP_QUERIES_EX),
OPTIONAL_FEATURE(OPT_STATIC_REMOTEKEY),
Expand Down
2 changes: 1 addition & 1 deletion common/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void set_feature_bit(u8 **ptr, u32 bit);
#define OPT_GOSSIP_QUERIES_EX 10
#define OPT_STATIC_REMOTEKEY 12

/* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #9:
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #9:
*
* | 14/15 | `payment_secret` |... IN9 ...
* | 16/17 | `basic_mpp` |... IN9 ...
Expand Down
4 changes: 2 additions & 2 deletions common/onion.c
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ struct onion_payload *onion_decode(const tal_t *ctx,
if (rs->nextcase == ONION_FORWARD) {
p->total_msat = NULL;
} else {
/* BOLT-e36f7b6517e1173dcbd49da3b516cfe1f48ae556 #4:
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - if it is the final node:
* - MUST treat `total_msat` as if it were equal to
* `amt_to_forward` if it is not present. */
Expand Down Expand Up @@ -279,7 +279,7 @@ struct onion_payload *onion_decode(const tal_t *ctx,
p->total_msat = NULL;
} else {
p->forward_channel = NULL;
/* BOLT-e36f7b6517e1173dcbd49da3b516cfe1f48ae556 #4:
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - if it is the final node:
* - MUST treat `total_msat` as if it were equal to
* `amt_to_forward` if it is not present. */
Expand Down
1 change: 1 addition & 0 deletions lightningd/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ LIGHTNINGD_SRC := \
lightningd/gossip_msg.c \
lightningd/hsm_control.c \
lightningd/htlc_end.c \
lightningd/htlc_set.c \
lightningd/invoice.c \
lightningd/io_loop_with_timers.c \
lightningd/json.c \
Expand Down
19 changes: 19 additions & 0 deletions lightningd/htlc_end.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@ struct htlc_in *find_htlc_in(const struct htlc_in_map *map,
return htlc_in_map_get(map, &key);
}

struct htlc_in *remove_htlc_in_by_dbid(struct htlc_in_map *remaining_htlcs_in,
u64 dbid)
{
struct htlc_in *hin;
struct htlc_in_map_iter ini;

for (hin = htlc_in_map_first(remaining_htlcs_in, &ini); hin;
hin = htlc_in_map_next(remaining_htlcs_in, &ini)) {
if (hin->dbid == dbid) {
htlc_in_map_del(remaining_htlcs_in, hin);
return hin;
}
}
return NULL;
}

static void destroy_htlc_in(struct htlc_in *hend, struct htlc_in_map *map)
{
htlc_in_map_del(map, hend);
Expand Down Expand Up @@ -252,6 +268,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
const struct sha256 *payment_hash,
const u8 *onion_routing_packet,
bool am_origin,
u64 partid,
struct htlc_in *in)
{
struct htlc_out *hout = tal(ctx, struct htlc_out);
Expand All @@ -273,6 +290,8 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
hout->preimage = NULL;

hout->am_origin = am_origin;
if (am_origin)
hout->partid = partid;
hout->in = NULL;
if (in)
htlc_out_connect_htlc_in(hout, in);
Expand Down
9 changes: 8 additions & 1 deletion lightningd/htlc_end.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ struct htlc_out {
* is saved to the database, must be >0 after saving to the
* database. */
u64 dbid;
u64 origin_htlc_id;
struct htlc_key key;
struct amount_msat msat;
u32 cltv_expiry;
Expand All @@ -82,6 +81,9 @@ struct htlc_out {
/* Is this a locally-generated payment? Implies ->in is NULL. */
bool am_origin;

/* If am_origin, this is the partid of the payment. */
u64 partid;

/* Where it's from, if not going to us. */
struct htlc_in *in;
};
Expand Down Expand Up @@ -120,6 +122,10 @@ struct htlc_in *find_htlc_in(const struct htlc_in_map *map,
const struct channel *channel,
u64 htlc_id);

/* FIXME: Slow function only used at startup. */
struct htlc_in *remove_htlc_in_by_dbid(struct htlc_in_map *remaining_htlcs_in,
u64 dbid);

struct htlc_out *find_htlc_out(const struct htlc_out_map *map,
const struct channel *channel,
u64 htlc_id);
Expand All @@ -140,6 +146,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
const struct sha256 *payment_hash,
const u8 *onion_routing_packet,
bool am_origin,
u64 partid,
struct htlc_in *in);

void connect_htlc_in(struct htlc_in_map *map, struct htlc_in *hin);
Expand Down
212 changes: 212 additions & 0 deletions lightningd/htlc_set.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
#include <common/memleak.h>
#include <common/timeout.h>
#include <lightningd/htlc_end.h>
#include <lightningd/htlc_set.h>
#include <lightningd/lightningd.h>
#include <lightningd/peer_htlcs.h>

#if EXPERIMENTAL_FEATURES
/* If an HTLC times out, we need to free entire set, since we could be processing
* it in invoice.c right now. */
static void htlc_set_hin_destroyed(struct htlc_in *hin,
struct htlc_set *set)
{
for (size_t i = 0; i < tal_count(set->htlcs); i++) {
if (set->htlcs[i] == hin) {
/* Don't try to re-fail this HTLC! */
tal_arr_remove(&set->htlcs, i);
/* Kind of the correct failure code. */
htlc_set_fail(set, WIRE_MPP_TIMEOUT);
return;
}
}
abort();
}

static void destroy_htlc_set(struct htlc_set *set,
struct htlc_set_map *map)
{
htlc_set_map_del(map, set);
}

/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - MUST fail all HTLCs in the HTLC set after some reasonable
* timeout.
* - SHOULD use `mpp_timeout` for the failure message.
*/
static void timeout_htlc_set(struct htlc_set *set)
{
htlc_set_fail(set, WIRE_MPP_TIMEOUT);
}
#endif /* EXPERIMENTAL_FEATURES */

void htlc_set_fail(struct htlc_set *set, enum onion_type failcode)
{
for (size_t i = 0; i < tal_count(set->htlcs); i++) {
#if EXPERIMENTAL_FEATURES
/* Don't remove from set */
tal_del_destructor2(set->htlcs[i], htlc_set_hin_destroyed, set);
#endif
fail_htlc(set->htlcs[i], failcode);
}
tal_free(set);
}

void htlc_set_fulfill(struct htlc_set *set, const struct preimage *preimage)
{
for (size_t i = 0; i < tal_count(set->htlcs); i++) {
#if EXPERIMENTAL_FEATURES
/* Don't remove from set */
tal_del_destructor2(set->htlcs[i], htlc_set_hin_destroyed, set);
#endif
fulfill_htlc(set->htlcs[i], preimage);
}
tal_free(set);
}

static struct htlc_set *new_htlc_set(struct lightningd *ld,
struct htlc_in *hin,
struct amount_msat total_msat)
{
struct htlc_set *set;

set = tal(ld, struct htlc_set);
set->total_msat = total_msat;
set->payment_hash = hin->payment_hash;
Comment thread
cdecker marked this conversation as resolved.
set->so_far = AMOUNT_MSAT(0);
set->htlcs = tal_arr(set, struct htlc_in *, 1);
set->htlcs[0] = hin;

#if EXPERIMENTAL_FEATURES
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - MUST fail all HTLCs in the HTLC set after some reasonable
* timeout.
* - SHOULD wait for at least 60 seconds after the initial
* HTLC.
*/
notleak(new_reltimer(ld->timers, set, time_from_sec(70),
timeout_htlc_set, set));
htlc_set_map_add(&ld->htlc_sets, set);
tal_add_destructor2(set, destroy_htlc_set, &ld->htlc_sets);
#endif
return set;
}

void htlc_set_add(struct lightningd *ld,
struct htlc_in *hin,
struct amount_msat total_msat,
Comment thread
cdecker marked this conversation as resolved.
const struct secret *payment_secret)
{
struct htlc_set *set;
const struct invoice_details *details;

/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* The final node:
* - MUST fail the HTLC if dictated by Requirements under
* [Failure Messages](#failure-messages)
* - Note: "amount paid" specified there is the `total_msat` field.
*/
details = invoice_check_payment(tmpctx, ld, &hin->payment_hash,
total_msat, payment_secret);
if (!details) {
fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS);
return;
}

#if !EXPERIMENTAL_FEATURES
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - if it does not support `basic_mpp`:
* - MUST fail the HTLC if `total_msat` is not exactly equal to
* `amt_to_forward`.
*/
if (!amount_msat_eq(hin->msat, total_msat)) {
fail_htlc(hin, WIRE_FINAL_INCORRECT_HTLC_AMOUNT);
return;
}

/* We create a transient set which just has one entry. */
set = new_htlc_set(ld, hin, total_msat);
#else
/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - otherwise, if it supports `basic_mpp`:
* - MUST add it to the HTLC set corresponding to that `payment_hash`.
* - if the total `amount_msat` of this HTLC set equals `total_msat`:
* - SHOULD fulfill all HTLCs in the HTLC set
*/
set = htlc_set_map_get(&ld->htlc_sets, &hin->payment_hash);
if (!set)
set = new_htlc_set(ld, hin, total_msat);
else {
/* BOLT-0729433704dd11cc07a0535c09e5f64de7a5017b #4:
*
* if it supports `basic_mpp`:
* ...
* - otherwise, if the total `amount_msat` of this HTLC set is
* less than `total_msat`:
* ...
* - MUST require `payment_secret` for all HTLCs in the set.
*/
/* We check this now, since we want to fail with this as soon
* as possible, to avoid other probing attacks. */
if (!payment_secret) {
fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS);
return;
}
tal_arr_expand(&set->htlcs, hin);
}

/* Remove from set should hin get destroyed somehow */
tal_add_destructor2(hin, htlc_set_hin_destroyed, set);

/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - SHOULD fail the entire HTLC set if `total_msat` is not
* the same for all HTLCs in the set.
*/
if (!amount_msat_eq(total_msat, set->total_msat)) {
log_unusual(ld->log, "Failing HTLC set %s:"
" total_msat %s new htlc total %s",
type_to_string(tmpctx, struct sha256,
&set->payment_hash),
type_to_string(tmpctx, struct amount_msat,
&set->total_msat),
type_to_string(tmpctx, struct amount_msat,
&total_msat));
htlc_set_fail(set, WIRE_FINAL_INCORRECT_HTLC_AMOUNT);
return;
}
#endif /* EXPERIMENTAL_FEATURES */

/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - if the total `amount_msat` of this HTLC set equals `total_msat`:
* - SHOULD fulfill all HTLCs in the HTLC set
*/
if (!amount_msat_add(&set->so_far, set->so_far, hin->msat)) {
log_unusual(ld->log, "Failing HTLC set %s:"
" overflow adding %s+%s",
type_to_string(tmpctx, struct sha256,
&set->payment_hash),
type_to_string(tmpctx, struct amount_msat,
&set->so_far),
type_to_string(tmpctx, struct amount_msat,
&hin->msat));
htlc_set_fail(set, WIRE_FINAL_INCORRECT_HTLC_AMOUNT);
return;
}

if (amount_msat_eq(set->so_far, total_msat)) {
invoice_try_pay(ld, set, details);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be take(set) for self-documentation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, no, it has to dispose of it. take() is for things where it may or may not take ownership.

We've generally used naming to try to indicate which functions are a sink. Should we rename this to 'invoice_resolve_htlc_in'?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds like a good idea. I keep getting confused by take() :-)

I'm tempted to propose an OWNS annotation and an own() nop-helper that just serves as annotation to say: I'll take ownership no matter what and you MUST take ownership. Purely documentation but we'd know which side of the call is wrong if we have a leak without having to dig through both the caller and callee to see what the semantics should have been. Just fantasizing here of course 😃

return;
}

/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4:
* - otherwise, if the total `amount_msat` of this HTLC set is less than
* `total_msat`:
* - MUST NOT fulfill any HTLCs in the HTLC set
*...
* - MUST require `payment_secret` for all HTLCs in the set. */
/* This catches the case of the first payment in a set. */
if (!payment_secret) {
htlc_set_fail(set, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS);
return;
}
}
Loading