Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Process fee updates as any other update message #2397

Conversation

Projects
None yet
3 participants
@halseth
Copy link
Collaborator

halseth commented Jan 2, 2019

Instead of special casing the UpdateFee messages, we instead add them to
the update logs like any other HTLC update message. This lets us avoid
having to keep an extra set of variables to keep track of the fee
updates, and instead reuse the commit/ack logic used for other updates.

This fixes a bug where we would reset the pendingFeeUpdate variable
after signing our next commitment, which would make us calculate the new
fee incorrectly if the remote sent a commitment concurrently.

A test to exercise the previous failure case is added.

Fixes #2348
Fixes #1908

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Jan 2, 2019

Possibly also fixes #1908

@Roasbeef
Copy link
Member

Roasbeef left a comment

Solid set of changes! We should've implemented it like this from the very start, the resulting logic is also much easier to follow as there're no longer any special cases for fee updates.

@@ -421,6 +421,11 @@ func PayDescsFromRemoteLogUpdates(chanID lnwire.ShortChannelID, height uint64,
Index: uint16(i),
},
}

// NOTE: UpdateFee is not expected since they are not forwarded.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 3, 2019

Member

Return an error instead of panicing perhaps?

This comment has been minimized.

Copy link
@halseth

halseth Jan 4, 2019

Author Collaborator

Done!

Show resolved Hide resolved lnwallet/channel.go Outdated
theirBalance *lnwire.MilliSatoshi, nextHeight uint64,
remoteChain, mutateState bool) *htlcView {
theirBalance *lnwire.MilliSatoshi, nextHeight uint64, remoteChain,
mutateState bool, feePerKw SatPerKWeight) (*htlcView, SatPerKWeight) {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 3, 2019

Member

An alternative to returning an extra parameter would be to add a feePerKw field directly to the htlcView. This seems more natural as the fee rate itself is a part of the view as it results from evaluating the log.

This comment has been minimized.

Copy link
@halseth

halseth Jan 4, 2019

Author Collaborator

Yes, that's much better!

@halseth halseth force-pushed the halseth:reject-commitment-expected-fee-reset-bug branch 2 times, most recently from 71ce90b to 5e97cbb Jan 4, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 8, 2019

New set of tests failures with the latest rebase.

@Roasbeef
Copy link
Member

Roasbeef left a comment

It looks like the fee is being accounted for twice causing us to overdraw the amount in channel in the case of a potential close.

// is denominated in msat we won't lose precision when storing the
// sat/kw denominated feerate.
case *lnwire.UpdateFee:
pd = &PaymentDescriptor{

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 8, 2019

Member

addCommitHeightRemote isn't set. We need to properly record this in order to handle the retransmission case.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 8, 2019

Member
--- FAIL: TestChannelRetransmissionFeeUpdate (0.13s)
    channel_test.go:3998: unable to complete bob's state transition: rejected commitment: commit_height=2, invalid_commit_sig=30440220682b6dd35e569da6cbe26568e5388bae4c76a9ed61162087bc77471c4558ae9102207a8dad8ffd4a8297c8f50463c2c8a1177ded151f2ea003ee8bedf83f20bdfc1b, commit_tx=0200000001b794385f2d1ef7ab4d9273d1906381b44f2f6f2588a3efb96a491883319847530000000000d1af1c8003204e0000000000002200204e62432fb93d883e0910926ed09e70556f2ca6d62fa74096d016fc389ec55cdbe016cd1d00000000220020d7ba6761e1ac17cbb0722c5818bb61d312ea660cf3505f0eeef4b39f81a2d4330065cd1d000000001600144e8187664a78afec5b8c6991dbc52d575ef5e27116539e20, sig_hash=9ffec9eac9de4aaeb680da3c95bb650468f48a2b57f7ef7eac81caa5bd6863ee

This comment has been minimized.

Copy link
@halseth

halseth Jan 8, 2019

Author Collaborator

You're right. Test failure was due to 516a6a7, which is fixed in 821cb23

// Initiate feePerKw to the last committed fee for this chain as we'll
// need this to determine which HTLCs are dust, and also the final fee
// rate.
view.feePerKw = commitChain.tip().feePerKw

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 8, 2019

Member

This seems incorrect. We only know the final fee rate for this view after we fully evaluate it (as we do below).

This comment has been minimized.

Copy link
@halseth

halseth Jan 8, 2019

Author Collaborator

Yeah, reverted this, as it was the culprit of the test failures

Show resolved Hide resolved lnwallet/channel.go

@halseth halseth force-pushed the halseth:reject-commitment-expected-fee-reset-bug branch from 5e97cbb to 821cb23 Jan 8, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Jan 8, 2019

One of the fixup commits introduced the travis test failure. Should be fixed now. Reminder to self: check travis before squashing commits!

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

Very clean PR! Gave this a thorough pass and LGTM. Made sure to double check that these changes are backwards compatible with the forwarding packages, which I believe they are! Stoked to have a fix in for this bug 🎉

@@ -1999,6 +1999,88 @@ func TestUpdateFeeFail(t *testing.T) {

}

// TestUpdateFeeConcurrentSig tests that the channel can properly handle a fee
// update that it receives concurrently with signing its next commitment.
func TestUpdateFeeConcurrentSig(t *testing.T) {

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Collaborator

Solid test!


// For fee updates we'll create a FeeUpdate type to add to the log. We
// reuse the amount field to hold the fee rate. Since the amount field
// is denominated in msat we won't lose precision when storing the

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Collaborator

I was wondering this myself until I looked it up :)

@@ -1571,7 +1571,8 @@ func (lc *LightningChannel) logUpdateToPayDesc(logUpdate *channeldb.LogUpdate,
Amount: lnwire.NewMSatFromSatoshis(
btcutil.Amount(wireMsg.FeePerKw),
),
EntryType: FeeUpdate,
EntryType: FeeUpdate,
removeCommitHeightRemote: commitHeight,

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 10, 2019

Member

Shouldn't this be add?

This comment has been minimized.

Copy link
@halseth

halseth Jan 10, 2019

Author Collaborator

It should indeed.... Fixed!

This comment has been minimized.

Copy link
@halseth

halseth Jan 10, 2019

Author Collaborator

Thninking a bit more about this, maybe it should really be remove.. To make the feeUpdates being removed when compacting logs, then the remove height must be set. lmk what you think.

This comment has been minimized.

Copy link
@halseth

halseth Jan 10, 2019

Author Collaborator

Since FeeUpdates doesn't really have the same property of getting added and removed at different heights, I decided to just set both fields at the same time.

When compacting logs we'll now properly remove it if the remove height has passed: 277949c

Also added a test to check that it gets removed during log compacting: e6ee835

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 10, 2019

Member

Cool yeah it's slightly different since it doesn't have the same add/remove semantics as the others. However, I think we can emulate that based on the two add heights. Nice was going to recommend we extend the tests to ensure that the log is empty after fully processing a fee update, will check out those extra commits!

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 10, 2019

Collaborator

Yeah I thought about this as well, but it makes sense since we want the fee update to be "insta-removed" in a sense, plus there's so secondary message to cleanup the feeupdate add. might it be correct to set both to the same height?

@halseth halseth force-pushed the halseth:reject-commitment-expected-fee-reset-bug branch from cb48abf to 233597c Jan 10, 2019

lnwallet/channel: add new PaymentDescriptor type `FeeUpdate`
This commit adds a new updateType that can be used for
PaymentDescriptors: FeeUpdate. We repurpose the fields of the existing
PaymentDescriptor struct such that we can easily re-use the commit/ack
logic used for other update types also for fee updates.

@halseth halseth force-pushed the halseth:reject-commitment-expected-fee-reset-bug branch from 233597c to 567a41f Jan 10, 2019

halseth added some commits Jan 10, 2019

lnwallet/channel: add lnwire<->PaymentDescriptor FeeUpdate conversion
This commit adds conversion between the lnwire.UpdateFee message and the
new FeeUpdate PaymentDescriptor. We re-purpose the existing Amount field
in the PaymentDescriptor stuct to hold the feerate.
lnwallet/channel: remove FeeUpdates when compacting logs
When compacting the update logs we remove any fee updates when they
remove height is passed. We do this since we'll assume fee updates are
added and removed at the same commit height, as they will apply for all
commitments following the fee update.

@halseth halseth force-pushed the halseth:reject-commitment-expected-fee-reset-bug branch from 567a41f to 3e59d39 Jan 10, 2019

halseth added some commits Jan 10, 2019

lnwallet/channel: process FeeUpdate found in update log
This commit makes the evaluateHTLCView method process any found
FeeUpdates in the logs, by returning the last set feerate.
lnwallet/channel: add fee updates to update logs
Instead of special casing the UpdateFee messages, we instead add them to
the update logs like any other HTLC update message. This lets us avoid
having to keep an extra set of variables to keep track of the fee
updates, and instead reuse the commit/ack logic used for other updates.

This fixes a bug where we would reset the pendingFeeUpdate variable
after signing our next commitment, which would make us calculate the new
fee incorrectly if the remote sent a commitment concurrently.

When restoring state logs, we also make sure to re-add any fee updates.
lnwallet/channel test: add TestUpdateFeeConcurrentSig
This tests make sure we don't reset our expected fee upate after signing
our next commitment. This test would fail without the previous set of
commits.
lnwallet/channel: don't return feePerKw from computeView
Since the feerate is part of the computed view now, we don't have to
pass the found feerate as a distinct parameter.

@halseth halseth force-pushed the halseth:reject-commitment-expected-fee-reset-bug branch from 3e59d39 to e6ee835 Jan 10, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🐉

Moved onto testing the diff as is on the faucet as well now.

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

LGTM 🎩

@@ -1229,6 +1232,16 @@ func compactLogs(ourLog, theirLog *updateLog,
if remoteChainTail >= htlc.removeCommitHeightRemote &&
localChainTail >= htlc.removeCommitHeightLocal {

// Fee updates have no parent htlcs, so we only

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 12, 2019

Collaborator

👍

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 15, 2019

Faucet testing went off without a hitch, ready to land!

@Roasbeef Roasbeef merged commit 9c59ac4 into lightningnetwork:master Jan 15, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.003%) to 56.273%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.