Returns structured errors from FundPsbt #5436

arshbot · 2021-06-25T21:15:36Z

Converts errors likely to be thrown from string based errors to error
classes with returned grpc error codes. Addresses #5411

Pull Request Checklist

Converts errors likely to be thrown from string based errors to error classes with returned grpc error codes

joostjager · 2021-06-27T14:17:41Z

lnrpc/walletrpc/psbt.go

@@ -36,8 +38,7 @@ func verifyInputsUnspent(inputs []*wire.TxIn, utxos []*lnwallet.Utxo) error {
 		}

 		if !found {
-			return fmt.Errorf("input %d not found in list of non-"+
-				"locked UTXO", idx)
+			return status.Error(codes.NotFound, ErrUnspentInputNotFound(idx).Error())


I don't think this will return a structured error to the client and they still need to parse the text message to find out the index?

I guess it depends what you mean by a structured error. On the client side, they can introspect the error and extract the error code used and act upon then, possibly doing string parsing if they need any additional context.

Unless you mean return a response that has fields to enumerate the different types of errors?

Did some digging in the docs, and looks like it's possible for us to actually get the best of both worlds here: https://pkg.go.dev/google.golang.org/grpc/internal/status?utm_source=godoc#Status.WithDetails

This API lets you make a status code to return using status.Error, but then also attach an arbitrary proto message that can be used to let the client optionally get more structured information if it needs to. This blog post has a good overview of how things would work end to end: https://jbrandhorst.com/post/grpc-errors/

Yes, WithDetails is what I used before indeed for that. I think that can come in useful in various places, because not everything maps cleanly to a grpc code. And in this case, the index needs to be stored somewhere. If an utxo is locked, it is likely that a client needs info about which one it is exactly to do another funding attempt.

Should the response provide key value information per error type (if index not found, return missing index via key) or should we go with more generic error codes with arbitrary information? The issue of having systems respond appropriately to failures is addressed with the generic error codes.

If think the requirement here is that systems should be able to respond appropriately to a specific utxo being unavailable. Just a generic error code wouldn't suffice then.

I didn't know about the WithDetails, could be useful indeed. Though the fact that it needs to be a proto message would mean we'd need to add new messages to our main proto file just for structured errors? Might get bloated quite quickly.

Also, I wonder how such an error message (with details) would look on the command line, if the error is just printed?

A problem to consider would also be the scope of structured errors, and the lack of unity between different error cases. FundPSBT has many unique error cases, while only 2 in particular are covered by this ticket.

@guggero the error should be printed in json to the client imo, as the purpose is for machine consumption.

There may be many unique error cases, but all the 'unexpected' ones - the internal errors - don't need to be returned in a structured way I'd say. I am not sure if that many structured errors remain.

Using status.WithDetails, we attach more metadata which is then printed as json in certain cases for machine consumption. Added the errdetails package to provide more robust error coding

This commit compiles the grpc protos since additional output is added to the FundPSBT command

Adds a test to ensure errors are thrown when utxos with incorrect indices are provided to FundPSBT.

guggero · 2021-08-31T14:48:13Z

I think the idea of starting to return more useful information with errors is a really good one!

There are a few things I personally don't like about the current approach:

We need a new error type/struct for each specific case.
We rely on the googleapis messages which might not fit our use cases very well.
The client needs to to a type switch to interpret the error details.
We use a custom error message in the detail that just annotates another reason string. This could be done through a custom error code.

But I think the ideas in this PR are good and should be taken into account.
So here is my counter proposal:

We unify this PR with rpcperms: add gRPC codes to errors #5633 which proposes to use the gRPC status code to encode more information about where an error comes from, similar to HTTP status codes.
- I think we should use 3 digits to encode the DOMAIN, for example:
  - 100: General server error
  - 101: Wallet error
  - 102: Validation error
  - 103: Funding error
  - 104: PSBT error
  - xxx: Define as needed, up to 999 domains possible.
- Then we can use two more digits to encode the more specific CODE, which can be defined per domain. For example the PSBT domain could have the codes:
  - 00: Unspent input not found
  - 01: Funding error
  - yy: Define as needed, up to 99 codes per domain possible.
- With this, the two errors described in this PR would get the full codes 10400 and 10401 respectively.
To transport additional details in a structured manner, we add a simple lnrpc.ErrDetails message which is a map<string, string> details = 1; gRPC type. With that we could encode the index of the first error as details["index"] = fmt.Sprintf("%d", idx). That would still require the client to match the string name of the field, but that name should remain much more stable.

What do you think, @arshbot, @joostjager ?

alexbosworth · 2021-08-31T21:04:42Z

2. To transport additional details in a structured manner, we add a simple lnrpc.ErrDetails message which is a map<string, string> details = 1; gRPC type. With that we could encode the index of the first error as details["index"] = fmt.Sprintf("%d", idx). That would still require the client to match the string name of the field, but that name should remain much more stable.

If error details are a string, is that structured error details?

guggero · 2021-09-01T07:24:13Z

If error details are a string, is that structured error details?

I would argue yes. You get key/value pairs with known and stable keys and you don't have to parse a string to get to them. Yes, you might need to convert the string value into a native data type. But at least this would be very generic and could still be shown as a human readable error string to the user.

But this is just my idea for making things more generic, I'm open to suggestions if you feel there's another way.

joostjager · 2021-09-01T13:02:09Z

I think that ideally you don't have a generic error code, not even if it is per domain. Because with a generic code, you still need to go through the lnd source code to see what the exact error codes are that need to be handled for a specific call, or rely on documentation that can get desynced with the code relatively easy.

Using a specific proto error object gets you the strictest contract. It doesn't need to be one of the predefined google messages that is attached with WithDetails. Also it could be good enough to have a single object per call that is a union of all error attributes for that call. Then the type select isn't needed to distinguish between the various error cases.

I do see the point of ease of use. Just checking the top-level grpc code is very convenient.

alexbosworth · 2021-09-01T14:53:37Z

But this is just my idea for making things more generic, I'm open to suggestions if you feel there's another way.

I think the existing model has its strength in surfacing the documentation of known common failure states to watch out for, but I agree the weakness is having unknown or uncommon errors return a result that is hard to rely on and also it's kind of unreasonable to expect enumeration of every possible error in the gRPC.

For common failures I like the proto structured failure responses, for uncommon errors I like the current model of strings explaining what happened but I think the proposed schema for a number system wouldn't help me much with knowing what to do in response to that error. The main weakness of the structured responses is that sometimes the real underlying failure doesn't really match up to what is reported in the structure, but that could be resolved by adding more enumerations or not being so strict with trying to return a structured error in unexpected cases.

In HTTP the error codes classes of errors also have prescriptions of what to do: in a 4xx class you generally need to fix your own problem and in a 5xx class you need to retry or wait for the server to fix their problem, and within those codes there are specific prescriptions for behavior. That pattern is hard to replicate here so I'm not sure it can be copied.

I'd definitely like to have a 'unique identifier' for an error though which is basically just replacing the string that I'm currently matching for with some value that the RPC says won't change and then the RPC would be more free to change the strings that I'm matching against for typos or adding context etc. It's especially difficult to do string matching when the error string is adding in contextual details, then I have to do regex matching.

guggero · 2021-11-29T12:29:37Z

@arshbot any thoughts on the discussion above? Should we try to pick this up again for 0.15?
I'm going to remove my request for review until we know how we want to proceed with this.

ziggie1984 · 2023-02-21T08:02:53Z

I would take this issue and finish it, is this ok? I saw its up for grabs but asking anyway before starting.

guggero · 2023-02-21T08:06:49Z

I would take this issue and finish it, is this ok? I saw its up for grabs but asking anyway before starting.

Yes, feel free to start working on this. But just a heads up, I don't think we actually came to a conclusion on how exactly we'd like to structure the error codes (see discussion above), so it's possible there might be quite a bit of back and forth during the review.
But given the large design space, it's probably easiest to just show a concrete example in code and take the discussion from there.

ziggie1984 · 2023-03-04T17:54:29Z

not working on this currently, have found another more urgent issue for now, will come back in the future!

lightninglabs-deploy · 2023-08-30T21:58:32Z

@arshbot, remember to re-request review from reviewers when ready

lightninglabs-deploy · 2023-08-31T18:12:21Z

Closing due to inactivity

lightninglabs-deploy · 2023-08-31T19:15:19Z

Closing due to inactivity

lightninglabs-deploy · 2023-08-31T20:18:07Z

Closing due to inactivity

lightninglabs-deploy · 2023-08-31T21:21:18Z

Closing due to inactivity

lightninglabs-deploy · 2023-08-31T22:23:53Z

Closing due to inactivity

lightninglabs-deploy · 2023-08-31T23:26:12Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T00:30:15Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T01:33:01Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T02:35:31Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T03:38:48Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T04:41:30Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T05:43:36Z

Closing due to inactivity

lightninglabs-deploy · 2023-09-01T06:47:11Z

Closing due to inactivity

arshbot force-pushed the structure-fundpsbt branch from 4370424 to ea550e1 Compare June 25, 2021 21:29

walletrpc: Returns structured errors from FundPsbt

4a7356f

Converts errors likely to be thrown from string based errors to error classes with returned grpc error codes

arshbot force-pushed the structure-fundpsbt branch from ea550e1 to 4a7356f Compare June 25, 2021 21:44

joostjager reviewed Jun 27, 2021

View reviewed changes

Roasbeef mentioned this pull request Jun 28, 2021

Add unit tests to guard unstructured errors that are likely to be parsed #5412

Open

arshbot force-pushed the structure-fundpsbt branch from e0042ca to 92c8c0d Compare June 30, 2021 16:02

walletrpc: Attach and organize more metadata

a842fcd

Using status.WithDetails, we attach more metadata which is then printed as json in certain cases for machine consumption. Added the errdetails package to provide more robust error coding

arshbot force-pushed the structure-fundpsbt branch from 92c8c0d to a842fcd Compare July 3, 2021 16:10

lnrpc: compiling protos

d91d8b7

This commit compiles the grpc protos since additional output is added to the FundPSBT command

guggero mentioned this pull request Jul 9, 2021

rpc: Bake and validate macaroons with external permissions #5304

Merged

walletrpc: Add tests for incorrect inputs for FundPSBT

99315a7

Adds a test to ensure errors are thrown when utxos with incorrect indices are provided to FundPSBT.

Roasbeef added this to the v0.14.0 milestone Aug 31, 2021

Roasbeef requested a review from guggero August 31, 2021 00:57

Roasbeef added the P3 might get fixed, nice to have label Aug 31, 2021

guggero mentioned this pull request Aug 31, 2021

rpcperms: add gRPC codes to errors #5633

Open

10 tasks

yyforyongyu mentioned this pull request Sep 10, 2021

itest: fix restore backup file test flake for bitcoind backend #5637

Merged

Roasbeef modified the milestones: v0.14.0, v0.13.2, v0.15.0 Sep 27, 2021

guggero removed their request for review November 29, 2021 12:29

Roasbeef removed this from the v0.15.0 milestone Feb 2, 2022

Roasbeef added the up for grabs PRs which have been abandoned by their original authors and can be taken up by someone else label Feb 2, 2022

guggero mentioned this pull request Aug 29, 2022

Structured Error Handling for BuildRoute #6861

Open

guggero closed this Sep 1, 2023

guggero mentioned this pull request Aug 20, 2024

[feature]: Return structured information when BatchOpenChannel fails #9016

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Returns structured errors from FundPsbt #5436

Returns structured errors from FundPsbt #5436

arshbot commented Jun 25, 2021 •

edited

Loading

joostjager Jun 27, 2021

Roasbeef Jun 28, 2021 •

edited

Loading

Roasbeef Jun 28, 2021

joostjager Jun 29, 2021

arshbot Jun 29, 2021

joostjager Jun 29, 2021

guggero Jun 30, 2021

arshbot Jun 30, 2021

joostjager Jul 7, 2021

guggero commented Aug 31, 2021

alexbosworth commented Aug 31, 2021

guggero commented Sep 1, 2021

joostjager commented Sep 1, 2021

alexbosworth commented Sep 1, 2021

guggero commented Nov 29, 2021

ziggie1984 commented Feb 21, 2023

guggero commented Feb 21, 2023

ziggie1984 commented Mar 4, 2023

lightninglabs-deploy commented Aug 30, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

Returns structured errors from FundPsbt #5436

Returns structured errors from FundPsbt #5436

Conversation

arshbot commented Jun 25, 2021 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Roasbeef Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Aug 31, 2021

alexbosworth commented Aug 31, 2021

guggero commented Sep 1, 2021

joostjager commented Sep 1, 2021

alexbosworth commented Sep 1, 2021

guggero commented Nov 29, 2021

ziggie1984 commented Feb 21, 2023

guggero commented Feb 21, 2023

ziggie1984 commented Mar 4, 2023

lightninglabs-deploy commented Aug 30, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Aug 31, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

lightninglabs-deploy commented Sep 1, 2023

arshbot commented Jun 25, 2021 •

edited

Loading

Roasbeef Jun 28, 2021 •

edited

Loading