diff --git a/cmd/commands/commands.go b/cmd/commands/commands.go index e2726bb4fc6..f105cf322bd 100644 --- a/cmd/commands/commands.go +++ b/cmd/commands/commands.go @@ -271,7 +271,7 @@ func actionDecorator(f func(*cli.Context) error) func(*cli.Context) error { // 'unlock') but the wallet is already unlocked, then these // methods aren't recognized any more because this service is // shut down after successful unlock. - if s.Code() == codes.Unknown && strings.Contains( + if s.Code() == codes.FailedPrecondition && strings.Contains( s.Message(), rpcperms.ErrWalletUnlocked.Error(), ) && (c.Command.Name == "create" || c.Command.Name == "unlock" || @@ -283,7 +283,7 @@ func actionDecorator(f func(*cli.Context) error) func(*cli.Context) error { // lnd might be active, but not possible to contact using RPC if // the wallet is encrypted. - if s.Code() == codes.Unknown && strings.Contains( + if s.Code() == codes.FailedPrecondition && strings.Contains( s.Message(), rpcperms.ErrWalletLocked.Error(), ) { diff --git a/docs/release-notes/release-notes-0.22.0.md b/docs/release-notes/release-notes-0.22.0.md index 260c099f1f2..552d06ffec3 100644 --- a/docs/release-notes/release-notes-0.22.0.md +++ b/docs/release-notes/release-notes-0.22.0.md @@ -77,6 +77,14 @@ ## RPC Updates +* The RPC state interceptor now [returns structured gRPC status + codes](https://github.com/lightningnetwork/lnd/pull/10945) instead of plain + Go errors. `ErrWaitingToStart` and `ErrRPCStarting` map to + `codes.Unavailable`; `ErrNoWallet`, `ErrWalletLocked`, and + `ErrWalletUnlocked` map to `codes.FailedPrecondition`. Callers can now + branch on the status code directly rather than performing string matching + against the error message. + ## lncli Updates ## Breaking Changes diff --git a/rpcperms/interceptor.go b/rpcperms/interceptor.go index d9c9e6f0e9e..ca1d86a56b3 100644 --- a/rpcperms/interceptor.go +++ b/rpcperms/interceptor.go @@ -878,14 +878,16 @@ func (r *InterceptorChain) checkRPCState(srv interface{}) error { // Do not accept any RPC calls (unless to the state service) until LND // has not started. case waitingToStart: - return ErrWaitingToStart + return status.Error(codes.Unavailable, ErrWaitingToStart.Error()) // If the wallet does not exists, only calls to the WalletUnlocker are // accepted. case walletNotCreated: _, ok := srv.(lnrpc.WalletUnlockerServer) if !ok { - return ErrNoWallet + return status.Error( + codes.FailedPrecondition, ErrNoWallet.Error(), + ) } // If the wallet is locked, only calls to the WalletUnlocker are @@ -893,28 +895,38 @@ func (r *InterceptorChain) checkRPCState(srv interface{}) error { case walletLocked: _, ok := srv.(lnrpc.WalletUnlockerServer) if !ok { - return ErrWalletLocked + return status.Error( + codes.FailedPrecondition, ErrWalletLocked.Error(), + ) } // If the wallet is unlocked, but the RPC not yet active, we reject. case walletUnlocked: _, ok := srv.(lnrpc.WalletUnlockerServer) if ok { - return ErrWalletUnlocked + return status.Error( + codes.FailedPrecondition, + ErrWalletUnlocked.Error(), + ) } - return ErrRPCStarting + return status.Error(codes.Unavailable, ErrRPCStarting.Error()) // If the RPC server or lnd server is active, we allow calls to any // service except the WalletUnlocker. case rpcActive, serverActive: _, ok := srv.(lnrpc.WalletUnlockerServer) if ok { - return ErrWalletUnlocked + return status.Error( + codes.FailedPrecondition, + ErrWalletUnlocked.Error(), + ) } default: - return fmt.Errorf("unknown RPC state: %v", state) + return status.Errorf( + codes.Unknown, "unknown RPC state: %v", state, + ) } return nil diff --git a/rpcperms/interceptor_test.go b/rpcperms/interceptor_test.go index 1c014f6fbd6..d68becc5cf3 100644 --- a/rpcperms/interceptor_test.go +++ b/rpcperms/interceptor_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/btcsuite/btclog/v2" + "github.com/lightningnetwork/lnd/lnrpc" "github.com/stretchr/testify/require" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -111,6 +112,113 @@ func (s *recordingServerStream) SendMsg(any) error { return nil } +// mockUnlockerServer satisfies lnrpc.WalletUnlockerServer so it can be used +// as the server argument in checkRPCState tests. +type mockUnlockerServer struct { + lnrpc.UnimplementedWalletUnlockerServer +} + +// mockOtherServer is a server that does NOT implement WalletUnlockerServer. +type mockOtherServer struct{} + +// TestCheckRPCStateGRPCCodes verifies that checkRPCState returns errors with +// the correct gRPC status codes so callers can branch on codes rather than +// performing brittle string matching. +func TestCheckRPCStateGRPCCodes(t *testing.T) { + t.Parallel() + + unlocker := &mockUnlockerServer{} + other := &mockOtherServer{} + + chain := &InterceptorChain{} + + cases := []struct { + name string + state rpcState + srv interface{} + wantCode codes.Code + wantErrNil bool + }{ + { + name: "waitingToStart returns Unavailable", + state: waitingToStart, + srv: other, + wantCode: codes.Unavailable, + }, + { + name: "walletNotCreated non-unlocker returns FailedPrecondition", + state: walletNotCreated, + srv: other, + wantCode: codes.FailedPrecondition, + }, + { + name: "walletNotCreated unlocker returns nil", + state: walletNotCreated, + srv: unlocker, + wantErrNil: true, + }, + { + name: "walletLocked non-unlocker returns FailedPrecondition", + state: walletLocked, + srv: other, + wantCode: codes.FailedPrecondition, + }, + { + name: "walletLocked unlocker returns nil", + state: walletLocked, + srv: unlocker, + wantErrNil: true, + }, + { + name: "walletUnlocked unlocker returns FailedPrecondition", + state: walletUnlocked, + srv: unlocker, + wantCode: codes.FailedPrecondition, + }, + { + name: "walletUnlocked non-unlocker returns Unavailable", + state: walletUnlocked, + srv: other, + wantCode: codes.Unavailable, + }, + { + name: "rpcActive unlocker returns FailedPrecondition", + state: rpcActive, + srv: unlocker, + wantCode: codes.FailedPrecondition, + }, + { + name: "rpcActive non-unlocker returns nil", + state: rpcActive, + srv: other, + wantErrNil: true, + }, + { + name: "serverActive non-unlocker returns nil", + state: serverActive, + srv: other, + wantErrNil: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + chain.state = tc.state + err := chain.checkRPCState(tc.srv) + + if tc.wantErrNil { + require.NoError(t, err) + return + } + + require.Error(t, err) + require.Equal(t, tc.wantCode, status.Code(err)) + }) + } +} + // TestTruncatePanicStack asserts that panic stack traces are capped with a // readable truncation marker. func TestTruncatePanicStack(t *testing.T) {