Functional tests for BOLT 12 Offers payment flow

[jkczyz]

ChannelManager provides utilities to create Offers and Refunds along with utilities to initiate and request payment for them, respectively. It also manages the payment flow via implementing OffersMessageHandler. Test that functionality, including the resulting Event generation.

[jkczyz]

Just seeking Concept ACK on the testing approach. The difficulty here is that:

• the payment preimage is generated internally when an invoice is generated
• the onion message payloads are encrypted, so the tests don't have access to InvoiceRequest or Bolt12Invoice messages

[valentinewallace]

Concept ACK on this approach.

Would it be possible to add a wrapping TestOffersMessageHandler around TestChannelManager and use that as OnionMessenger's OffersMessageHandler? That way we could expect specific offers messages and check their properties.

[jkczyz]

I'm really confused by this test failure:

thread 'ln::monitor_tests::test_restored_packages_retry' panicked at lightning/src/ln/monitor_tests.rs:2034:13:
assertion `left == right` failed
  left: OutPoint { txid: 0dfec8a032dd7df936c62f8a43c40ed92b15fdc76a513f955be589092dd4cf45, vout: 0 }
 right: OutPoint { txid: 5df730c2e11eca1db9e0b7fd6ad8871a49674fc240c1db1e2a91b5f813e24829, vout: 0 }

error: test failed, to rerun pass `--lib`

failures:
    ln::monitor_tests::test_restored_packages_retry

Seems the commitment txid changes when reloading the node with an old ChannelMonitor serialization. The non-reload case works fine.

[jkczyz]

Concept ACK on this approach.

Would it be possible to add a wrapping TestOffersMessageHandler around TestChannelManager and use that as OnionMessenger's OffersMessageHandler? That way we could expect specific offers messages and check their properties.

That might require another level of configs? :( We need to store TestOffersMessageHandler prior to creating TestOnionMessenger but it also needs a reference to TestChannelManager.

Or at very least there would need to be another creation utility to create TestOffersMessageHandlers prior to calling create_network given the result of create_node_chanmgrs. And all call sites of create_network would need to be updated accordingly.

Is there any better alternatives? Maybe have two different create_network functions and where both the TestOnionMessenger and the new TestOffersMessageHandler fields becomes a Options? Might be simpler to use peel_onion_message to extract the message from the onion and perform any checks instead.

[valentinewallace]

Is there any better alternatives? Maybe have two different create_network functions and where both the TestOnionMessenger and the new TestOffersMessageHandler fields becomes a Options? Might be simpler to use peel_onion_message to extract the message from the onion and perform any checks instead.

Either of these seem reasonable to me. May be simpler to start with the latter, since we have that available now anyway.

[TheBlueMatt]

Approach seems fine. I don't think its worth nitpicking too much here, we'll figure out what is missing in the test framework as we write more tests and find bugs, trying to get it perfect now isn't really gonna happen.

[TheBlueMatt]

fewer macros, more functions please.

[jkczyz]

Need this one for matching the enum variant, but the others can be functions.

[jkczyz]

Rebased on #2781

[jkczyz]

I'm confused as to why d70279d would cause the following test failure:

thread 'ln::monitor_tests::test_restored_packages_retry' panicked at 'assertion failed: `(left == right)`
  left: `OutPoint { txid: 0x0dfec8a032dd7df936c62f8a43c40ed92b15fdc76a513f955be589092dd4cf45, vout: 0 }`,
 right: `OutPoint { txid: 0x5df730c2e11eca1db9e0b7fd6ad8871a49674fc240c1db1e2a91b5f813e24829, vout: 0 }`', lightning/src/ln/monitor_tests.rs:2040:13


failures:
    ln::monitor_tests::test_restored_packages_retry

Corresponding to this check:

rust-lightning/lightning/src/ln/monitor_tests.rs

Line 2040 in d70279d

assert_eq!(tx.input[0].previous_output, htlc_timeout_tx.input[0].previous_output);

Any ideas why this would be?

[jkczyz]

Any ideas why this would be?

Ok, I figured this out. OnionMessenger makes use of its EntropySource upon construction to feed random data to Secp256k1::seeded_randomize. Since the EntropySource is shard across other data structures, this would cause signatures to change and thus txids.

Ultimately, this test is brittle since it is using older serialized data that had used an EntropySource that wan't used with an OnionMessenger. To resolve this, a few options come to mind:

• Don't call Secp256k1::seeded_randomize in tests
• Initialize OnionMessenger with a dedicated EntropySource/TestKeysManager
• Somehow reconstruct the serialized data with an extra call to the EntropySource

I'll go for the second option unless anyone has strong opinions.

[codecov-commenter]

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (ea5de93) 88.52% compared to head (c43b4c0) 89.16%.

Files	Patch %	Lines
lightning/src/ln/offers_tests.rs	95.85%	22 Missing and 8 partials ⚠️
lightning/src/util/test_utils.rs	73.68%	10 Missing ⚠️
lightning/src/ln/outbound_payment.rs	81.81%	2 Missing ⚠️
lightning/src/sign/mod.rs	89.47%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2697      +/-   ##
==========================================
+ Coverage   88.52%   89.16%   +0.63%     
==========================================
  Files         114      115       +1     
  Lines       92090    92879     +789     
  Branches    92090    92879     +789     
==========================================
+ Hits        81526    82817    +1291     
+ Misses       8058     7533     -525     
- Partials     2506     2529      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[G8XSU]

Isn't it unsafe if this isn't persisted and we will reset it to new counter? Do we expect uniq seed every time?

Looking at this struct independently, it seems to imply "give me a static seed and it will work"

[jkczyz]

Isn't it unsafe if this isn't persisted and we will reset it to new counter? Do we expect uniq seed every time?

Yeah, a unique seed is expected if you don't want to produce the same values.

Looking at this struct independently, it seems to imply "give me a static seed and it will work"

Depends on what you mean by "works". There isn't anything in the public docs claiming that restarting with the same seed will not produce any of the previous values. Happy to reword the private docs here. I was just copying them.

[jkczyz]

CI finally passes. 😅 Had to make some minor changes to support no-std and c_bindings.

[coderabbitai]

Review Status

Actionable comments generated: 6

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ea5de93 and c43b4c0.

Files selected for processing (18)

• lightning/src/ln/channelmanager.rs (5 hunks)
• lightning/src/ln/features.rs (1 hunks)
• lightning/src/ln/functional_test_utils.rs (11 hunks)
• lightning/src/ln/functional_tests.rs (1 hunks)
• lightning/src/ln/mod.rs (1 hunks)
• lightning/src/ln/offers_tests.rs (1 hunks)
• lightning/src/ln/outbound_payment.rs (11 hunks)
• lightning/src/ln/payment_tests.rs (2 hunks)
• lightning/src/ln/peer_handler.rs (2 hunks)
• lightning/src/offers/offer.rs (1 hunks)
• lightning/src/offers/refund.rs (1 hunks)
• lightning/src/onion_message/messenger.rs (5 hunks)
• lightning/src/routing/gossip.rs (1 hunks)
• lightning/src/routing/router.rs (5 hunks)
• lightning/src/routing/scoring.rs (1 hunks)
• lightning/src/sign/mod.rs (9 hunks)
• lightning/src/util/test_utils.rs (5 hunks)
• lightning/src/util/time.rs (3 hunks)

Files skipped from review due to trivial changes (1)

• lightning/src/ln/payment_tests.rs

Additional comments: 168

lightning/src/ln/mod.rs (1)

• 82-84: The addition of the offers_tests module is consistent with the existing pattern of test module declarations in the file.

lightning/src/util/time.rs (4)

• 62-62: The conditional compilation directive for the MonotonicTime struct is correctly applied.
• 67-67: The SHIFT constant is appropriately gated with the "std" feature flag.
• 59-73: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [70-96]

The implementation of the Time trait for MonotonicTime is correctly placed under the "std" feature flag, and the logic to prevent overflow and handle time going backwards is sound.

• 180-180: The conditional compilation for the MonotonicTime subtraction test is correctly applied and the test logic is appropriate.

lightning/src/ln/offers_tests.rs (17)

• 59-69: Past comment by TheBlueMatt suggests using functions over macros, and jkczyz acknowledges the need for the macro in this specific case. No further action needed.
• 354-354: Past comments by valentinewallace and coderabbitai[bot] discuss the ConnectionNeeded flow and its testing. The conversation seems resolved with no further action required.
• 42-42: Past comment by coderabbitai[bot] about an extra newline has been addressed. No further action needed.
• 183-268: The test creates_and_pays_for_offer_using_two_hop_blinded_path is well-structured and covers the scenario of creating and paying for an offer using a two-hop blinded path. The assertions and flow are logical and consistent with the expected behavior.
• 273-348: The test creates_and_pays_for_refund_using_two_hop_blinded_path follows a similar structure to the previous test and correctly tests the refund flow using a two-hop blinded path. The test is clear and the assertions are appropriate.
• 354-405: The test creates_and_pays_for_offer_using_one_hop_blinded_path is correctly implemented to test the payment of an offer using a one-hop blinded path. The test is concise and the logic is sound.
• 411-459: The test creates_and_pays_for_refund_using_one_hop_blinded_path is consistent with the previous tests and correctly tests the refund flow using a one-hop blinded path. The test is well-structured and the assertions are valid.
• 465-502: The test pays_for_offer_without_blinded_paths successfully covers the scenario where an offer is paid without any blinded paths. The test is straightforward and the assertions are correct.
• 507-544: The test pays_for_refund_without_blinded_paths is implemented correctly to test the payment of a refund without blinded paths. The test is clear and the assertions are appropriate.
• 548-583: The test fails_creating_offer_without_blinded_paths correctly asserts that an error is returned when attempting to create an offer without blinded paths. The test is simple and effectively validates the expected error condition.
• 564-618: The test fails_creating_refund_without_blinded_paths is well-implemented to assert that an error is returned when attempting to create a refund without blinded paths. The test is concise and correctly checks for the expected error.
• 588-617: The test fails_creating_invoice_request_without_blinded_reply_path properly tests that an error is returned when a blinded reply path cannot be created. The test is straightforward and the expected error is correctly asserted.
• 621-657: The test fails_creating_invoice_request_with_duplicate_payment_id is correctly designed to test the error condition when a duplicate payment ID is used. The test is clear and the expected error is correctly asserted.
• 661-685: The test fails_creating_refund_with_duplicate_payment_id effectively tests the error condition when a duplicate payment ID is used for a refund. The test is well-structured and the expected error is correctly asserted.
• 689-752: The test fails_sending_invoice_without_blinded_payment_paths_for_offer is correctly implemented to test the error condition when no blinded payment paths are available for an offer. The test is clear and the expected error is correctly asserted.
• 755-799: The test fails_sending_invoice_without_blinded_payment_paths_for_refund is correctly implemented to test the error condition when no blinded payment paths are available for a refund. The test is clear and the expected error is correctly asserted.
• 803-893: The test fails_paying_invoice_more_than_once is correctly implemented to test the error condition when an invoice is paid more than once. The test is clear and the expected error is correctly asserted.

lightning/src/onion_message/messenger.rs (3)

• 43-43: The imports and trait implementations are correctly placed and seem to be necessary for the subsequent code.
• 716-719: The set_offers_handler function is a straightforward setter method, which is a common pattern in Rust for test configurations.
• 821-827: The peel_onion_message method is a wrapper around the peel_onion_message function, correctly passing the internal state to the function.

lightning/src/offers/refund.rs (2)

• 300-303: The addition of the clear_paths method to RefundBuilder is a straightforward change that allows for resetting the paths field to None.
• 304-304: The modification to the features_unchecked method to accept an InvoiceRequestFeatures argument aligns with the need to specify features for an invoice request. This change is consistent and appropriate for the context in which it is used.

lightning/src/ln/features.rs (1)

• 935-940: The addition of the clear_route_blinding method within the impl<T: sealed::RouteBlinding> Features<T> block is consistent with the existing pattern of feature flag manipulation methods. This method allows for the clearing of route blinding bits, which is useful in testing scenarios where features need to be toggled.

lightning/src/util/test_utils.rs (73)

• 35-35: The import of DefaultMessageRouter is correct and necessary for the changes in this file.
• 38-38: The import of DefaultRouter and related types is correct and necessary for the changes in this file.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.
• 120-129: The new function for TestRouter has been correctly updated to instantiate DefaultRouter with the new parameters.
• 204-204: The find_route method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter when no expected routes are queued.
• 210-215: The create_blinded_payment_paths method in the Router trait implementation for TestRouter correctly delegates to the DefaultRouter.
• 247-254: The TestMessageRouter struct and its new function are correctly implemented to wrap the DefaultMessageRouter.
• 258-271: The MessageRouter trait implementation for TestMessageRouter correctly delegates to the inner DefaultMessageRouter.
• 1433-1435: The conditional compilation for c_bindings and the implementation of the Score trait for TestScorer are correct.
• 107-113: The TestRouter struct has been correctly modified to include the DefaultRouter type with appropriate generics.

lightning/src/offers/offer.rs (1)

• 342-345: The addition of the clear_paths method is straightforward and correctly sets the paths field of the OfferContents to None. This method allows for clearing any previously added blinded paths, which can be useful in certain scenarios where an offer needs to be modified before finalization.

lightning/src/sign/mod.rs (1)

• 1877-1904: The RandomBytes struct and its implementation of the EntropySource trait look good. It uses the ChaCha20 algorithm to generate random bytes, which is a secure choice for cryptographic applications. The use of an atomic counter to ensure unique nonces for each call to get_secure_random_bytes is a good practice.

lightning/src/ln/outbound_payment.rs (38)

• 26-26: The PendingOutboundPayment enum is well-structured and covers various states of an outbound payment, such as Legacy, AwaitingInvoice, InvoiceReceived, Retryable, Fulfilled, and Abandoned. This structure allows for clear state management of payments within the system.
• 26-26: The implementation of methods within the PendingOutboundPayment enum, such as increment_attempts, is_auto_retryable_now, insert_previously_failed_scid, is_awaiting_invoice, is_fulfilled, abandoned, get_pending_fee_msat, payment_hash, mark_fulfilled, mark_abandoned, remove, and insert, are logically sound and provide a comprehensive API for managing the state of outbound payments.
• 285-285: The Retry enum provides a clear representation of retry strategies, including a maximum number of attempts (Attempts) and a timeout (Timeout). The conditional compilation ensures that the Timeout variant is only available when the "std" feature is enabled, which is appropriate since Duration is not available in no_std environments.
• 293-299: The impl_writeable_tlv_based_enum! macro is correctly used to implement serialization and deserialization for the Retry enum, with appropriate conditional compilation directives to handle the presence of the "std" feature.
• 312-315: The is_retryable_now method within the Retry implementation correctly calculates whether a retry is possible based on the strategy and the number of attempts made. The use of MonotonicTime and SinceEpoch under different compilation conditions (std and test) is appropriate and ensures that the method behaves correctly in both testing and production environments.
• 26-26: The has_expired function is a utility that checks if a given RouteParameters object has expired based on the current system time. This function is correctly gated behind the "std" feature flag, as it relies on std::time::SystemTime, which is not available in no_std environments.
• 26-26: The PaymentAttemptsUsingTime struct and its associated impl blocks are well-designed to track the number of payment attempts and the time at which the first attempt was made. The use of conditional compilation to handle the presence of the "std" feature is correct, and the new and fmt methods are implemented appropriately.
• 26-26: The impl_writeable_tlv_based_enum! macro is correctly used to implement serialization and deserialization for the StaleExpiration enum, which represents the expiration strategy for a payment awaiting an invoice.
• 26-26: The RetryableSendFailure enum provides a clear representation of possible failures that can occur when sending a payment. Each variant is well-documented and covers a distinct failure scenario, allowing for precise error handling.
• 26-26: The PaymentSendFailure enum provides a comprehensive set of failure states for payment sending, including parameter errors, path errors, and duplicate payments. The detailed variants allow for nuanced error handling and user feedback.
• 26-26: The Bolt12PaymentError enum accurately represents the specific errors that can occur when attempting to pay a BOLT 12 invoice, such as unexpected invoices or duplicate payment attempts. This targeted error handling is beneficial for debugging and user communication.
• 26-26: The ProbeSendFailure enum is well-defined to represent the errors that can occur when sending a payment probe. It includes cases for route not found and sending failures, which are essential for understanding the state of a probe.
• 26-26: The RecipientOnionFields struct and its methods provide a clear and flexible way to construct the onion fields required for sending HTLCs. The ability to create fields with only a secret, empty fields, or fields with custom TLVs is a robust design that accommodates various use cases.
• 26-26: The impl_writeable_tlv_based! macro is correctly used to implement serialization and deserialization for the RecipientOnionFields struct. The TLV fields are appropriately marked as optional, allowing for flexible onion construction.
• 26-26: The SendAlongPathArgs struct is a well-structured container for arguments required when sending a payment along a path. The OutboundPayments struct provides a comprehensive set of methods for managing outbound payments, including sending payments, handling HTLC claims, and removing stale or failed payments. The code is well-organized and maintainable.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The impl_writeable_tlv_based_enum_upgradable! macro is correctly used to implement serialization and deserialization for the PendingOutboundPayment enum. The macro allows for future upgrades to the enum by specifying a not_written field, which is a forward-compatible design.
• 26-26: The payment_is_probe function correctly determines whether a payment is a probe based on the payment hash and payment ID. The use of a probing cookie secret to generate the expected payment hash for a probe is a secure and effective way to identify probes.
• 26-26: The probing_cookie_from_id function generates a probing cookie based on a payment ID and a secret. The use of SHA256 for hashing ensures a secure and deterministic output, which is essential for the correct functioning of the payment_is_probe function.
• 26-26: The impl_writeable_tlv_based_enum! macro is correctly used to implement serialization and deserialization for the PendingOutboundPayment enum. The macro's parameters are well-defined, and the use of required and optional fields is appropriate for the data structure.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.
• 26-26: The implementation of the OutboundPayments struct provides a wide range of methods for managing outbound payments, including sending payments with or without a predefined route, sending spontaneous payments, handling payment probes, and managing payment attempts. The methods are logically sound and well-documented, facilitating maintainability.

lightning/src/routing/gossip.rs (1)

• 1848-1848: The change from #[cfg(feature = "no-std")] to #[cfg(not(feature = "std"))] is correct and ensures that the enclosed code is compiled only when the standard library is not available, which aligns with the comment explaining the need for this block in a no-std environment.

lightning/src/ln/peer_handler.rs (1)

• 51-52: The removal of the #[cfg(not(c_bindings))] conditional compilation blocks and the associated imports for channelmanager and onion_message modules is noted. The new imports for SimpleArcChannelManager, SimpleRefChannelManager, SimpleArcOnionMessenger, and SimpleRefOnionMessenger have been added, which aligns with the PR's objective to enhance the testing framework.

lightning/src/ln/functional_test_utils.rs (12)

• 22-33: The imports and module usage have been updated to include OnionMessageHandler, IgnoringMessageHandler, and OnionMessenger. These changes are consistent with the PR's objective to enhance the testing framework for BOLT 12 Offers.
• 48-52: The additional imports from alloc, core, and crate are necessary for the new structures and functionality introduced in the testing framework.
• 394-394: The NodeCfg struct has been modified to include a message_router field, which aligns with the PR's goal to test the message routing functionality within the BOLT 12 Offers payment flow.
• 414-421: The definition of TestOnionMessenger is appropriate for the testing context, using DedicatedEntropy and other test-specific types to ensure consistent behavior during tests.
• 423-432: The DedicatedEntropy struct and its Deref implementation are well-defined, ensuring that the RandomBytes type can be used consistently within the OnionMessenger during tests.
• 442-442: The Node struct now includes an onion_messenger field, which is necessary for testing the onion message handling as part of the BOLT 12 Offers payment flow.
• 462-465: The init_features method in the Node struct has been updated to combine features from both the ChannelManager and OnionMessenger. This change is consistent with the need to test feature negotiation in the context of BOLT 12 Offers.
• 635-635: The NodeCfg struct is correctly modified to include the message_router field during the creation of a Node, which is necessary for the new testing framework.
• 1090-1090: The macro reload_node! has been updated to set the offers handler on the onion_messenger. This is a necessary change to ensure that the OnionMessenger is correctly configured after a node is reloaded in the test environment.
• 2934-2935: The create_network function now initializes the message_router with a new instance of TestMessageRouter. This change is necessary to support the testing of message routing within the BOLT 12 Offers payment flow.
• 2989-3001: The loop for creating nodes in the network now includes the initialization of OnionMessenger with DedicatedEntropy. This is consistent with the PR's objective to test the onion message handling and ensure deterministic behavior in the test environment.
• 3016-3033: The connection setup between nodes now includes the initialization of features and the connection of the OnionMessenger. This is necessary to test the interaction between nodes in the context of BOLT 12 Offers and onion message handling.

lightning/src/routing/scoring.rs (1)

• 254-254: The conditional compilation directive correctly includes the RwLock<T> implementation of LockableScore when not compiling for c_bindings, or when the _test_utils or test features are enabled.

lightning/src/routing/router.rs (5)

• 6905-6905: The change from #[cfg(not(feature = "no-std"))] to #[cfg(feature = "std")] is consistent with the PR objectives to update conditional compilation directives.
• 6915-6915: The update to the conditional compilation attribute for the generate_routes test function is correct and aligns with the standard Rust feature flags.
• 6936-6936: The conditional compilation attribute for the generate_routes_mpp test function has been correctly updated to #[cfg(feature = "std")].
• 6957-6957: The change in the conditional compilation attribute for the generate_large_mpp_routes test function is appropriate and follows the Rust conventions.
• 8293-8293: The modification of the conditional compilation attribute for the bench_utils module is consistent with the changes made in other parts of the file and adheres to the intended feature flag usage.

lightning/src/ln/functional_tests.rs (2)

• 5536-5538: The instantiation of TestRouter and TestMessageRouter and their inclusion in the NodeCfg struct are consistent with the PR's objective to enhance the testing framework.
• 5538-5538: The NodeCfg struct is correctly instantiated with the new message_router field, ensuring that the new testing components are integrated into the node configuration.

lightning/src/ln/channelmanager.rs (5)

• 7782-7790: The conditional compilation directives here ensure that the correct version of respond_using_derived_keys is called based on the "std" feature flag. This is a good practice to maintain compatibility with different environments.
• 9227-9237: Similar to the previous hunk, the conditional compilation directives are used to handle the presence of the "std" feature. The code correctly uses a timestamp from highest_seen_timestamp when the "std" feature is not present, ensuring consistent behavior across environments.
• 9246-9248: Again, the use of conditional compilation directives here is appropriate. The code ensures that the correct method is called for responding to an invoice request based on the compilation environment.
• 12490-12490: The import statements for Bitcoin types and synchronization primitives are correctly placed and are necessary for the subsequent code.
• 12530-12530: The initialization of TestRouter with a new NetworkGraph instance and the configuration of max_dust_htlc_exposure are done correctly. These changes are likely related to setting up the environment for the new tests.

[coderabbitai]

The handle_onion_message method does not check if _peer_node_id matches next_node_id before forwarding the onion message. This could potentially lead to forwarding a message to an incorrect peer.

if next_node_id != *_peer_node_id {
    log_trace!(self.logger, "Received onion message intended for another peer: {}", next_node_id);
    return;
}

[jkczyz]

Hmm... no, _peer_node_id is that node that sent the message, whereas next_node_id is where the message should be forwarded to next.

[coderabbitai]

@jkczyz Thank you for the clarification. I understand now that _peer_node_id is the sender of the message and next_node_id is the intended recipient for forwarding. No further action is required regarding the comparison between these two IDs.

[coderabbitai]

The RandomBytes struct is introduced to provide an entropy source, but it's not clear how the index is used to ensure uniqueness. The index should be incremented on each call to get_secure_random_bytes to ensure different values are produced each time. Please clarify the usage or ensure the implementation guarantees unique random values for each call.

[coderabbitai]

The clone method for InMemorySigner is using RandomBytes::new(self.get_secure_random_bytes()), which seems to be using the output of get_secure_random_bytes as a new seed. This could potentially lead to predictable randomness if the get_secure_random_bytes method does not guarantee a unique seed on each invocation. Please ensure that the seed used for RandomBytes is unique and unpredictable.

[coderabbitai]

The new method for InMemorySigner is also using RandomBytes::new(rand_bytes_unique_start), where rand_bytes_unique_start is derived from various parameters. It's crucial to ensure that this results in a unique and unpredictable seed for the RandomBytes instance. If there's any doubt about the uniqueness or randomness of this seed, it should be addressed to prevent potential security issues.

[coderabbitai]

The get_secure_random_bytes method in InMemorySigner is directly calling self.entropy_source.get_secure_random_bytes(). It's important to verify that entropy_source is indeed providing secure, random bytes and that the index is being handled correctly to avoid any reuse of random values.

[coderabbitai]

The conditional compilation directive #[cfg(all(feature = "std", test))] is used to include SinceEpoch only when both the "std" feature and testing are enabled. This may lead to compilation errors or unexpected behavior when running tests without the "std" feature. Consider whether this is the intended behavior or if the directive should be adjusted to #[cfg(any(feature = "std", test))] to include SinceEpoch for either case.

[valentinewallace]

Kinda unfortunate how brittle that test is. Opened #2827.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ea5de93 and 4532fb5.

Files selected for processing (18)

• lightning/src/ln/channelmanager.rs (5 hunks)
• lightning/src/ln/features.rs (1 hunks)
• lightning/src/ln/functional_test_utils.rs (11 hunks)
• lightning/src/ln/functional_tests.rs (1 hunks)
• lightning/src/ln/mod.rs (1 hunks)
• lightning/src/ln/offers_tests.rs (1 hunks)
• lightning/src/ln/outbound_payment.rs (11 hunks)
• lightning/src/ln/payment_tests.rs (2 hunks)
• lightning/src/ln/peer_handler.rs (2 hunks)
• lightning/src/offers/offer.rs (1 hunks)
• lightning/src/offers/refund.rs (1 hunks)
• lightning/src/onion_message/messenger.rs (5 hunks)
• lightning/src/routing/gossip.rs (1 hunks)
• lightning/src/routing/router.rs (5 hunks)
• lightning/src/routing/scoring.rs (1 hunks)
• lightning/src/sign/mod.rs (9 hunks)
• lightning/src/util/test_utils.rs (5 hunks)
• lightning/src/util/time.rs (3 hunks)

Files skipped from review as they are similar to previous changes (18)

• lightning/src/ln/channelmanager.rs
• lightning/src/ln/features.rs
• lightning/src/ln/functional_test_utils.rs
• lightning/src/ln/functional_tests.rs
• lightning/src/ln/mod.rs
• lightning/src/ln/offers_tests.rs
• lightning/src/ln/outbound_payment.rs
• lightning/src/ln/payment_tests.rs
• lightning/src/ln/peer_handler.rs
• lightning/src/offers/offer.rs
• lightning/src/offers/refund.rs
• lightning/src/onion_message/messenger.rs
• lightning/src/routing/gossip.rs
• lightning/src/routing/router.rs
• lightning/src/routing/scoring.rs
• lightning/src/sign/mod.rs
• lightning/src/util/test_utils.rs
• lightning/src/util/time.rs

[TheBlueMatt]

LGTM, only did a quick glance at the overall changes, didn't really review the tests themselves, I'll leave that to @valentinewallace.