Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpcclient: update ErrTxAlreadyConfirmed to match both versions #2209

Closed
wants to merge 1 commit into from

Conversation

yyforyongyu
Copy link
Contributor

This commit updates the error str to match the same error ErrTxAlreadyConfirmed returned from btcd for both pre-0.24.2 and post-0.24.2.

This commit updates the error str to match the same error
ErrTxAlreadyConfirmed returned from `btcd` for both pre-0.24.2 and
post-0.24.2.
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9766451834

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 57.235%

Totals Coverage Status
Change from base Build 9701102084: 0.04%
Covered Lines: 29836
Relevant Lines: 52129

💛 - Coveralls

Copy link

@leafl3d leafl3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the past few patches, regressions, and bugs in this area have shown that this approach is too fragile. The initial culprit was this commit that "improved" some of the errors to give additional detail. This in turn broke the string based error matching logic.

Today bitcoind will send 3 primary JSON-RPC errors due to error cases encountered during the processing of sendrawtransaction: https://github.com/bitcoin/bitcoin/blob/173ab0ccf2164dd8cd3d486559dffda0d4a1a813/src/rpc/protocol.h#L47-L49

btcd on the other hand uses the existing error codes from the reject message (which bitcoind removed quite sometime ago unfortunately): https://github.com/btcsuite/btcd/blob/master/wire/msgreject.go#L21-L26.

To make our handling of sendrawtransaction RPC errors more robust, what if we just switched nearly entirely to:

  1. Detecting which backend is being used (we know that btcd operates over ws as an example).
  2. Depending on the backend, check the set of distinct errors.

IIUC, this forces our error handling to be much coarser. A side effect of this may be that we're able able to properly detect subtle cases like rejecting due to being invalid vs not following the RBF rules. To fill this gap, elsewhere we already use testmempoolaccept, which has a more structured error output it still isn't great though, as if any of these error strings change, then we enter unexpected behavior territory.

@yyforyongyu
Copy link
Contributor Author

A side effect of this may be that we're able to properly detect subtle cases like rejecting due to being invalid vs not following the RBF rules.

Think we can already distinguish the specific cases as implemented in #2138. I really like the idea of separation on handling errors, so implemented it here btcsuite/btcwallet#937.

For bitcoind, it has an error code along with the error message, which is kinda nice as it can be used to decide the error type independent of future error strings change, which is sort like the reject msg used in btcd. However these error codes are not granular enough - multiple errors share the same code, but we may want to further distinguish them.
Moving forward, we can replace the error string match if it's from btcd as we can easily define more reject messages here, then we can match against a concrete type or an error code. For bitcoind, it's not the case as in testmempoolaccept there's no error code returned in the reject-reason section 🤦🏻.

@ziggie1984
Copy link
Contributor

maybe we funnel this feedback in regards of the error strings vs concrete error types back to bitcoind so that they can adopt or at least think about other ways which might make the error handling easier for third party applications using their API ?

@yyforyongyu yyforyongyu deleted the fix-err-match branch July 10, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants