Add err match in the package `chain` #937

yyforyongyu · 2024-07-03T15:13:18Z

Implemented the idea suggested by @Roasbeef here, we now handle the error mapping separately.

I really like this approach, as the chain backend handling is now independent of btcd. Image a case where bitcoind updates its error str someday, instead of going through the update chain btcd -> btcwallet -> lnd, we can now just update via btcwallet -> lnd. Think it's also nice to move rpcclient to btcwallet since it's a chain backend multiplexer, future PR tho.

The original PR when this was added to btcd: btcsuite/btcd#2138

This commit moves the defined errors from [btcd/rpcclient/errors.go](https://github.com/btcsuite/btcd/blob/d881c686e61db35e332fb0309178152dac589b03/rpcclient/errors.go) to here so it's easier to manage error matching for different backends.

saubyk · 2024-07-04T15:19:26Z

cc @bhandras for review

saubyk · 2024-07-04T15:22:42Z

cc: @ziggie1984 for review

bhandras

LGTM 🧇

bhandras · 2024-07-04T17:08:16Z

chain/btcd.go

+
+	// If the backend is older than v0.24.2, we will try to match the error
+	// to the older version of `btcd`.
+	for btcdErr, matchedErr := range BtcdErrMapPre2402 {


nit: it could be slightly cleaner if we move this loop inside if !backend.SupportTestMempoolAccept() so that if the version is >= v0.24.2 we just return the error at the end.

yeah was thinking about not nesting ifs, but I guess it's cleaner so now changed.

ziggie1984

Looks good thank you for this fix, had some questions.

ziggie1984 · 2024-07-04T21:49:38Z

chain/bitcoind_client.go

+}
+
+// MapRPCErr takes an error returned from calling RPC methods from various
+// chain backend and map it to an defined error here.


Nit: various chain backends

ziggie1984 · 2024-07-05T08:38:26Z

chain/neutrino.go

@@ -815,3 +815,22 @@ out:
 	close(s.dequeueNotification)
 	s.wg.Done()
 }
+
+// MapRPCErr takes an error returned from calling RPC methods from various
+// chain backend and map it to an defined error here. It uses the `BtcdErrMap`,


Nit: and maps it (because it relates to the error ?)

Yea totally, now fixed!

ziggie1984 · 2024-07-05T08:44:56Z

chain/errors.go

@@ -477,6 +477,52 @@ var BtcdErrMap = map[string]error{
 	"max-fee-exceeded": ErrMaxFeeExceeded,
 }

+// BtcdErrMapPre2402 defines the error mapping for btcd versions prior to
+// 0.24.2 - all the errors changed in this commit have been defined here to


Q: Why did we remove this errors for the newest versions ?

they are not removed, instead, in this commit they are changed so it's easier to do error matching, as they are more "unique".

ziggie1984 · 2024-07-05T09:02:03Z

chain/errors.go

+// - https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_dust.py
+// - https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_limit.py
+// - https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp
+func (r RPCErr) Error() string {


Q: I wonder why we don't name this error in particular bitcoind error, because it reflects the errors of this particular backend ? Calling it rpcerr, I always think this is a generalized error ?

yeah originally it was named BitcoindRPCErr, but in reality this functions as the generalized error - when we use it in lnd, we don't check for backends, but just check against, say, chain.ErrMempoolConflict.

Ideally, we can define another set of errors and put them in a map similar to BtcdErrMap, keyed by the error string returned from bitcoind and valued by the RPCErr, which brings more symmetry. However, unless we have a very different error string returned from the chain, there's no point in doing so as in the current design, we always use the error string defined in bitcoind. In other words, this Error() needs to return something that's human-readable, and we decide to use the error string from bitcoind.

ziggie1984 · 2024-07-05T09:04:15Z

chain/errors.go

+// are results from calling either `testmempoolaccept` or `sendrawtransaction`
+// in `bitcoind`.
+//
+// Errors not mapped in `btcd`:


Q: Do you mean these are particular btcd errros which exist only in btcd or the other way around ?

so these errors can be returned from btcd, but they are not mapped. so yeah they only exist in btcd.

ziggie1984 · 2024-07-05T09:18:31Z

chain/errors.go

+//     `CheckTransactionInputs`.
+//   - errors from `CalcSequenceLock`.
+//
+// NOTE: This is not an exhaustive list of errors, but it covers the


Hmm I am still a bit surprised why we are moving the error definition to the btcwallet pkg, imo they fit better into the btcd pkg, especially the btcd errors should be defined there anyways because its the official client?

IMO it's better here. First of all, this set of newly defined error types are not btcd errors, they are RPC errors, returned by either calling sendrawtransaction or in the reject-reason field in a response returned by testmempoolaccept. When in lnd, we don't care about the specific backend, rather, we just want to get a defined error and act on it.

This means whether it's btcd, bitcoind, or neutrino, we should have a place to catch the implementation-specific errors, and "translate" them into a uniformized RPC error. chain is a good place to perform this task as it acts like a backend-multiplexer - it provides a standardized interface chain.Interface, and handles the implementation-specific logic here.

On the other hand, btcd should just be, well, btcd, an independent implementation, that's oblivious about how neutrino or bitcoind handles stuff, which is why in addition to this error handling, I also think the btcd/rpcclient should be moved here. The overall design is, that btcwallet provides multi-backend handling, and btcd just being a protocol implementation.

Great explanation thank you

ziggie1984 · 2024-07-05T09:34:00Z

chain/neutrino.go

+	}
+
+	// If not matched, return the original error wrapped.
+	return fmt.Errorf("%w: %v", ErrUndefined, rpcErr)


Q: Shouldn't we wrap it the other way around or even both ?

ErrUndefined is the defined type tho, so in lnd when we see chain.ErrUndefined we know it's an undefined error. On the other hand rpcErr won't have a type in this case, also we cannot use more than one %w in fmt.Errorf unfortunately.

This commit adds a new interface method `MapRPCErr` and use it in `TestMempoolAccept` and `SendRawTransaction`.

ziggie1984

LGTM 🔥

yyforyongyu mentioned this pull request Jul 3, 2024

rpcclient: update ErrTxAlreadyConfirmed to match both versions btcsuite/btcd#2209

Closed

yyforyongyu force-pushed the add-err-match branch from 2bc65fd to 6e238fd Compare July 3, 2024 15:27

yyforyongyu mentioned this pull request Jul 3, 2024

multi: fix lnwallet.ErrDoubleSpend lightningnetwork/lnd#8887

Merged

yyforyongyu force-pushed the add-err-match branch 2 times, most recently from ecd01d4 to bc151ce Compare July 4, 2024 05:42

bhandras approved these changes Jul 4, 2024

View reviewed changes

yyforyongyu force-pushed the add-err-match branch from bc151ce to 178e735 Compare July 4, 2024 17:30

ziggie1984 reviewed Jul 5, 2024

View reviewed changes

chain+wallet: define and use MapRPCErr for chain backends

f4133be

This commit adds a new interface method `MapRPCErr` and use it in `TestMempoolAccept` and `SendRawTransaction`.

yyforyongyu force-pushed the add-err-match branch from 178e735 to 1f08d12 Compare July 5, 2024 10:02

chain: match errors from btcd pre-v0.24.2

a90a3ec

yyforyongyu force-pushed the add-err-match branch from 1f08d12 to a90a3ec Compare July 5, 2024 10:12

ziggie1984 approved these changes Jul 5, 2024

View reviewed changes

yyforyongyu merged commit e391a1c into btcsuite:master Jul 6, 2024
3 checks passed

yyforyongyu deleted the add-err-match branch July 6, 2024 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add err match in the package `chain` #937

Add err match in the package `chain` #937

yyforyongyu commented Jul 3, 2024 •

edited

Loading

saubyk commented Jul 4, 2024

saubyk commented Jul 4, 2024

bhandras left a comment

bhandras Jul 4, 2024

yyforyongyu Jul 4, 2024

ziggie1984 left a comment

ziggie1984 Jul 4, 2024

yyforyongyu Jul 5, 2024

ziggie1984 Jul 5, 2024

yyforyongyu Jul 5, 2024

ziggie1984 Jul 5, 2024

yyforyongyu Jul 5, 2024

ziggie1984 Jul 5, 2024 •

edited

Loading

yyforyongyu Jul 5, 2024

ziggie1984 Jul 5, 2024

yyforyongyu Jul 5, 2024

ziggie1984 Jul 5, 2024

yyforyongyu Jul 5, 2024

ziggie1984 Jul 5, 2024

ziggie1984 Jul 5, 2024

yyforyongyu Jul 5, 2024

ziggie1984 left a comment

Add err match in the package chain #937

Add err match in the package chain #937

Conversation

yyforyongyu commented Jul 3, 2024 • edited Loading

saubyk commented Jul 4, 2024

saubyk commented Jul 4, 2024

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 left a 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

ziggie1984 Jul 5, 2024 • 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

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

Add err match in the package `chain` #937

Add err match in the package `chain` #937

yyforyongyu commented Jul 3, 2024 •

edited

Loading

ziggie1984 Jul 5, 2024 •

edited

Loading