lnwallet: use new errors returned from rpcclient
#8554
Conversation
WalkthroughThis update focuses on refining error handling and transaction rejection logic across the system. It removes an unnecessary logging argument from a build process, enhances error handling in transaction broadcasting by leveraging Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
yyforyongyu
force-pushed
the
use-new-errors
branch
2 times, most recently
from
912a37c
to
645abe7
Compare
yyforyongyu
force-pushed
the
use-new-errors
branch
from
645abe7
to
fd6bba8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ff31426 and fd6bba8a0342eaec6c68affd0bfc5f885b7d12c2.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files not reviewed due to errors (2)
- (no review received)
- lnwallet/test/wallet_test.go (no review received)
Additional comments: 5
.github/workflows/main.yml (1)
- 221-221: The removal of the
log="stdlog debug"argument from themakecommand simplifies the build process. However, ensure that this change does not impact the visibility or debugging capabilities of the build logs, especially in cases of build failures.docs/release-notes/release-notes-0.18.0.md (1)
- 215-217: The documentation accurately reflects the enhancement in error handling by specifying that
PublishTransactionnow returns error types defined inbtcd/rpcclient. This change is crucial for developers and users to understand the improvements in error specificity and handling, especially in the context of transaction broadcasting and RBF failures.config_builder.go (3)
- 21-21: The addition of the
github.com/btcsuite/btcd/rpcclientimport is necessary for utilizing therpcclient.MapRPCErrfunction for improved error handling. This change aligns with the PR objectives of enhancing error handling within thelnwalletmodule by leveraging new, detailed RPC error types.- 1443-1464: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1446-1472]
The modifications to the
broadcastErrorMapperfunction to utilizerpcclient.MapRPCErrfor error mapping and to handle specific backend errors with custom logic are well-implemented. This approach enhances error handling by providing more granular control over how different types of errors are treated, especially in the context of transaction broadcasting. It's important to ensure that all relevant error cases are covered and that the custom error handling logic aligns with the expected behavior of the system.
- 1469-1475: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1475-1577]
The
initNeutrinoBackendfunction is well-structured and implements error handling appropriately. The use of a cleanup function to ensure resources are properly released in case of initialization errors is a good practice. It's also commendable that the function takes into account various configuration parameters and adapts the neutrino client's behavior accordingly.
yyforyongyu
force-pushed
the
use-new-errors
branch
from
fd6bba8
to
a1b6b60
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ff31426 and a1b6b60cf3cef7c06118be41d1e2cca45210254e.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Check Runs (1)
check commits failure (4)
- lnwallet/btcwallet/btcwallet.go: 1237-1237: undefined: base.ErrReplacement
- lnwallet/btcwallet/btcwallet.go: 1304-1304: undefined: base.MapBroadcastBackendError
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/test/test_interface.go
Additional comments: 28
docs/release-notes/release-notes-0.18.0.md (24)
- 215-217: The update to
PublishTransactionto return error types frombtcd/rpcclientis accurately documented. This change aligns with the PR's objective to enhance error handling by utilizing detailed RPC error types.- 212-213: The addition of custom callbacks in the
healthcheckpackage is well-documented. This feature allows for more flexible and responsive health monitoring.- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [6-6]
The term "lncli" is correctly used in the context of command-line interface commands and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [24-24]
The command
sendcoinsis correctly spelled as it refers to a specific command name. No change is needed.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [38-38]
The abbreviation "lnd" is correctly used to refer to the software name and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [50-50]
The term "neutrino" is correctly used in the context of a backend and does not require a comma after "backend."
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [53-53]
The term "walletrpc" is correctly used to refer to a specific RPC endpoint and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [56-56]
The abbreviation "lnd" is correctly used to refer to the software name and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [71-71]
The use of an accent character in "
shutdowns" is appropriate given the context of marking a command within text. No change is needed.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [76-76]
The phrase "at the same time that" is correctly used in this context to describe concurrent events. No change is needed.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [78-78]
The term "re-init" is used correctly in the context of reinitializing a process. No change is needed.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [84-84]
The term "rpc" is correctly used in the context of RPC (Remote Procedure Call) and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [98-98]
The term "onchain" is commonly used in the cryptocurrency domain to refer to transactions that occur on the blockchain. No change is needed.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [108-108]
The term "lncli" is correctly used in the context of command-line interface commands and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [114-114]
The term "http" is correctly used in the context of HTTP (Hypertext Transfer Protocol) and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [126-126]
The term "lncli" is correctly used in the context of command-line interface commands and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [135-135]
The abbreviation "lnd" is correctly used to refer to the software name and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [138-138]
The term "testmempoolaccept" is correctly used to refer to a specific RPC command in Bitcoin Core and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [145-145]
The term "PsbtCoinSelect" is correctly used to refer to a specific option of the
FundPsbtRPC and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [186-186]
The term "lncli" is correctly used in the context of command-line interface commands and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [198-198]
The term "lncli" is correctly used in the context of command-line interface commands and does not require correction.
- 212-221: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [203-203]
The term "Tlv" refers to Type-Length-Value encoding and is correctly used in this context. No change is needed.
- 213-213: The term "PublishTransaction" is correctly used to refer to a specific function and does not require correction.
- 216-216: The term "btcd/rpcclient" is correctly used to refer to a specific package and does not require correction.
lnwallet/btcwallet/btcwallet.go (4)
- 1251-1251: The call to
mapRpcclientErrorhere is appropriate for handling errors fromPublishTransactionin a consistent manner. This ensures that any RPC-related errors are translated into internal error types that are more meaningful within the context of the wallet.- 1270-1270: The usage of
mapRpcclientErrorafter attempting to publish the transaction directly whenTestMempoolAcceptis not supported by the backend is correct. It maintains consistency in error handling across different scenarios.- 1289-1289: Again, the application of
mapRpcclientErrorafter publishing the transaction, based on the result ofTestMempoolAccept, is consistent with the approach taken in other parts of the code. This ensures that any errors encountered are properly mapped to internal types.- 1314-1329: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1299-1326]
The handling of specific errors returned from
TestMempoolAcceptby mapping them usingrpcclient.MapRPCErrand then further processing withmapRpcclientErroris a good approach. However, there's a reference tobase.MapBroadcastBackendErrorin the static analysis hints which is not present in the provided code. If this is an error handling function that should be used here, ensure it's correctly defined and imported. Additionally, the comment about rebroadcasting and the TODO note provide valuable context for future improvements.// Handle specific errors returned from TestMempoolAccept. err = rpcclient.MapRPCErr(errors.New(result.RejectReason)) switch { case errors.Is(err, rpcclient.ErrTxAlreadyInMempool), errors.Is(err, rpcclient.ErrTxAlreadyKnown): // Call PublishTransaction again to mark the label in the wallet. err := b.wallet.PublishTransaction(tx, label) log.Errorf("PublishTransaction error: %v", err) return mapRpcclientError(err) } return mapRpcclientError(err)Consider verifying the existence and correct usage of
base.MapBroadcastBackendErrorif it's relevant to this context.
yyforyongyu
force-pushed
the
use-new-errors
branch
from
a1b6b60
to
b6c4eec
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ff31426 and b6c4eec287f71e2e222da9f5c379115bd87055f1.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/test/test_interface.go
Additional comments: 1
docs/release-notes/release-notes-0.18.0.md (1)
- 215-217: The update to
PublishTransactionto return error types frombtcd/rpcclientis a significant improvement in error handling. It's crucial to ensure that all references to error handling in the documentation and code comments reflect this change for consistency and clarity.
yyforyongyu
force-pushed
the
use-new-errors
branch
from
b6c4eec
to
7b134e2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 51ebc20 and 7b134e22396d61f49d09082740cff218eacc5b40.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/test/test_interface.go
Additional comments: 4
docs/release-notes/release-notes-0.18.0.md (1)
- 215-217: The update to
PublishTransactionto return specific error types frombtcd/rpcclientis a significant improvement for error handling. This change aligns with the PR's objective to enhance error granularity and specificity, especially in transaction broadcasting and RBF failure scenarios.lnwallet/btcwallet/btcwallet.go (3)
- 1250-1250: The usage of
mapRpcclientErrorto map errors when publishing a transaction is consistent with the new error handling strategy. This ensures that errors from therpcclientpackage are correctly interpreted within the context of the wallet.- 1269-1269: Again, the use of
mapRpcclientErrorhere is appropriate and maintains consistency in error handling across different parts of the transaction publishing process.- 1288-1288: The consistent use of
mapRpcclientErrorfor error mapping in the transaction publishing logic is noted and approved. It's good to see a uniform approach to error handling.
There was a problem hiding this comment.
Looks very good had just some Nits.
I wonder if we map BTCD errors resulting from the testmempoolaccept cmd to bitcoind's sendrawtransaction errors or are we strict and try to always map them equally:
btcd's testmempool accept errors => bitcoind's testmempool accept errors ?
yyforyongyu
force-pushed
the
use-new-errors
branch
from
7b134e2
to
89c2570
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between b0552da and 89c25706a71503bbde3aa5d733ae24c457aac284.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/test/test_interface.go
Additional comments: 1
docs/release-notes/release-notes-0.18.0.md (1)
- 215-217: The update to
PublishTransactionnow returning error types frombtcd/rpcclientis a significant improvement in error handling. This change aligns with the PR's objective to enhance error granularity and specificity, especially in transaction broadcasting and RBF failure scenarios.
yyforyongyu
force-pushed
the
use-new-errors
branch
from
89c2570
to
d7342c7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between b0552da and d7342c7045371ed28968356b12ab24c5e09c4de4.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/test/test_interface.go
Additional comments: 1
docs/release-notes/release-notes-0.18.0.md (1)
- 223-226: The update to the
PublishTransactionfunction to return error types frombtcd/rpcclientis a significant improvement for error handling. This change aligns with the PR's objective to enhance error handling capabilities, especially in scenarios involving Replace-By-Fee (RBF) failures. It's crucial to ensure that all relevant documentation and examples are updated to reflect this change, helping users to understand the new error types and how to handle them effectively.
yyforyongyu
force-pushed
the
use-new-errors
branch
from
d7342c7
to
750c72c
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between b1d3b9f and 750c72cb507acb9413d1435cd5d230679b76ea80.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/test/test_interface.go
Additional comments: 1
docs/release-notes/release-notes-0.18.0.md (1)
- 223-226: The documentation of
PublishTransactionnow returning error types defined inbtcd/rpcclientis clear and accurately reflects the changes made in the PR. This enhancement should improve error handling capabilities, aligning with the PR's objectives.Additionally, it's recommended to review the entire document for potential typographical and grammatical improvements. While the static analysis hints suggest several issues, a manual review can help ensure the document's overall quality and readability.
This commit updates `btcd` and `btcwallet` packages and make use of the new RPC error mappings.
These loggings are not helpful.
yyforyongyu
force-pushed
the
use-new-errors
branch
from
750c72c
to
9dace43
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- .github/workflows/main.yml (1 hunks)
- config_builder.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnwallet/btcwallet/btcwallet.go (6 hunks)
- lnwallet/test/test_interface.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/main.yml
- config_builder.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/test/test_interface.go
Additional comments: 1
docs/release-notes/release-notes-0.18.0.md (1)
- 223-226: The documentation of
PublishTransactionnow returning error types frombtcd/rpcclientaligns well with the PR's objectives and is correctly formatted. This change should enhance error handling capabilities as intended.
Depends on,
rpcclientbtcsuite/btcwallet#918This PR starts to make use of the detailed RPC error types, enabling us to have better control over RBF failures.
Summary by CodeRabbit
PublishTransactionfunction usingbtcd/rpcclient.PublishTransactionfor clarity.broadcastErrorMapperto userpcclient.MapRPCErr.