From ff37b711c6dea62f6acb9d0aaca36417edcda820 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 20 Sep 2019 10:55:20 +0200 Subject: [PATCH 1/4] funding: dont's send ErrorCode on wire Since the ErrorCodes are not part of the spec, they cannot be read by other implementations. Instead of only sending the error code we therefore send the complete error message. This will have the same effect at the client, as it will just get the full error instead of the code indicating which error it is. It will also be compatible with other impls. Note that the GRPC error codes will change, since we don't set them anymore. --- fundingmanager.go | 29 +++++++---------------------- lntest/itest/lnd_test.go | 5 +++-- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index 855df1b89..34b0a8f16 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -26,7 +26,6 @@ import ( "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/routing" "golang.org/x/crypto/salsa20" - "google.golang.org/grpc" ) const ( @@ -862,15 +861,14 @@ func (f *fundingManager) failFundingFlow(peer lnpeer.Peer, tempChanID [32]byte, var msg lnwire.ErrorData switch e := fundingErr.(type) { - // Let the actual error message be sent to the remote. + // Let the actual error message be sent to the remote for the + // whitelisted types. case lnwallet.ReservationError: msg = lnwire.ErrorData(e.Error()) - - // Send the status code. case lnwire.ErrorCode: - msg = lnwire.ErrorData{byte(e)} + msg = lnwire.ErrorData(e.Error()) - // We just send a generic error. + // For all other error types we just send a generic error. default: msg = lnwire.ErrorData("funding failed due to internal error") } @@ -2963,25 +2961,12 @@ func (f *fundingManager) handleErrorMsg(fmsg *fundingErrorMsg) { // If we did indeed find the funding workflow, then we'll return the // error back to the caller (if any), and cancel the workflow itself. - lnErr := lnwire.ErrorCode(protocolErr.Data[0]) - fndgLog.Errorf("Received funding error from %x: %v", + fundingErr := fmt.Errorf("received funding error from %x: %v", fmsg.peerKey.SerializeCompressed(), string(protocolErr.Data), ) + fndgLog.Errorf(fundingErr.Error()) - // If this isn't a simple error code, then we'll display the entire - // thing. - if len(protocolErr.Data) > 1 { - err = grpc.Errorf( - lnErr.ToGrpcCode(), string(protocolErr.Data), - ) - } else { - // Otherwise, we'll attempt to display just the error code - // itself. - err = grpc.Errorf( - lnErr.ToGrpcCode(), lnErr.String(), - ) - } - resCtx.err <- err + resCtx.err <- fundingErr } // pruneZombieReservations loops through all pending reservations and fails the diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index fbc0ea1e9..e0760423f 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -42,7 +42,6 @@ import ( "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/routing" "golang.org/x/net/context" - "google.golang.org/grpc" ) var ( @@ -6160,7 +6159,9 @@ func testMaxPendingChannels(net *lntest.NetworkHarness, t *harnessTest) { if err == nil { t.Fatalf("error wasn't received") - } else if grpc.Code(err) != lnwire.ErrMaxPendingChannels.ToGrpcCode() { + } else if !strings.Contains( + err.Error(), lnwire.ErrMaxPendingChannels.Error(), + ) { t.Fatalf("not expected error was received: %v", err) } From 949f6c6cec4dafb67c9ad5c302df03c2c8d2d158 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 20 Sep 2019 10:55:20 +0200 Subject: [PATCH 2/4] lnwire: remove ErrorCode encoding/decoding Never sent on the wire. --- lnwire/lnwire.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/lnwire/lnwire.go b/lnwire/lnwire.go index 28dca7dff..e1f6a7dd4 100644 --- a/lnwire/lnwire.go +++ b/lnwire/lnwire.go @@ -117,12 +117,6 @@ func WriteElement(w io.Writer, element interface{}) error { if _, err := w.Write(b[:]); err != nil { return err } - case ErrorCode: - var b [2]byte - binary.BigEndian.PutUint16(b[:], uint16(e)) - if _, err := w.Write(b[:]); err != nil { - return err - } case MilliSatoshi: var b [8]byte binary.BigEndian.PutUint64(b[:], uint64(e)) @@ -506,12 +500,6 @@ func ReadElement(r io.Reader, element interface{}) error { return err } *e = ChanUpdateChanFlags(b[0]) - case *ErrorCode: - var b [2]byte - if _, err := io.ReadFull(r, b[:]); err != nil { - return err - } - *e = ErrorCode(binary.BigEndian.Uint16(b[:])) case *uint32: var b [4]byte if _, err := io.ReadFull(r, b[:]); err != nil { From 33fe09482b70db2f09f5680ee1ef46641e139ecf Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 20 Sep 2019 10:55:21 +0200 Subject: [PATCH 3/4] lnwire+multi: define Error() for lnwire.Error To make lnwire.Error actually satisfy the error interface, define the Error method directly. --- fundingmanager.go | 4 ++-- fundingmanager_test.go | 26 +++++++++++++------------- htlcswitch/link.go | 19 +------------------ lnwire/error.go | 24 ++++++++++++++++++++++++ peer.go | 2 +- 5 files changed, 41 insertions(+), 34 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index 34b0a8f16..04c41f090 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -2955,14 +2955,14 @@ func (f *fundingManager) handleErrorMsg(fmsg *fundingErrorMsg) { resCtx, err := f.cancelReservationCtx(fmsg.peerKey, chanID) if err != nil { fndgLog.Warnf("Received error for non-existent funding "+ - "flow: %v (%v)", err, spew.Sdump(protocolErr)) + "flow: %v (%v)", err, protocolErr.Error()) return } // If we did indeed find the funding workflow, then we'll return the // error back to the caller (if any), and cancel the workflow itself. fundingErr := fmt.Errorf("received funding error from %x: %v", - fmsg.peerKey.SerializeCompressed(), string(protocolErr.Data), + fmsg.peerKey.SerializeCompressed(), protocolErr.Error(), ) fndgLog.Errorf(fundingErr.Error()) diff --git a/fundingmanager_test.go b/fundingmanager_test.go index e53ed045c..4fb31ee88 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -602,7 +602,7 @@ func fundChannel(t *testing.T, alice, bob *testNode, localFundingAmt, if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -728,7 +728,7 @@ func assertFundingMsgSent(t *testing.T, msgChan chan lnwire.Message, errorMsg, gotError := msg.(*lnwire.Error) if gotError { t.Fatalf("expected %s to be sent, instead got error: %v", - msgType, lnwire.ErrorCode(errorMsg.Data[0])) + msgType, errorMsg.Error()) } _, _, line, _ := runtime.Caller(1) @@ -1469,7 +1469,7 @@ func TestFundingManagerPeerTimeoutAfterInitFunding(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -1531,7 +1531,7 @@ func TestFundingManagerPeerTimeoutAfterFundingOpen(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -1602,7 +1602,7 @@ func TestFundingManagerPeerTimeoutAfterFundingAccept(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -2326,7 +2326,7 @@ func TestFundingManagerCustomChannelParameters(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -2561,7 +2561,7 @@ func TestFundingManagerMaxPendingChannels(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -2725,7 +2725,7 @@ func TestFundingManagerRejectPush(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -2736,9 +2736,9 @@ func TestFundingManagerRejectPush(t *testing.T) { // Assert Bob responded with an ErrNonZeroPushAmount error. err := assertFundingMsgSent(t, bob.msgChan, "Error").(*lnwire.Error) - if "Non-zero push amounts are disabled" != string(err.Data) { + if !strings.Contains(err.Error(), "Non-zero push amounts are disabled") { t.Fatalf("expected ErrNonZeroPushAmount error, got \"%v\"", - string(err.Data)) + err.Error()) } } @@ -2782,7 +2782,7 @@ func TestFundingManagerMaxConfs(t *testing.T) { if gotError { t.Fatalf("expected OpenChannel to be sent "+ "from bob, instead got error: %v", - lnwire.ErrorCode(errorMsg.Data[0])) + errorMsg.Error()) } t.Fatalf("expected OpenChannel to be sent from "+ "alice, instead got %T", aliceMsg) @@ -2805,9 +2805,9 @@ func TestFundingManagerMaxConfs(t *testing.T) { // Alice should respond back with an error indicating MinAcceptDepth is // too large. err := assertFundingMsgSent(t, alice.msgChan, "Error").(*lnwire.Error) - if !strings.Contains(string(err.Data), "minimum depth") { + if !strings.Contains(err.Error(), "minimum depth") { t.Fatalf("expected ErrNumConfsTooLarge, got \"%v\"", - string(err.Data)) + err.Error()) } } diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 64e6dd000..c4603d4b7 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1882,13 +1882,9 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { // Error received from remote, MUST fail channel, but should // only print the contents of the error message if all // characters are printable ASCII. - errMsg := "non-ascii data" - if isASCII(msg.Data) { - errMsg = string(msg.Data) - } l.fail(LinkFailureError{code: ErrRemoteError}, "ChannelPoint(%v): received error from peer: %v", - l.channel.ChannelPoint(), errMsg) + l.channel.ChannelPoint(), msg.Error()) default: log.Warnf("ChannelPoint(%v): received unknown message of type %T", l.channel.ChannelPoint(), msg) @@ -3100,16 +3096,3 @@ func (l *channelLink) tracef(format string, a ...interface{}) { msg := fmt.Sprintf(format, a...) log.Tracef("ChannelLink(%s) %s", l.ShortChanID(), msg) } - -// isASCII is a helper method that checks whether all bytes in `data` would be -// printable ASCII characters if interpreted as a string. -func isASCII(data []byte) bool { - isASCII := true - for _, c := range data { - if c < 32 || c > 126 { - isASCII = false - break - } - } - return isASCII -} diff --git a/lnwire/error.go b/lnwire/error.go index 5a4dd1bfd..c4159e7ab 100644 --- a/lnwire/error.go +++ b/lnwire/error.go @@ -1,6 +1,7 @@ package lnwire import ( + "fmt" "io" "google.golang.org/grpc/codes" @@ -87,6 +88,18 @@ func NewError() *Error { // interface. var _ Message = (*Error)(nil) +// Error returns the string representation to Error. +// +// NOTE: Satisfies the error interface. +func (c *Error) Error() string { + errMsg := "non-ascii data" + if isASCII(c.Data) { + errMsg = string(c.Data) + } + + return fmt.Sprintf("chan_id=%v, err=%v", c.ChanID, errMsg) +} + // Decode deserializes a serialized Error message stored in the passed // io.Reader observing the specified protocol version. // @@ -125,3 +138,14 @@ func (c *Error) MaxPayloadLength(uint32) uint32 { // 32 + 2 + 65501 return 65535 } + +// isASCII is a helper method that checks whether all bytes in `data` would be +// printable ASCII characters if interpreted as a string. +func isASCII(data []byte) bool { + for _, c := range data { + if c < 32 || c > 126 { + return false + } + } + return true +} diff --git a/peer.go b/peer.go index 2b4460a4d..247daab7d 100644 --- a/peer.go +++ b/peer.go @@ -1250,7 +1250,7 @@ func messageSummary(msg lnwire.Message) string { msg.ChanID, msg.ID, msg.FailureCode) case *lnwire.Error: - return fmt.Sprintf("chan_id=%v, err=%v", msg.ChanID, string(msg.Data)) + return fmt.Sprintf("%v", msg.Error()) case *lnwire.AnnounceSignatures: return fmt.Sprintf("chan_id=%v, short_chan_id=%v", msg.ChannelID, From e4301d3a8f5499eb100c58e814cd2e8bd7d4a45e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 20 Sep 2019 10:55:21 +0200 Subject: [PATCH 4/4] lnwire: rename ErrorCode -> FundingError To make it clear that these errors are not part of the spec, rename them to FundingError. --- fundingmanager.go | 4 ++-- lnwire/error.go | 34 +++++++++++----------------------- 2 files changed, 13 insertions(+), 25 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index 04c41f090..db159fae3 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -857,7 +857,7 @@ func (f *fundingManager) failFundingFlow(peer lnpeer.Peer, tempChanID [32]byte, } // We only send the exact error if it is part of out whitelisted set of - // errors (lnwire.ErrorCode or lnwallet.ReservationError). + // errors (lnwire.FundingError or lnwallet.ReservationError). var msg lnwire.ErrorData switch e := fundingErr.(type) { @@ -865,7 +865,7 @@ func (f *fundingManager) failFundingFlow(peer lnpeer.Peer, tempChanID [32]byte, // whitelisted types. case lnwallet.ReservationError: msg = lnwire.ErrorData(e.Error()) - case lnwire.ErrorCode: + case lnwire.FundingError: msg = lnwire.ErrorData(e.Error()) // For all other error types we just send a generic error. diff --git a/lnwire/error.go b/lnwire/error.go index c4159e7ab..c9fa39a8a 100644 --- a/lnwire/error.go +++ b/lnwire/error.go @@ -3,40 +3,30 @@ package lnwire import ( "fmt" "io" - - "google.golang.org/grpc/codes" ) -// ErrorCode represents the short error code for each of the defined errors -// within the Lightning Network protocol spec. -type ErrorCode uint8 - -// ToGrpcCode is used to generate gRPC specific code which will be propagated -// to the ln rpc client. This code is used to have more detailed view of what -// goes wrong and also in order to have the ability pragmatically determine the -// error and take specific actions on the client side. -func (e ErrorCode) ToGrpcCode() codes.Code { - return (codes.Code)(e) + 100 -} +// FundingError represents a set of errors that can be encountered and sent +// during the funding workflow. +type FundingError uint8 const ( // ErrMaxPendingChannels is returned by remote peer when the number of // active pending channels exceeds their maximum policy limit. - ErrMaxPendingChannels ErrorCode = 1 + ErrMaxPendingChannels FundingError = 1 // ErrSynchronizingChain is returned by a remote peer that receives a // channel update or a funding request while their still syncing to the // latest state of the blockchain. - ErrSynchronizingChain ErrorCode = 2 + ErrSynchronizingChain FundingError = 2 // ErrChanTooLarge is returned by a remote peer that receives a // FundingOpen request for a channel that is above their current // soft-limit. - ErrChanTooLarge ErrorCode = 3 + ErrChanTooLarge FundingError = 3 ) -// String returns a human readable version of the target ErrorCode. -func (e ErrorCode) String() string { +// String returns a human readable version of the target FundingError. +func (e FundingError) String() string { switch e { case ErrMaxPendingChannels: return "Number of pending channels exceed maximum" @@ -49,10 +39,10 @@ func (e ErrorCode) String() string { } } -// Error returns the human redable version of the target ErrorCode. +// Error returns the human redable version of the target FundingError. // -// Satisfies the Error interface. -func (e ErrorCode) Error() string { +// NOTE: Satisfies the Error interface. +func (e FundingError) Error() string { return e.String() } @@ -66,8 +56,6 @@ type ErrorData []byte // format is purposefully general in order to allow expression of a wide array // of possible errors. Each Error message is directed at a particular open // channel referenced by ChannelPoint. -// -// TODO(roasbeef): remove the error code type Error struct { // ChanID references the active channel in which the error occurred // within. If the ChanID is all zeros, then this error applies to the