Refactor PeerManager::get_peer_node_ids in favor of list_peers/peer_by_node_id returning additional information
#2905
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note Reviews PausedUse the following commands to manage reviews:
Warning Rate Limit Exceeded@tnull has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 15 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent update enhances the functionality of the Lightning network codebase, specifically focusing on peer management and node identification. These changes aim to improve the handling of peer connections and network initialization within the system. 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/peer_handler.rs (4 hunks)
Additional comments: 6
lightning/src/ln/peer_handler.rs (6)
- 968-982: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [961-979]
The implementation of
get_peer_node_idsmethod correctly retrieves the node IDs, network addresses, and init features of peers that have completed the initial handshake. This method is crucial for applications to discern the capabilities of their peers.
- 2750-2751: The usage of
init_featuresmethod to determine the features supported by the peer during the connection setup is correct. This ensures that the node can make informed decisions based on the capabilities of its peers.- 2773-2774: The assertions in the
establish_connectionfunction correctly verify that the peers have been added to each other'sget_peer_node_idslist with the expected network addresses and features. This ensures that the connection setup process completes successfully and both peers recognize each other.- 2750-2751: The tests provided in the file are comprehensive and cover a wide range of scenarios related to peer management and message handling. These tests are essential for ensuring the robustness and reliability of the
PeerManagerfunctionality.- 2750-2751: The
filter_addressesfunction correctly implements the filtering of local and private IP addresses based on IANA assignments. This is crucial for maintaining privacy and security by preventing the exposure of non-routable addresses.- 2750-2751: The usage of the
spawn_thread!macro in thefuzz_threaded_connectionsandtest_process_events_multithreadedtests is appropriate and simplifies the process of spawning threads for testing the multithreaded behavior of thePeerManager. This approach aids in testing the robustness of the peer management functionality under concurrent conditions.
tnull
force-pushed
the
2024-02-expose-init-features
branch
from
737a65f
to
5556830
Compare
|
@coderabbitai pause |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fuzz/src/full_stack.rs (1 hunks)
- lightning/src/ln/peer_handler.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/peer_handler.rs
Additional comments: 1
fuzz/src/full_stack.rs (1)
- 742-742: The change to include an additional element in the tuple extraction from a list for disconnecting nodes based on peer node IDs seems to align with the PR's objectives. However, it's crucial to ensure that all usages of this tuple pattern match are updated accordingly to prevent runtime errors.
wpaulino
reviewed
Feb 20, 2024
tnull
force-pushed
the
2024-02-expose-init-features
branch
from
5556830
to
4f14d81
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fuzz/src/full_stack.rs (1 hunks)
- lightning/src/ln/peer_handler.rs (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- fuzz/src/full_stack.rs
- lightning/src/ln/peer_handler.rs
tnull
changed the title
InitFeatures in PeerManager::get_peer_node_idsPeerManager::get_peer_node_ids in favor of list_peers/peer_by_node_id returning additional information
tnull
force-pushed
the
2024-02-expose-init-features
branch
from
4f14d81
to
5bd97df
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fuzz/src/full_stack.rs (1 hunks)
- lightning/src/ln/peer_handler.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- fuzz/src/full_stack.rs
Additional comments: 4
lightning/src/ln/peer_handler.rs (4)
- 434-452: The
PeerDetailsstruct has been updated to includeinit_featureswhich is a positive change as it allows for better insight into peer capabilities. This aligns with the PR's objective to enhance peer capability assessment.- 981-1033: The methods
list_peersandpeer_by_node_idhave been correctly updated to include the initialization features of peers. This is a crucial update for applications to determine peer capabilities, especially for operations like opening channels. The logic correctly handles the case where the handshake has not been completed, returningNoneas expected.- 2822-2833: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2802-2830]
The test
establish_connectioncorrectly asserts the inclusion ofinit_featuresinPeerDetailsfor both peers after establishing a connection. This ensures that the updatedPeerDetailsstruct is being correctly utilized and that the initialization features are being correctly propagated and accessed.
- 2822-2833: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2802-2830]
The changes in the tests and internal logic adjustments to accommodate the new
init_featuresinPeerDetailsare correctly implemented. The tests have been updated to reflect the new changes and ensure that the functionality works as expected.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2905 +/- ##
==========================================
- Coverage 89.12% 89.08% -0.04%
==========================================
Files 117 117
Lines 94244 94287 +43
Branches 94244 94287 +43
==========================================
+ Hits 83992 83998 +6
- Misses 7781 7808 +27
- Partials 2471 2481 +10 ☔ View full report in Codecov by Sentry. |
.. returning `PeerDetails` rather than tuples of peer-associated values. Previously, we wouldn't offer any way to retrieve the features a peer provided in their `Init` message. Here, we allow to retrieve them via a new `PeerDetails` struct, side-by-side with `SocketAddress`es and a bool indicating the direction of the peer connection.
tnull
force-pushed
the
2024-02-expose-init-features
branch
from
5bd97df
to
0c74cdc
Compare
|
@coderabbitai pause |
wpaulino
approved these changes
Feb 21, 2024
TheBlueMatt
approved these changes
Feb 22, 2024
k0k0ne
pushed a commit
to bitlightlabs/rust-lightning
that referenced
this pull request
Sep 30, 2024
v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we wouldn't offer any way to retrieve the features a peer provided in theirInitmessage.Here, we allow to retrieve them viaget_peer_node_ids, similar to theSocketAddresses.Here, we introduce
PeerManager::list_peersandpeer_by_node_idmethods returningPeerDetailsrather than tuples of peer-associated values.Previously, we wouldn't offer any way to retrieve the features a peer provided in their
Initmessage. Here, we allow to retrieve them via a newPeerDetailsstruct, side-by-side withSocketAddresses and a bool indicating the directionof the peer connection.
For context: This is useful for all kinds of applications, however, most recently I found that it's required for maintaining an Anchor emergency reserve upfront, as we otherwise can't tell if the counterparty supports Anchors before initiating the actual channel open (at least assuming they are not necessarily in the public graph and/or we source our graph data via RGS, in which case we won't have access to their
NodeAnnouncement).