Fail request_refund_payment for unsupported chain
#2917
Conversation
WalkthroughThe recent updates focus on enhancing security and consistency in the handling of chains within the refund process and offer creation in a Lightning Network implementation. Specifically, these changes ensure that operations related to refunds and offers are conducted on the correct blockchain network by verifying the chain hash. If a mismatch is detected, the process is halted, and an error is returned, thereby preventing potential issues arising from chain incompatibility. Changes
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 (
|
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/offers_tests.rs (2 hunks)
- lightning/src/offers/offer.rs (1 hunks)
- lightning/src/offers/refund.rs (1 hunks)
Additional comments: 6
lightning/src/ln/offers_tests.rs (3)
- 43-43: The addition of the
Networkimport frombitcoin::network::constantsis appropriate for the tests that follow, which involve specifying blockchain networks (e.g., Signet) to simulate scenarios with unsupported chains.- 671-695: The test
fails_creating_invoice_request_for_unsupported_chaincorrectly simulates a scenario where an invoice request is made for an offer that specifies an unsupported chain (Network::Signet). The test setup, execution, and assertion that anErr(Bolt12SemanticError::UnsupportedChain)is returned are all correctly implemented.- 697-725: The test
fails_sending_invoice_with_unsupported_chain_for_refundaccurately simulates a scenario where a refund request is made for a refund that specifies an unsupported chain (Network::Signet). The setup, execution, and the assertion that anErr(Bolt12SemanticError::UnsupportedChain)is returned are correctly implemented, ensuring that the system behaves as expected when dealing with refunds on unsupported chains.lightning/src/offers/refund.rs (1)
- 305-308: The addition of the
clear_chainmethod in theRefundBuilderstruct allows for resetting thechainfield toNone. This change is consistent with the PR's objective to enhance the handling of refunds on unsupported chains by providing a way to clear previously set chain information. The method is correctly implemented, following Rust's ownership and borrowing rules, and it returnsselffor chaining calls, aligning with the builder pattern used throughout the file.However, it's important to ensure that this method is used appropriately in the context where a refund's chain needs to be validated or reset. Additionally, consider adding unit tests specifically for this method to verify its behavior in various scenarios, such as when the chain is initially set and then cleared.
lightning/src/offers/offer.rs (1)
- 342-345: The addition of the
clear_chainsmethod to theOfferBuilderstruct allows for resetting thechainsfield toNone. This method enhances the flexibility of theOfferBuilderby enabling the removal of previously set chains, which could be particularly useful in testing scenarios or when adjusting an offer to match specific requirements. The implementation is straightforward and correctly sets thechainsfield toNone, ensuring that no blockchain chains are associated with the offer being built. This change aligns with the PR's objective of improving the handling of refunds on unsupported chains by providing a mechanism to clear the chains field in offers, likely for testing purposes.lightning/src/ln/channelmanager.rs (1)
- 7901-7901: Following the chain hash check, the method proceeds with creating an inbound payment if the chain hashes match. This control flow is logical and maintains the integrity of the refund process by ensuring that only valid refunds proceed to the invoice creation stage. It's crucial to verify that the error handling for the new chain hash mismatch scenario is integrated into the broader error handling strategy of the
ChannelManager, ensuring a seamless user experience and clear error messaging.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2917 +/- ##
==========================================
+ Coverage 89.11% 89.30% +0.19%
==========================================
Files 117 117
Lines 95029 95929 +900
Branches 95029 95929 +900
==========================================
+ Hits 84685 85670 +985
+ Misses 7856 7785 -71
+ Partials 2488 2474 -14 ☔ View full report in Codecov by Sentry. |
jkczyz
force-pushed
the
2024-02-refund-unsupported-chain
branch
from
25761b9
to
27f7731
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/offers_tests.rs (2 hunks)
- lightning/src/offers/offer.rs (1 hunks)
- lightning/src/offers/refund.rs (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/offers_tests.rs
- lightning/src/offers/offer.rs
- lightning/src/offers/refund.rs
Yeah, let's wait for that to land. Rebasing this would be easier. |
jkczyz
force-pushed
the
2024-02-refund-unsupported-chain
branch
from
27f7731
to
da3623b
Compare
|
Rebased and resolved merge conflicts with #2903. |
If a Refund has an unsupported chain, ChannelManager should not send an invoice as it can't be paid on that chain. Instead, return an error when calling ChannelManager::request_refund_payment for such refunds.
jkczyz
force-pushed
the
2024-02-refund-unsupported-chain
branch
from
da3623b
to
228e72c
Compare
|
Dropped |
If a
Refundhas an unsupported chain,ChannelManagershould not send an invoice as it can't be paid on that chain. Instead, return an error when callingChannelManager::request_refund_paymentfor such refunds.