From 241c79397f1ebbf8da2533d9adcb7b9425930a94 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 22 Mar 2018 12:52:12 +0100 Subject: [PATCH 1/4] lnwallet/channel: fix crash on receiving too few HTLC sigs This commit fixes an out of bounds error that would occur in the case where we received a new commitment where the accompanying HTLC sigs were too few. Now we'll just reject such an commitment. A test exercising the behavior is also added. --- lnwallet/channel.go | 12 +++++++ lnwallet/channel_test.go | 71 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index bed2aa990..496425e6d 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3532,6 +3532,12 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment, return sigHash, nil } + // Make sure there are more signatures left. + if i >= len(htlcSigs) { + return nil, fmt.Errorf("not enough HTLC " + + "signatures.") + } + // With the sighash generated, we'll also store the // signature so it can be written to disk if this state // is valid. @@ -3578,6 +3584,12 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment, return sigHash, nil } + // Make sure there are more signatures left. + if i >= len(htlcSigs) { + return nil, fmt.Errorf("not enough HTLC " + + "signatures.") + } + // With the sighash generated, we'll also store the // signature so it can be written to disk if this state // is valid. diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 2f8ccad10..cd56e0e8a 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1461,6 +1461,77 @@ func TestHTLCDustLimit(t *testing.T) { } } +// TestHTLCSigNumber tests that a received commitment is only accepted if it +// comes with the exact number of valid HTLC signatures. +func TestHTLCSigNumber(t *testing.T) { + t.Parallel() + + // createChanWithHTLC is a helper method that sets ut two channels, and + // adds HTLCs with the passed values to the channels. + createChanWithHTLC := func(htlcValues ...btcutil.Amount) ( + *LightningChannel, *LightningChannel, func()) { + + // Create a test channel funded evenly with Alice having 5 BTC, + // and Bob having 5 BTC. Alice's dustlimit is 200 sat, while + // Bob has 1300 sat. + aliceChannel, bobChannel, cleanUp, err := createTestChannels(3) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + + for i, htlcSat := range htlcValues { + htlcMsat := lnwire.NewMSatFromSatoshis(htlcSat) + htlc, _ := createHTLC(i, htlcMsat) + _, err := aliceChannel.AddHTLC(htlc, nil) + if err != nil { + t.Fatalf("alice unable to add htlc: %v", err) + } + _, err = bobChannel.ReceiveHTLC(htlc) + if err != nil { + t.Fatalf("bob unable to receive htlc: %v", err) + } + } + + return aliceChannel, bobChannel, cleanUp + } + + // Calculate two values that will be below and above Bob's dust limit. + estimator := &StaticFeeEstimator{24} + feePerVSize, err := estimator.EstimateFeePerVSize(1) + if err != nil { + t.Fatalf("unable to get fee: %v", err) + } + feePerKw := feePerVSize.FeePerKWeight() + + aboveDust := btcutil.Amount(1400) + htlcSuccessFee(feePerKw) + + // =================================================================== + // Test that Bob will reject a commitment if Alice doesn't send enough + // HTLC signatures. + // =================================================================== + aliceChannel, bobChannel, cleanUp := createChanWithHTLC(aboveDust, + aboveDust) + defer cleanUp() + + aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("Error signing next commitment: %v", err) + } + + if len(aliceHtlcSigs) != 2 { + t.Fatalf("expected 2 htlc sig, instead got %v", + len(aliceHtlcSigs)) + } + + // Now discard one signature from the htlcSig slice. Bob should reject + // the commitment because of this. + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs[1:]) + if err == nil { + t.Fatalf("Expected Bob to reject signatures") + } + +} + // TestChannelBalanceDustLimit tests the condition when the remaining balance // for one of the channel participants is so small as to be considered dust. In // this case, the output for that participant is removed and all funds (minus From 263d6b9c1fbeb67319dda5dee806c47c46534ea5 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 22 Mar 2018 12:59:11 +0100 Subject: [PATCH 2/4] lnwallet/channel: don't accept immediately on empty htlc sigs This commit fixes an issue where we would blindly accept a commitment which came without any accompanying HTLC signatures. A test exercising the scenario is added. --- lnwallet/channel.go | 6 ------ lnwallet/channel_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 496425e6d..eb0adeaf7 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3465,12 +3465,6 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment, keyRing *CommitmentKeyRing, htlcSigs []lnwire.Sig, localChanCfg, remoteChanCfg *channeldb.ChannelConfig) ([]verifyJob, error) { - // If this new commitment state doesn't have any HTLC's that are to be - // signed, then we'll return a nil slice. - if len(htlcSigs) == 0 { - return nil, nil - } - txHash := localCommitmentView.txn.TxHash() feePerKw := localCommitmentView.feePerKw diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index cd56e0e8a..01e158d8f 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1530,6 +1530,30 @@ func TestHTLCSigNumber(t *testing.T) { t.Fatalf("Expected Bob to reject signatures") } + // =================================================================== + // Test that Bob will reject a commitment if Alice doesn't send any + // HTLC signatures. + // =================================================================== + aliceChannel, bobChannel, cleanUp = createChanWithHTLC(aboveDust) + defer cleanUp() + + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("Error signing next commitment: %v", err) + } + + if len(aliceHtlcSigs) != 1 { + t.Fatalf("expected 1 htlc sig, instead got %v", + len(aliceHtlcSigs)) + } + + // Now just give Bob an empty htlcSig slice. He should reject the + // commitment because of this. + err = bobChannel.ReceiveNewCommitment(aliceSig, []lnwire.Sig{}) + if err == nil { + t.Fatalf("Expected Bob to reject signatures") + } + } // TestChannelBalanceDustLimit tests the condition when the remaining balance From 70b86e596e4c54813a02b31437f3aedbf0355924 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 22 Mar 2018 13:04:57 +0100 Subject: [PATCH 3/4] lnwallet/channel: use remote dustlimit when generating HTLC sigs This commit fixes an issue which would arise in some cases when the local and remote dust limits would differ, resulting in lnd not producing the expected number of HTLC signatures. This was a result of checking dust against the local instead of the remote dust limit. A test exercising the scenario is added. --- lnwallet/channel.go | 2 +- lnwallet/channel_test.go | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index eb0adeaf7..4f69ed2a1 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2540,7 +2540,7 @@ func genRemoteHtlcSigJobs(keyRing *CommitmentKeyRing, remoteCommitView *commitment) ([]signJob, chan struct{}, error) { txHash := remoteCommitView.txn.TxHash() - dustLimit := localChanCfg.DustLimit + dustLimit := remoteChanCfg.DustLimit feePerKw := remoteCommitView.feePerKw // With the keys generated, we'll make a slice with enough capacity to diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 01e158d8f..2046ed3ad 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1503,6 +1503,7 @@ func TestHTLCSigNumber(t *testing.T) { } feePerKw := feePerVSize.FeePerKWeight() + belowDust := btcutil.Amount(500) + htlcTimeoutFee(feePerKw) aboveDust := btcutil.Amount(1400) + htlcSuccessFee(feePerKw) // =================================================================== @@ -1554,6 +1555,52 @@ func TestHTLCSigNumber(t *testing.T) { t.Fatalf("Expected Bob to reject signatures") } + // ============================================================== + // Test that sigs are not returned for HTLCs below dust limit. + // ============================================================== + aliceChannel, bobChannel, cleanUp = createChanWithHTLC(belowDust) + defer cleanUp() + + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("Error signing next commitment: %v", err) + } + + // Since the HTLC is below Bob's dust limit, Alice won't need to send + // any signatures for this HTLC. + if len(aliceHtlcSigs) != 0 { + t.Fatalf("expected no htlc sigs, instead got %v", + len(aliceHtlcSigs)) + } + + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err != nil { + t.Fatalf("Bob failed receiving commitment: %v", err) + } + + // ================================================================ + // Test that sigs are correctly returned for HTLCs above dust limit. + // ================================================================ + aliceChannel, bobChannel, cleanUp = createChanWithHTLC(aboveDust) + defer cleanUp() + + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("Error signing next commitment: %v", err) + } + + // Since the HTLC is above Bob's dust limit, Alice should send a + // signature for this HTLC. + if len(aliceHtlcSigs) != 1 { + t.Fatalf("expected 1 htlc sig, instead got %v", + len(aliceHtlcSigs)) + } + + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err != nil { + t.Fatalf("Bob failed receiving commitment: %v", err) + } + } // TestChannelBalanceDustLimit tests the condition when the remaining balance From a6e7dce7b7047f55ef0d7356649ee3f352091c9d Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 22 Mar 2018 13:23:23 +0100 Subject: [PATCH 4/4] lnwallet/channel: reject received commitment with too many htlc sigs This commit adds a check that will make LightningChannel reject a received commitment if it is accompanied with too many HTLC signatures. This enforces the requirement in BOLT-2, saying: if num_htlcs is not equal to the number of HTLC outputs in the local commitment transaction: * MUST fail the channel. A test exercising the behaviour is added. --- lnwallet/channel.go | 7 +++++++ lnwallet/channel_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 4f69ed2a1..cefc3753e 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3606,6 +3606,13 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment, i++ } + // If we received a number of HTLC signatures that doesn't match our + // commitment, we'll return an error now. + if len(htlcSigs) != i { + return nil, fmt.Errorf("number of htlc sig mismatch. "+ + "Expected %v sigs, got %v", i, len(htlcSigs)) + } + return verifyJobs, nil } diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 2046ed3ad..345affb58 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1601,6 +1601,35 @@ func TestHTLCSigNumber(t *testing.T) { t.Fatalf("Bob failed receiving commitment: %v", err) } + // ==================================================================== + // Test that Bob will not validate a received commitment if Alice sends + // signatures for HTLCs below the dust limit. + // ==================================================================== + aliceChannel, bobChannel, cleanUp = createChanWithHTLC(belowDust, + aboveDust) + defer cleanUp() + + // Alice should produce only one signature, since one HTLC is below + // dust. + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("Error signing next commitment: %v", err) + } + + if len(aliceHtlcSigs) != 1 { + t.Fatalf("expected 1 htlc sig, instead got %v", + len(aliceHtlcSigs)) + } + + // Add an extra signature. + aliceHtlcSigs = append(aliceHtlcSigs, aliceHtlcSigs[0]) + + // Bob should reject these signatures since they don't match the number + // of HTLCs above dust. + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err == nil { + t.Fatalf("Expected Bob to reject signatures") + } } // TestChannelBalanceDustLimit tests the condition when the remaining balance