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.

Sign up for GitHub

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

lnwallet: limit received htlc's to MaxAcceptedHTLCs #3910

Merged
merged 3 commits into from
Feb 19, 2020
Merged

lnwallet: limit received htlc's to MaxAcceptedHTLCs #3910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56c07a5
lnwallet: call valdiateCommitmentSanity in ReceiveHTLC
Crypt-iQ Feb 15, 2020
4af00c6
lnwallet: fixing unit tests to properly handle new receive validation
Crypt-iQ Feb 15, 2020
5a5e095
lnwallet: adding TestMaxAsynchronousHtlcs unit test
Crypt-iQ Feb 15, 2020
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
25 changes: 20 additions & 5 deletions lnwallet/channel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3084,12 +3084,16 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
view := lc.fetchHTLCView(theirLogCounter, ourLogCounter)

// If we are checking if we can add a new HTLC, we add this to the
// update log, in order to validate the sanity of the commitment
// resulting from _actually adding_ this HTLC to the state.
// appropriate update log, in order to validate the sanity of the
// commitment resulting from _actually adding_ this HTLC to the state.
if predictAdded != nil {
// If we are adding an HTLC, this will be an Add to the local
// update log.
view.ourUpdates = append(view.ourUpdates, predictAdded)
// If the remoteChain bool is true, add to ourUpdates.
if remoteChain {
Roasbeef marked this conversation as resolved.
Show resolved Hide resolved
view.ourUpdates = append(view.ourUpdates, predictAdded)
} else {
// Else add to theirUpdates.
view.theirUpdates = append(view.theirUpdates, predictAdded)
}
}

commitChain := lc.localCommitChain
Expand Down Expand Up @@ -4640,6 +4644,17 @@ func (lc *LightningChannel) ReceiveHTLC(htlc *lnwire.UpdateAddHTLC) (uint64, err
OnionBlob: htlc.OnionBlob[:],
}

localACKedIndex := lc.remoteCommitChain.tail().ourMessageIndex

// Clamp down on the number of HTLC's we can receive by checking the
// commitment sanity.
err := lc.validateCommitmentSanity(
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, nice I think this is it. If I'm not mistaken this will also handle the CASE 2 you outlined earlier, since it will apply the settles/fails accordingly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup it applies the same logic that the sender applies so it handles case 2

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this early validation violate the protocol? For example if we'd receive and settle an htlc in the same batch with no more commitment slot left? If validating only for the whole batch, that would still be possible perhaps?

Copy link
Collaborator Author

@Crypt-iQ Crypt-iQ Feb 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not entirely sure I follow - the prior behavior was to do no validation and wait until a commitment was received to do the validation. This just makes the validation earlier so there shouldn't be a problem. It also mimics lnd's sender-side logic (to validate upon calling AddHTLC) so if this is a protocol violation, then that logic would be too.

I think it wouldn't be a protocol violation since it will only add an additional HTLC if their next signature for us must have a settle/fail that would remove one HTLC to make room for this new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the question is indeed if it is a protocol violation. If the commitment is full, should we allow another htlc to be added? Because before the signature comes, a settle may still be added.

Copy link
Collaborator Author

@Crypt-iQ Crypt-iQ Feb 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So there are two places in the spec I can see where this is elaborated on:

if result would be offering more than the remote's max_accepted_htlcs HTLCs, in the remote commitment transaction:

    MUST NOT add an HTLC.

and

if a sending node adds more than receiver max_accepted_htlcs HTLCs to its local commitment transaction [...]:

    SHOULD fail the channel.

So they only seem concerned about the effect on the commitment transaction. So I think this won't violate the spec since ReceiveHTLC just goes into the update logs. Plus it's only applied to the commit tx later because there is a settle/fail that ensures only the max can be applies to the commit tx.

Copy link
Contributor

Choose a reason for hiding this comment

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

It still feels a little inconclusive. But as the second quote is from the description of the add message, I indeed think that validation should take place already then and not be postponed until the signature arrives.

Copy link
Collaborator Author

@Crypt-iQ Crypt-iQ Feb 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this worth bringing up / making an issue in the spec repo? I don't see the receive logic being the problem, but rather the sending logic. If we think a send in this async case is ok but our peer thinks it's a violation (esp w/ diff implementations), then I'd see a problem as we may be disconnected. All in all though, the receive should match the send logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth bringing up as a spec issue I think, although I think it is pretty obvious that each received message would have to lead to a valid commitment transaction. Otherwise how many updates would you allow while waiting fo a settel to open up the commitment. But agree that this could be more clearly spelled out in the spec.

lc.remoteUpdateLog.logIndex, localACKedIndex, false, pd,
)
if err != nil {
return 0, err
}

lc.remoteUpdateLog.appendHtlc(pd)

return pd.HtlcIndex, nil
Expand Down
260 changes: 224 additions & 36 deletions lnwallet/channel_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5462,23 +5462,26 @@ func TestMaxAcceptedHTLCs(t *testing.T) {
defer cleanUp()

// One over the maximum number of HTLCs that either can accept.
const numHTLCs = 20
const numHTLCsReceived = 12
const numHTLCs = 12

// Set the remote's required MaxAcceptedHtlcs. This means that alice
// Set the remote's required MaxAcceptedHtlcs. This means that Alice
// can only offer the remote up to numHTLCs HTLCs.
aliceChannel.channelState.LocalChanCfg.MaxAcceptedHtlcs = numHTLCs
bobChannel.channelState.RemoteChanCfg.MaxAcceptedHtlcs = numHTLCs

// Similarly, set the remote config's MaxAcceptedHtlcs. This means
// that the remote will be aware that Alice will only accept up to
// numHTLCsRecevied at a time.
aliceChannel.channelState.RemoteChanCfg.MaxAcceptedHtlcs = numHTLCsReceived
bobChannel.channelState.LocalChanCfg.MaxAcceptedHtlcs = numHTLCsReceived
// that the remote will be aware that Bob will only accept up to
// numHTLCs at a time.
aliceChannel.channelState.RemoteChanCfg.MaxAcceptedHtlcs = numHTLCs
bobChannel.channelState.LocalChanCfg.MaxAcceptedHtlcs = numHTLCs

// Each HTLC amount is 0.1 BTC.
htlcAmt := lnwire.NewMSatFromSatoshis(0.1 * btcutil.SatoshiPerBitcoin)

// htlcID is used to keep track of the HTLC that Bob will fail back to
// Alice.
var htlcID uint64

// Send the maximum allowed number of HTLCs.
for i := 0; i < numHTLCs; i++ {
htlc, _ := createHTLC(i, htlcAmt)
Expand All @@ -5488,6 +5491,13 @@ func TestMaxAcceptedHTLCs(t *testing.T) {
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}

// Just assign htlcID to the last received HTLC.
htlcID = htlc.ID
}

if err := ForceStateTransition(aliceChannel, bobChannel); err != nil {
t.Fatalf("unable to transition state: %v", err)
}

// The next HTLC should fail with ErrMaxHTLCNumber.
Expand All @@ -5497,15 +5507,211 @@ func TestMaxAcceptedHTLCs(t *testing.T) {
t.Fatalf("expected ErrMaxHTLCNumber, instead received: %v", err)
}

// After receiving the next HTLC, next state transition should fail
// with ErrMaxHTLCNumber.
// Receiving the next HTLC should fail.
if _, err := bobChannel.ReceiveHTLC(htlc); err != ErrMaxHTLCNumber {
t.Fatalf("expected ErrMaxHTLCNumber, instead received: %v", err)
}

// Bob will fail the htlc specified by htlcID and then force a state
// transition.
err = bobChannel.FailHTLC(htlcID, []byte{}, nil, nil, nil)
if err != nil {
t.Fatalf("unable to fail htlc: %v", err)
}

if err := aliceChannel.ReceiveFailHTLC(htlcID, []byte{}); err != nil {
t.Fatalf("unable to receive fail htlc: %v", err)
}

if err := ForceStateTransition(bobChannel, aliceChannel); err != nil {
t.Fatalf("unable to transition state: %v", err)
}

// Bob should succeed in adding a new HTLC since a previous HTLC was just
// failed. We use numHTLCs here since the previous AddHTLC with this index
// failed.
htlc, _ = createHTLC(numHTLCs, htlcAmt)
if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil {
t.Fatalf("unable to add htlc: %v", err)
}
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}
err = ForceStateTransition(aliceChannel, bobChannel)
if err != ErrMaxHTLCNumber {

// Add a commitment to Bob's commitment chain.
aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment()
if err != nil {
t.Fatalf("unable to sign next commitment: %v", err)
}
err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
if err != nil {
t.Fatalf("unable to recv new commitment: %v", err)
}

// The next HTLC should fail with ErrMaxHTLCNumber. The index is incremented
// by one.
htlc, _ = createHTLC(numHTLCs+1, htlcAmt)
if _, err = aliceChannel.AddHTLC(htlc, nil); err != ErrMaxHTLCNumber {
t.Fatalf("expected ErrMaxHTLCNumber, instead received: %v", err)
}

// Likewise, Bob should not be able to receive this HTLC if Alice can't
// add it.
if _, err := bobChannel.ReceiveHTLC(htlc); err != ErrMaxHTLCNumber {
t.Fatalf("expected ErrMaxHTLCNumber, instead received: %v", err)
}
}

// TestMaxAsynchronousHtlcs tests that Bob correctly receives (and does not
// fail) an HTLC from Alice when exchanging asynchronous payments. We want to
// mimic the following case where Bob's commitment transaction is full before
// starting:
// Alice Bob
// 1. <---settle/fail---
// 2. <-------sig-------
// 3. --------sig------> (covers an add sent before step 1)
// 4. <-------rev-------
// 5. --------rev------>
// 6. --------add------>
// 7. - - - - sig - - ->
// This represents an asynchronous commitment dance in which both sides are
// sending signatures at the same time. In step 3, the signature does not
// cover the recent settle/fail that Bob sent in step 1. However, the add that
// Alice sends to Bob in step 6 does not overflow Bob's commitment transaction.
// This is because validateCommitmentSanity counts the HTLC's by ignoring
// HTLC's which will be removed in the next signature that Alice sends. Thus,
// the add won't overflow. This is because the signature received in step 7
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this contradict with the conclusion in #3910 (review) ?

Copy link
Member

Choose a reason for hiding this comment

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

In the end, we can only really execute these heuristics on a best effort basis. There're other classes of the concurrent send case that aren't covered in this test/scenario. At the end of the day, the real defense point is at the sender, in that they should be careful to not send items that would result in a force close.

// covers the settle/fail in step 1 and makes space for the add in step 6.
func TestMaxAsynchronousHtlcs(t *testing.T) {
t.Parallel()

// We'll kick off the test by creating our channels which both are
// loaded with 5 BTC each.
aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(true)
if err != nil {
t.Fatalf("unable to create test channels: %v", err)
}
defer cleanUp()

// One over the maximum number of HTLCs that either can accept.
const numHTLCs = 12

// Set the remote's required MaxAcceptedHtlcs. This means that Alice
// can only offer the remote up to numHTLCs HTLCs.
aliceChannel.channelState.LocalChanCfg.MaxAcceptedHtlcs = numHTLCs
bobChannel.channelState.RemoteChanCfg.MaxAcceptedHtlcs = numHTLCs

// Similarly, set the remote config's MaxAcceptedHtlcs. This means
// that the remote will be aware that Bob will only accept up to
// numHTLCs at a time.
aliceChannel.channelState.RemoteChanCfg.MaxAcceptedHtlcs = numHTLCs
bobChannel.channelState.LocalChanCfg.MaxAcceptedHtlcs = numHTLCs

// Each HTLC amount is 0.1 BTC.
htlcAmt := lnwire.NewMSatFromSatoshis(0.1 * btcutil.SatoshiPerBitcoin)

var htlcID uint64

// Send the maximum allowed number of HTLCs minus one.
for i := 0; i < numHTLCs-1; i++ {
htlc, _ := createHTLC(i, htlcAmt)
if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil {
t.Fatalf("unable to add htlc: %v", err)
}
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}

// Just assign htlcID to the last received HTLC.
htlcID = htlc.ID
}

if err := ForceStateTransition(aliceChannel, bobChannel); err != nil {
t.Fatalf("unable to transition state: %v", err)
}

// Send an HTLC to Bob so that Bob's commitment transaction is full.
htlc, _ := createHTLC(numHTLCs-1, htlcAmt)
if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil {
t.Fatalf("unable to add htlc: %v", err)
}
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}

// Fail back an HTLC and sign a commitment as in steps 1 & 2.
err = bobChannel.FailHTLC(htlcID, []byte{}, nil, nil, nil)
if err != nil {
t.Fatalf("unable to fail htlc: %v", err)
}

if err := aliceChannel.ReceiveFailHTLC(htlcID, []byte{}); err != nil {
t.Fatalf("unable to receive fail htlc: %v", err)
}

bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment()
if err != nil {
t.Fatalf("unable to sign next commitment: %v", err)
}

err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
if err != nil {
t.Fatalf("unable to receive new commitment: %v", err)
}

// Cover the HTLC referenced with id equal to numHTLCs-1 with a new
// signature (step 3).
aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment()
if err != nil {
t.Fatalf("unable to sign next commitment: %v", err)
}

err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
if err != nil {
t.Fatalf("unable to receive new commitment: %v", err)
}

// Both sides exchange revocations as in step 4 & 5.
bobRevocation, _, err := bobChannel.RevokeCurrentCommitment()
if err != nil {
t.Fatalf("unable to revoke revocation: %v", err)
}

_, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation)
if err != nil {
t.Fatalf("unable to receive revocation: %v", err)
}

aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment()
if err != nil {
t.Fatalf("unable to revoke revocation: %v", err)
}

_, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation)
if err != nil {
t.Fatalf("unable to receive revocation: %v", err)
}

// Send the final Add which should succeed as in step 6.
htlc, _ = createHTLC(numHTLCs, htlcAmt)
if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil {
t.Fatalf("unable to add htlc: %v", err)
}
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}

// Receiving the commitment should succeed as in step 7 since space was
// made.
aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment()
if err != nil {
t.Fatalf("unable to sign next commitment: %v", err)
}

err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
if err != nil {
t.Fatalf("unable to receive new commitment: %v", err)
}
}

// TestMaxPendingAmount tests that the maximum overall pending HTLC value is met
Expand Down Expand Up @@ -5556,13 +5762,8 @@ func TestMaxPendingAmount(t *testing.T) {
t.Fatalf("expected ErrMaxPendingAmount, instead received: %v", err)
}

// And also Bob shouldn't be accepting this HTLC in the next state
// transition.
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}
err = ForceStateTransition(aliceChannel, bobChannel)
if err != ErrMaxPendingAmount {
// And also Bob shouldn't be accepting this HTLC upon calling ReceiveHTLC.
if _, err := bobChannel.ReceiveHTLC(htlc); err != ErrMaxPendingAmount {
t.Fatalf("expected ErrMaxPendingAmount, instead received: %v", err)
}
}
Expand Down Expand Up @@ -5684,12 +5885,8 @@ func TestChanReserve(t *testing.T) {
t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err)
}

// Alice will reject this htlc when a state transition is attempted.
if _, err := aliceChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}
err = ForceStateTransition(aliceChannel, bobChannel)
if err != ErrBelowChanReserve {
// Alice will reject this htlc upon receiving the htlc.
if _, err := aliceChannel.ReceiveHTLC(htlc); err != ErrBelowChanReserve {
t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err)
}

Expand Down Expand Up @@ -5731,13 +5928,8 @@ func TestChanReserve(t *testing.T) {
t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err)
}

// Likewise, Bob will reject a state transition after this htlc is
// received, of the same reason.
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("unable to recv htlc: %v", err)
}
err = ForceStateTransition(aliceChannel, bobChannel)
if err != ErrBelowChanReserve {
// Likewise, Bob will reject receiving the htlc because of the same reason.
if _, err := bobChannel.ReceiveHTLC(htlc); err != ErrBelowChanReserve {
t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err)
}

Expand Down Expand Up @@ -5857,13 +6049,9 @@ func TestMinHTLC(t *testing.T) {
t.Fatalf("expected ErrBelowMinHTLC, instead received: %v", err)
}

// Bob will receive this HTLC, but reject the next state update, since
// Bob will receive this HTLC, but reject the next received htlc, since
// the htlc is too small.
_, err = bobChannel.ReceiveHTLC(htlc)
if err != nil {
t.Fatalf("error receiving htlc: %v", err)
}
err = ForceStateTransition(aliceChannel, bobChannel)
if err != ErrBelowMinHTLC {
t.Fatalf("expected ErrBelowMinHTLC, instead received: %v", err)
}
Expand Down