Do complete fee tracking. #3329
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Aiming to fix #3253 |
rustyrussell
force-pushed
the
guilt/fee-mess
branch
2 times, most recently
from
fa723fe
to
42f30b7
Compare
rustyrussell
changed the title
rustyrussell
added
Optech Make Me Famous!
This uses the same state machine as HTLCs, but they're only ever added, not removed. Since we can only have one in each state, we use a simple array; mostly NULL. We could make this more space-efficient by folding everything into the first 5 states, but that would be more complex than just using the identical state machine. One subtlety: we don't send uncommitted fee_states over the wire. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And also move it to initial_channel, so we can use it there. This saves churn in the next patch. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…mitment. The `channel_got_commitsig` we send the lightningd also implies we sent the revoke_and_ack, as an optimization. It doesn't currently matter, since channel_sending_revoke_and_ack doesn't do anything important to the state, but that changes once we start uploading the entire fee_states. So now we move our state machine *before* sending to lightningd, in preparation for sending fee_states too. Unfortunately, we need to marshall the info to send before we increment the state, as lightningd expects that. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is an intermediary step: we still don't save it to the database, but we do use the fee_states struct to track it internally. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These used to be necessary as we could have feerate changes which we couldn't track: now we do, we don't need these flags. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/fee-mess
branch
2 times, most recently
from
e5fc39a
to
7be6bbe
Compare
The upgrade here is a bit tricky: we map the two values into the feerate_state. This is trivial if they're both the same, but if they're different we don't know exactly what state they're in (this being the source of the bug!). So, we assume that the have received the update and not acked it, as that would be the normal case. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the final step: we pass the complete fee_states to and from channeld. Changelog-Fixed: "Bad commitment signature" closing channels when we sent back-to-back update_fee messages across multiple reconnects. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The norm for channels is a 1% reserve. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/fee-mess
branch
from
7be6bbe
to
740bb27
Compare
|
Rebased on master (db conflict), plugged dumb memleak in test, added Changelog. |
cdecker
approved these changes
Dec 12, 2019
Comment on lines
+49
to
+52
| for (size_t i = 0; i < ARRAY_SIZE(n->feerate); i++) { | ||
| if (n->feerate[i]) | ||
| n->feerate[i] = tal_dup(n, u32, n->feerate[i]); | ||
| } |
There was a problem hiding this comment.
This seems like a no-op to me, it's assigning a copy of the array member to itself, or am I missing something?
There was a problem hiding this comment.
It should probably be n->feerate[i] = tal_dup(n, u32, fee_states->feerate[i]);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Long ago, our implementation used to track fee changes just like HTLCs: first they apply to the recipient, then when they're acked they apply back to the sender. However, HTLCs have other states which fee-changes don't have (in particular, HTLCs get removed, whereas fee changes just overwrite one another), and at the Milan summit we decided only the funder could set fees.
So c-lightning simply kept two fee amounts, one for each side, and it seemed to work.
Unfortunately, there were corner cases where it was wrong, see 9f175de and 552e56d and it still had corner cases.
With some reluctance, this goes back to our first, full-bodied approach. There is a danger in transition, however since we have to guess what the state was if the fees were different on each side, but it's a once-off (and small) risk.
It's also worth noting that the second patch ("channeld: use fee_states internally") breaks some tests; since we don't save and restore the full state from the database here, we end up in the wrong state over some restarts. I didn't bother disabling the tests in that commit and re-enabling them once we put them into the db.