lukechilds
/
lightning
mirror of https://github.com/lukechilds/lightning.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

channeld: allow transient negative balance. 

Travis randomly picked up an error in test_feerate_stress:
**BROKEN** 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-channeld-chan#1: Cannot add htlc #0 10000msat to LOCAL (version a2541b9-modded)

This is because it hit an unlikely corner case involving applying multiple HTLCs
(similar to the previous c96cee9b8d).

In this case, the test sends a 500,000,000 "balancing" setup payment L1->L2.
It waits for L2 to get the preimage (which is the when pay() helper returns),
but crucially, it starts spamming with HTLCs before that HTLC is completely
removed.

From L2's point of view, the setup HTLC is in state RCVD_REMOVE_REVOCATION;
gone from L1's commitment tx, but still waiting for the commitment_signed
from L1 to remove it from L2's.

Note that each side keeps a local and remove view of both sides' current
balances: at this point, L2's view is REMOTE: "500,000,000 to L1, 499,900,000
to L2", LOCAL: "500,000,000 to L1, 0 to L2".

L2 sends a 10,000 msat HTLC to L1: legal, since L1 will allow it,
then the commitment_signed.  L1 sends the revoke-and-ack for this,
*then* belatedly follows with the commitment_signed which both completes the
removal of the setup HTLC and adds the new one.

But L2 processes the HTLCs in hashtable (i.e. random) order: so if it
tries to apply its own HTLC first, it freaks out because it doesn't have
funds in its local view.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Unlikely corner case is simultanous HTLCs near balance limits fixed.
travis-debug
Rusty Russell 5 years ago
parent
d1df4d6959
commit
6defc69482
1 changed files with 136 additions and 179 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 315
      channeld/full_channel.c

315
channeld/full_channel.c
View File

@ -32,6 +32,59 @@ static void memleak_help_htlcmap(struct htable *memtable,
} }
#endif /* DEVELOPER */ #endif /* DEVELOPER */
/* This is a dangerous thing! Because we apply HTLCs in many places
* in bulk, we can temporarily go negative. You must check balance_ok()
* at the end! */
struct balance {
s64 msat;
};
static void to_balance(struct balance *balance,
const struct amount_msat msat)
{
balance->msat = msat.millisatoshis; /* Raw: balance */
assert(balance->msat >= 0);
}
/* What does adding the HTLC do to the balance for this side (subtracts) */
static void balance_add_htlc(struct balance *balance,
const struct htlc *htlc,
enum side side)
{
if (htlc_owner(htlc) == side)
balance->msat -= htlc->amount.millisatoshis; /* Raw: balance */
}
/* What does removing the HTLC do to the balance for this side (adds) */
static void balance_remove_htlc(struct balance *balance,
const struct htlc *htlc,
enum side side)
{
enum side paid_to;
/* Fulfilled HTLCs are paid to recipient, otherwise returns to owner */
if (htlc->r)
paid_to = !htlc_owner(htlc);
else
paid_to = htlc_owner(htlc);
if (side == paid_to)
balance->msat += htlc->amount.millisatoshis; /* Raw: balance */
}
static bool balance_ok(const struct balance *balance,
struct amount_msat *msat)
WARN_UNUSED_RESULT;
static bool balance_ok(const struct balance *balance,
struct amount_msat *msat)
{
if (balance->msat < 0)
return false;
msat->millisatoshis = balance->msat; /* Raw: balance */
return true;
}
struct channel *new_full_channel(const tal_t *ctx, struct channel *new_full_channel(const tal_t *ctx,
const struct bitcoin_txid *funding_txid, const struct bitcoin_txid *funding_txid,
unsigned int funding_txout, unsigned int funding_txout,
@ -81,34 +134,6 @@ static void htlc_arr_append(const struct htlc ***arr, const struct htlc *htlc)
tal_arr_expand(arr, htlc); tal_arr_expand(arr, htlc);
} }
/* What does adding the HTLC do to the balance for this side (subtracts) */
static bool WARN_UNUSED_RESULT balance_add_htlc(struct amount_msat *msat,
const struct htlc *htlc,
enum side side)
{
if (htlc_owner(htlc) == side)
return amount_msat_sub(msat, *msat, htlc->amount);
return true;
}
/* What does removing the HTLC do to the balance for this side (adds) */
static bool WARN_UNUSED_RESULT balance_remove_htlc(struct amount_msat *msat,
const struct htlc *htlc,
enum side side)
{
enum side paid_to;
/* Fulfilled HTLCs are paid to recipient, otherwise returns to owner */
if (htlc->r)
paid_to = !htlc_owner(htlc);
else
paid_to = htlc_owner(htlc);
if (side == paid_to)
return amount_msat_add(msat, *msat, htlc->amount);
return true;
}
static void dump_htlc(const struct htlc *htlc, const char *prefix) static void dump_htlc(const struct htlc *htlc, const char *prefix)
{ {
enum htlc_state remote_state; enum htlc_state remote_state;
@ -304,34 +329,33 @@ static bool get_room_above_reserve(const struct channel *channel,
const struct htlc **adding, const struct htlc **adding,
const struct htlc **removing, const struct htlc **removing,
enum side side, enum side side,
struct amount_msat *balance) struct amount_msat *remainder)
{ {
bool ok;
/* Reserve is set by the *other* side */ /* Reserve is set by the *other* side */
struct amount_sat reserve = channel->config[!side].channel_reserve; struct amount_sat reserve = channel->config[!side].channel_reserve;
struct balance balance;
*balance = view->owed[side]; to_balance(&balance, view->owed[side]);
ok = true;
for (size_t i = 0; i < tal_count(removing); i++) for (size_t i = 0; i < tal_count(removing); i++)
ok &= balance_remove_htlc(balance, removing[i], side); balance_remove_htlc(&balance, removing[i], side);
for (size_t i = 0; i < tal_count(adding); i++) for (size_t i = 0; i < tal_count(adding); i++)
ok &= balance_add_htlc(balance, adding[i], side); balance_add_htlc(&balance, adding[i], side);
/* Can happen if amount completely exceeds capacity */ /* Can happen if amount completely exceeds capacity */
if (!ok) { if (!balance_ok(&balance, remainder)) {
status_debug("Failed to add %zu remove %zu htlcs", status_debug("Failed to add %zu remove %zu htlcs",
tal_count(adding), tal_count(removing)); tal_count(adding), tal_count(removing));
return false; return false;
} }
if (!amount_msat_sub_sat(balance, *balance, reserve)) { if (!amount_msat_sub_sat(remainder, *remainder, reserve)) {
status_debug("%s cannot afford htlc: would make balance %s" status_debug("%s cannot afford htlc: would make balance %s"
" below reserve %s", " below reserve %s",
side_to_str(side), side_to_str(side),
type_to_string(tmpctx, struct amount_msat, type_to_string(tmpctx, struct amount_msat,
balance), remainder),
type_to_string(tmpctx, struct amount_sat, type_to_string(tmpctx, struct amount_sat,
&reserve)); &reserve));
return false; return false;
@ -511,7 +535,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
* - SHOULD fail the channel. * - SHOULD fail the channel.
*/ */
if (enforce_aggregate_limits) { if (enforce_aggregate_limits) {
struct amount_msat balance; struct amount_msat remainder;
struct amount_sat fee = fee_for_htlcs(channel, view, struct amount_sat fee = fee_for_htlcs(channel, view,
committed, committed,
adding, adding,
@ -525,20 +549,20 @@ static enum channel_add_err add_htlc(struct channel *channel,
* *
* The change is being applied to the receiver but it will * The change is being applied to the receiver but it will
* come back to the sender after revoke_and_ack. So the check * come back to the sender after revoke_and_ack. So the check
* here is that the balance to the sender doesn't go below the * here is that the remainder to the sender doesn't go below the
* sender's reserve. */ * sender's reserve. */
if (!get_room_above_reserve(channel, view, if (!get_room_above_reserve(channel, view,
adding, removing, sender, adding, removing, sender,
&balance)) &remainder))
return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
if (channel->funder == sender) { if (channel->funder == sender) {
if (amount_msat_less_sat(balance, fee)) { if (amount_msat_less_sat(remainder, fee)) {
status_debug("Cannot afford fee %s with %s above reserve", status_debug("Cannot afford fee %s with %s above reserve",
type_to_string(tmpctx, struct amount_sat, type_to_string(tmpctx, struct amount_sat,
&fee), &fee),
type_to_string(tmpctx, struct amount_msat, type_to_string(tmpctx, struct amount_msat,
&balance)); &remainder));
return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
} }
} }
@ -549,7 +573,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
if (!get_room_above_reserve(channel, view, if (!get_room_above_reserve(channel, view,
adding, removing, adding, removing,
channel->funder, channel->funder,
&balance)) &remainder))
return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
/* Should be able to afford both their own commit tx /* Should be able to afford both their own commit tx
* fee, and other's commit tx fee, which are subtly * fee, and other's commit tx fee, which are subtly
@ -562,14 +586,14 @@ static enum channel_add_err add_htlc(struct channel *channel,
/* set fee output pointer if given */ /* set fee output pointer if given */
if (htlc_fee && amount_sat_greater(fee, *htlc_fee)) if (htlc_fee && amount_sat_greater(fee, *htlc_fee))
*htlc_fee = fee; *htlc_fee = fee;
if (amount_msat_less_sat(balance, fee)) { if (amount_msat_less_sat(remainder, fee)) {
status_debug("Funder could not afford own fee %s with %s above reserve", status_debug("Funder could not afford own fee %s with %s above reserve",
type_to_string(tmpctx, type_to_string(tmpctx,
struct amount_sat, struct amount_sat,
&fee), &fee),
type_to_string(tmpctx, type_to_string(tmpctx,
struct amount_msat, struct amount_msat,
&balance)); &remainder));
return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
} }
fee = fee_for_htlcs(channel, view, fee = fee_for_htlcs(channel, view,
@ -580,14 +604,14 @@ static enum channel_add_err add_htlc(struct channel *channel,
/* set fee output pointer if given */ /* set fee output pointer if given */
if (htlc_fee && amount_sat_greater(fee, *htlc_fee)) if (htlc_fee && amount_sat_greater(fee, *htlc_fee))
*htlc_fee = fee; *htlc_fee = fee;
if (amount_msat_less_sat(balance, fee)) { if (amount_msat_less_sat(remainder, fee)) {
status_debug("Funder could not afford peer's fee %s with %s above reserve", status_debug("Funder could not afford peer's fee %s with %s above reserve",
type_to_string(tmpctx, type_to_string(tmpctx,
struct amount_sat, struct amount_sat,
&fee), &fee),
type_to_string(tmpctx, type_to_string(tmpctx,
struct amount_msat, struct amount_msat,
&balance)); &remainder));
return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
} }
} }
@ -750,7 +774,8 @@ enum channel_remove_err channel_fail_htlc(struct channel *channel,
static void htlc_incstate(struct channel *channel, static void htlc_incstate(struct channel *channel,
struct htlc *htlc, struct htlc *htlc,
enum side sidechanged) enum side sidechanged,
struct balance owed[NUM_SIDES])
{ {
int preflags, postflags; int preflags, postflags;
const int committed_f = HTLC_FLAG(sidechanged, HTLC_F_COMMITTED); const int committed_f = HTLC_FLAG(sidechanged, HTLC_F_COMMITTED);
@ -769,61 +794,21 @@ static void htlc_incstate(struct channel *channel,
/* If we've added or removed, adjust balances. */ /* If we've added or removed, adjust balances. */
if (!(preflags & committed_f) && (postflags & committed_f)) { if (!(preflags & committed_f) && (postflags & committed_f)) {
status_debug("htlc added %s: local %s remote %s", status_debug("htlc added %s: local %"PRId64" remote %"PRId64,
side_to_str(sidechanged), side_to_str(sidechanged),
type_to_string(tmpctx, struct amount_msat, owed[LOCAL].msat, owed[REMOTE].msat);
&channel->view[sidechanged].owed[LOCAL]), balance_add_htlc(&owed[LOCAL], htlc, LOCAL);
type_to_string(tmpctx, struct amount_msat, balance_add_htlc(&owed[REMOTE], htlc, REMOTE);
&channel->view[sidechanged].owed[REMOTE])); status_debug("-> local %"PRId64" remote %"PRId64,
if (!balance_add_htlc(&channel->view[sidechanged].owed[LOCAL], owed[LOCAL].msat, owed[REMOTE].msat);
htlc, LOCAL))
status_failed(STATUS_FAIL_INTERNAL_ERROR,
"Cannot add htlc #%"PRIu64" %s"
" to LOCAL",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
if (!balance_add_htlc(&channel->view[sidechanged].owed[REMOTE],
htlc, REMOTE))
status_failed(STATUS_FAIL_INTERNAL_ERROR,
"Cannot add htlc #%"PRIu64" %s"
" to REMOTE",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
status_debug("-> local %s remote %s",
type_to_string(tmpctx, struct amount_msat,
&channel->view[sidechanged].owed[LOCAL]),
type_to_string(tmpctx, struct amount_msat,
&channel->view[sidechanged].owed[REMOTE]));
} else if ((preflags & committed_f) && !(postflags & committed_f)) { } else if ((preflags & committed_f) && !(postflags & committed_f)) {
status_debug("htlc added %s: local %s remote %s", status_debug("htlc added %s: local %"PRId64" remote %"PRId64,
side_to_str(sidechanged), side_to_str(sidechanged),
type_to_string(tmpctx, struct amount_msat, owed[LOCAL].msat, owed[REMOTE].msat);
&channel->view[sidechanged].owed[LOCAL]), balance_remove_htlc(&owed[LOCAL], htlc, LOCAL);
type_to_string(tmpctx, struct amount_msat, balance_remove_htlc(&owed[REMOTE], htlc, REMOTE);
&channel->view[sidechanged].owed[REMOTE])); status_debug("-> local %"PRId64" remote %"PRId64,
if (!balance_remove_htlc(&channel->view[sidechanged].owed[LOCAL], owed[LOCAL].msat, owed[REMOTE].msat);
htlc, LOCAL))
status_failed(STATUS_FAIL_INTERNAL_ERROR,
"Cannot remove htlc #%"PRIu64" %s"
" from LOCAL",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
if (!balance_remove_htlc(&channel->view[sidechanged].owed[REMOTE],
htlc, REMOTE))
status_failed(STATUS_FAIL_INTERNAL_ERROR,
"Cannot remove htlc #%"PRIu64" %s"
" from REMOTE",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
status_debug("-> local %s remote %s",
type_to_string(tmpctx, struct amount_msat,
&channel->view[sidechanged].owed[LOCAL]),
type_to_string(tmpctx, struct amount_msat,
&channel->view[sidechanged].owed[REMOTE]));
} }
} }
@ -839,13 +824,17 @@ static int change_htlcs(struct channel *channel,
struct htlc *h; struct htlc *h;
int cflags = 0; int cflags = 0;
size_t i; size_t i;
struct balance owed[NUM_SIDES];
for (i = 0; i < NUM_SIDES; i++)
to_balance(&owed[i], channel->view[sidechanged].owed[i]);
for (h = htlc_map_first(channel->htlcs, &it); for (h = htlc_map_first(channel->htlcs, &it);
h; h;
h = htlc_map_next(channel->htlcs, &it)) { h = htlc_map_next(channel->htlcs, &it)) {
for (i = 0; i < n_hstates; i++) { for (i = 0; i < n_hstates; i++) {
if (h->state == htlc_states[i]) { if (h->state == htlc_states[i]) {
htlc_incstate(channel, h, sidechanged); htlc_incstate(channel, h, sidechanged, owed);
dump_htlc(h, prefix); dump_htlc(h, prefix);
htlc_arr_append(htlcs, h); htlc_arr_append(htlcs, h);
cflags |= (htlc_state_flags(htlc_states[i]) cflags |= (htlc_state_flags(htlc_states[i])
@ -853,6 +842,19 @@ static int change_htlcs(struct channel *channel,
} }
} }
} }
for (i = 0; i < NUM_SIDES; i++) {
if (!balance_ok(&owed[i], &channel->view[sidechanged].owed[i])) {
status_failed(STATUS_FAIL_INTERNAL_ERROR,
"%s: %s balance underflow: %s -> %"PRId64,
side_to_str(sidechanged),
side_to_str(i),
type_to_string(tmpctx, struct amount_msat,
&channel->view[sidechanged].owed[i]),
owed[i].msat);
}
}
return cflags; return cflags;
} }
@ -1095,7 +1097,8 @@ size_t num_channel_htlcs(const struct channel *channel)
return n; return n;
} }
static bool adjust_balance(struct channel *channel, struct htlc *htlc) static bool adjust_balance(struct balance view_owed[NUM_SIDES][NUM_SIDES],
struct htlc *htlc)
{ {
enum side side; enum side side;
@ -1105,24 +1108,8 @@ static bool adjust_balance(struct channel *channel, struct htlc *htlc)
continue; continue;
/* Add it. */ /* Add it. */
if (!balance_add_htlc(&channel->view[side].owed[LOCAL], balance_add_htlc(&view_owed[side][LOCAL], htlc, LOCAL);
htlc, LOCAL)) { balance_add_htlc(&view_owed[side][REMOTE], htlc, REMOTE);
status_broken("Cannot add htlc #%"PRIu64" %s"
" to LOCAL",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
return false;
}
if (!balance_add_htlc(&channel->view[side].owed[REMOTE],
htlc, REMOTE)) {
status_broken("Cannot add htlc #%"PRIu64" %s"
" to REMOTE",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
return false;
}
/* If it is no longer committed, remove it (depending /* If it is no longer committed, remove it (depending
* on fail || fulfill). */ * on fail || fulfill). */
@ -1138,58 +1125,8 @@ static bool adjust_balance(struct channel *channel, struct htlc *htlc)
htlc_state_name(htlc->state)); htlc_state_name(htlc->state));
return false; return false;
} }
if (!balance_remove_htlc(&channel->view[side].owed[LOCAL], balance_remove_htlc(&view_owed[side][LOCAL], htlc, LOCAL);
htlc, LOCAL)) { balance_remove_htlc(&view_owed[side][REMOTE], htlc, REMOTE);
status_broken("Cannot remove htlc #%"PRIu64" %s"
" from LOCAL",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
return false;
}
if (!balance_remove_htlc(&channel->view[side].owed[REMOTE],
htlc, REMOTE)) {
status_broken("Cannot remove htlc #%"PRIu64" %s"
" from REMOTE",
htlc->id,
type_to_string(tmpctx, struct amount_msat,
&htlc->amount));
return false;
}
}
return true;
}
static bool offset_balances(struct channel *channel)
{
for (enum side view = LOCAL; view < NUM_SIDES; view++) {
for (enum side side = LOCAL; side < NUM_SIDES; side++) {
struct amount_msat *a = &channel->view[view].owed[side];
if (amount_msat_add(a, *a, AMOUNT_MSAT((u64)1 << 63)))
continue;
status_broken("Can't offset %s",
type_to_string(tmpctx, struct amount_msat,
a));
return false;
}
}
return true;
}
static bool unoffset_balances(struct channel *channel)
{
for (enum side view = LOCAL; view < NUM_SIDES; view++) {
for (enum side side = LOCAL; side < NUM_SIDES; side++) {
struct amount_msat *a = &channel->view[view].owed[side];
if (amount_msat_sub(a, *a, AMOUNT_MSAT((u64)1 << 63)))
continue;
status_broken("Can't unoffset %s",
type_to_string(tmpctx, struct amount_msat,
a));
return false;
}
} }
return true; return true;
} }
@ -1206,6 +1143,7 @@ bool channel_force_htlcs(struct channel *channel,
size_t i; size_t i;
struct htlc *htlc; struct htlc *htlc;
struct htlc_map_iter it; struct htlc_map_iter it;
struct balance view_owed[NUM_SIDES][NUM_SIDES];
if (tal_count(hstates) != tal_count(htlcs)) { if (tal_count(hstates) != tal_count(htlcs)) {
status_broken("#hstates %zu != #htlcs %zu", status_broken("#hstates %zu != #htlcs %zu",
@ -1347,21 +1285,40 @@ bool channel_force_htlcs(struct channel *channel,
htlc->failed_scid = NULL; htlc->failed_scid = NULL;
} }
/* Add giant offset so we never go negative here. */
if (!offset_balances(channel))
return false;
/* You'd think, since we traverse HTLCs in ID order, this would never /* You'd think, since we traverse HTLCs in ID order, this would never
* go negative. But this ignores the fact that HTLCs ids from each * go negative. But this ignores the fact that HTLCs ids from each
* side have no correlation with each other. */ * side have no correlation with each other. Copy into struct balance,
* to allow transient underflow. */
for (int view = 0; view < NUM_SIDES; view++) {
for (int side = 0; side < NUM_SIDES; side++) {
to_balance(&view_owed[view][side],
channel->view[view].owed[side]);
}
}
for (htlc = htlc_map_first(channel->htlcs, &it); for (htlc = htlc_map_first(channel->htlcs, &it);
htlc; htlc;
htlc = htlc_map_next(channel->htlcs, &it)) { htlc = htlc_map_next(channel->htlcs, &it)) {
if (!adjust_balance(channel, htlc)) if (!adjust_balance(view_owed, htlc))
return false; return false;
} }
return unoffset_balances(channel); /* Convert back and check */
for (int view = 0; view < NUM_SIDES; view++) {
for (int side = 0; side < NUM_SIDES; side++) {
if (!balance_ok(&view_owed[view][side],
&channel->view[view].owed[side])) {
status_broken("view %s[%s] balance underflow:"
" %"PRId64,
side_to_str(view),
side_to_str(side),
view_owed[view][side].msat);
return false;
}
}
}
return true;
} }
const char *channel_add_err_name(enum channel_add_err e) const char *channel_add_err_name(enum channel_add_err e)

Write Preview
Loading…
Cancel
Save