From 6fac3438ddfb7a47cddf758739558dd3d5bd9dd3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Nov 2017 15:56:58 +1030 Subject: [PATCH] channeld: track feerates. Handling feerates for the fundee (who only receives fee_update) is simple: it's practically atomic since we accept commitment and send revocation, thus they're applied to both sides at once. Handling feerates for the funder is more complex: in theory we could have multiple in flight. However, if we avoid this using the same logic as we use to suppress multiple commitments in flight, it's simple again. We fix the test code to use real feerate manipulation, thus have to remove an assert about feerate being non-zero. And now we have feechanges, we need to rely on the changes_pending flags, as we can have changes without an HTLCs changing. Signed-off-by: Rusty Russell --- channeld/full_channel.c | 63 +++++++++++++++++--------------- channeld/test/run-full_channel.c | 35 ++++++++++++++++-- 2 files changed, 66 insertions(+), 32 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index eeee198bf..6c405b9ad 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -406,8 +406,6 @@ static enum channel_add_err add_htlc(struct channel *channel, u64 dust = dust_limit_satoshis(channel, recipient); size_t untrimmed; - assert(feerate >= 1); - assert(dust >= 1); untrimmed = commit_tx_num_untrimmed(committed, feerate, dust, recipient) + commit_tx_num_untrimmed(adding, feerate, dust, @@ -743,7 +741,11 @@ bool channel_update_feerate(struct channel *channel, u32 feerate_per_kw) if (!can_funder_afford_feerate(channel, feerate_per_kw)) return false; + status_trace("Setting %s feerate to %u", + side_to_str(!channel->funder), feerate_per_kw); + channel->view[!channel->funder].feerate_per_kw = feerate_per_kw; + channel->changes_pending[!channel->funder] = true; return true; } @@ -752,11 +754,9 @@ u32 channel_feerate(const struct channel *channel, enum side side) return channel->view[side].feerate_per_kw; } -/* FIXME: Handle fee changes too. */ bool channel_sending_commit(struct channel *channel, const struct htlc ***htlcs) { - int change; const enum htlc_state states[] = { SENT_ADD_HTLC, SENT_REMOVE_REVOCATION, SENT_ADD_REVOCATION, @@ -769,9 +769,8 @@ bool channel_sending_commit(struct channel *channel, return false; } - change = change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states), - htlcs, "sending_commit"); - assert(change & HTLC_REMOTE_F_COMMITTED); + change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states), + htlcs, "sending_commit"); channel->changes_pending[REMOTE] = false; assert(!channel->awaiting_revoke_and_ack); @@ -799,13 +798,23 @@ bool channel_rcvd_revoke_and_ack(struct channel *channel, assert(channel->awaiting_revoke_and_ack); channel->awaiting_revoke_and_ack = false; - return change & HTLC_LOCAL_F_PENDING; + /* For funder, ack also means time to apply new feerate locally. */ + if (channel->funder == LOCAL && + (channel->view[LOCAL].feerate_per_kw + != channel->view[REMOTE].feerate_per_kw)) { + status_trace("Applying feerate %u to LOCAL", + channel->view[REMOTE].feerate_per_kw); + channel->view[LOCAL].feerate_per_kw + = channel->view[REMOTE].feerate_per_kw; + channel->changes_pending[LOCAL] = true; + } + + return channel->changes_pending[LOCAL]; } /* FIXME: We can actually merge these two... */ bool channel_rcvd_commit(struct channel *channel, const struct htlc ***htlcs) { - int change; const enum htlc_state states[] = { RCVD_ADD_REVOCATION, RCVD_REMOVE_HTLC, RCVD_ADD_HTLC, @@ -818,13 +827,11 @@ bool channel_rcvd_commit(struct channel *channel, const struct htlc ***htlcs) return false; } - change = change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states), htlcs, - "rcvd_commit"); + change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states), htlcs, + "rcvd_commit"); - assert(change & HTLC_LOCAL_F_COMMITTED); channel->changes_pending[LOCAL] = false; - - return change & HTLC_LOCAL_F_COMMITTED; + return true; } bool channel_sending_revoke_and_ack(struct channel *channel) @@ -842,7 +849,18 @@ bool channel_sending_revoke_and_ack(struct channel *channel) if (change & HTLC_REMOTE_F_PENDING) channel->changes_pending[REMOTE] = true; - return change & HTLC_REMOTE_F_PENDING; + /* For non-funder, sending ack means we apply any fund changes to them */ + if (channel->funder == REMOTE + && (channel->view[LOCAL].feerate_per_kw + != channel->view[REMOTE].feerate_per_kw)) { + status_trace("Applying feerate %u to REMOTE", + channel->view[LOCAL].feerate_per_kw); + channel->view[REMOTE].feerate_per_kw + = channel->view[LOCAL].feerate_per_kw; + channel->changes_pending[REMOTE] = true; + } + + return channel->changes_pending[REMOTE]; } static bool htlc_awaiting_revoke_and_ack(const struct htlc *h) @@ -862,20 +880,7 @@ static bool htlc_awaiting_revoke_and_ack(const struct htlc *h) bool channel_awaiting_revoke_and_ack(const struct channel *channel) { - struct htlc_map_iter it; - struct htlc *h; - - /* FIXME: remove debugging iteration. */ - for (h = htlc_map_first(channel->htlcs, &it); - h; - h = htlc_map_next(channel->htlcs, &it)) { - if (htlc_awaiting_revoke_and_ack(h)) { - assert(channel->awaiting_revoke_and_ack); - return true; - } - } - assert(!channel->awaiting_revoke_and_ack); - return false; + return channel->awaiting_revoke_and_ack; } bool channel_has_htlcs(const struct channel *channel) diff --git a/channeld/test/run-full_channel.c b/channeld/test/run-full_channel.c index c767be737..8a0616e57 100644 --- a/channeld/test/run-full_channel.c +++ b/channeld/test/run-full_channel.c @@ -298,6 +298,35 @@ static void send_and_fulfill_htlc(struct channel *channel, } } +static void update_feerate(struct channel *channel, u32 feerate) +{ + bool ret; + + ret = channel_update_feerate(channel, feerate); + assert(ret); + if (channel->funder == LOCAL) { + ret = channel_sending_commit(channel, NULL); + assert(ret); + ret = channel_rcvd_revoke_and_ack(channel, NULL); + assert(ret); + ret = channel_rcvd_commit(channel, NULL); + assert(ret); + ret = channel_sending_revoke_and_ack(channel); + assert(!ret); + } else { + ret = channel_rcvd_commit(channel, NULL); + assert(ret); + ret = channel_sending_revoke_and_ack(channel); + assert(ret); + ret = channel_sending_commit(channel, NULL); + assert(ret); + ret = channel_rcvd_revoke_and_ack(channel, NULL); + assert(!ret); + } + assert(channel_feerate(channel, LOCAL) == feerate); + assert(channel_feerate(channel, REMOTE) == feerate); +} + int main(void) { tal_t *tmpctx = tal_tmpctx(NULL); @@ -513,9 +542,9 @@ int main(void) rchannel, &local_per_commitment_point, 42, REMOTE); txs_must_be_eq(txs, txs2); - /* FIXME: Adjust properly! */ - lchannel->view[LOCAL].feerate_per_kw = feerate_per_kw[LOCAL]; - rchannel->view[REMOTE].feerate_per_kw = feerate_per_kw[REMOTE]; + update_feerate(lchannel, feerate_per_kw[LOCAL]); + update_feerate(rchannel, feerate_per_kw[REMOTE]); + htlcs = include_htlcs(lchannel, LOCAL); include_htlcs(rchannel, REMOTE);