Fix (and DRY) the conditionals before calling peer_disconnected #2035
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
Codecov ReportBase: 87.23% // Head: 87.90% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2035 +/- ##
==========================================
+ Coverage 87.23% 87.90% +0.67%
==========================================
Files 100 102 +2
Lines 44117 50424 +6307
Branches 44117 50424 +6307
==========================================
+ Hits 38486 44326 +5840
- Misses 5631 6098 +467
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
08ab1bb
to
be815e2
Compare
jkczyz
reviewed
Feb 16, 2023
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
be815e2
to
360ced3
Compare
jkczyz
reviewed
Feb 17, 2023
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
360ced3
to
6bcfb11
Compare
wpaulino
reviewed
Feb 17, 2023
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
6bcfb11
to
c3e1678
Compare
wpaulino
reviewed
Feb 17, 2023
| /// handler methods. Thus, this implies we've finished our handshake and can talk to this peer | ||
| /// normally. | ||
| fn handshake_complete(&self) -> bool { | ||
| self.their_features.is_some() |
There was a problem hiding this comment.
Is it worth tracking whether we've sent our Init message as well? Is there a case where a gossip message we're broadcasting to all our peers could be queued before we queue our Init?
There was a problem hiding this comment.
We always send our init when we finish the handshake (ie before we receive their init), so its currently no issue.
There was a problem hiding this comment.
Didn't know that was allowed if they initiated the connection? I must be confused and should read the bolt again lol.
There was a problem hiding this comment.
It may tell you to do something different, but it doesn't matter - we've already exchanged the crypto handshake, so both sides have sent a good chunk of bytes, and they send theirs right after the handshake completes from their perspective, which is before it does from our perspective.
|
Looks good for squash at least. |
If we have a peer that sends a non-`Init` first message, we'll call `peer_disconnected` without ever having called `peer_connected` (which has to wait until we have an `Init` message). This is a violation of our API guarantees, though should generally not be an issue. Because this bug was repeated in a few places, we also take this opportunity to DRY up the logic which checks the peer state before calling `peer_disconnected`. Found by the new `ChannelManager` assertions and the `full_stack_target` fuzzer.
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
c3e1678
to
bbd073a
Compare
|
Squashed without further changes. |
jkczyz
reviewed
Feb 21, 2023
This test fails without the previous commit.
In general, we should be checking if a `Peer` has `their_features` set as the "is this peer connected and have they finished the handshake" flag as it indicates an `Init` message was received. While none of these appear to be reachable bugs, there were a number of places where we checked other flags for this purpose, which may lead to sending messages before `Init` in the future. Here we clean these cases up to always use the correct check (via the new util method).
This fixes new errors in `full_stack_target` pointed out by Chaincode's generous fuzzing infrastructure. Specifically, there's no reason to check the error message in the `funding_transaction_generated` return value - it can only return a failure if the channel has closed since the funding transaction was generated (which is fine) or if the signer refuses to sign (which can't happen in fuzzing).
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
bbd073a
to
f97a450
Compare
Long ago, we used the `no_connection_possible` to signal that a peer has some unknown feature set or some other condition prevents us from ever connecting to the given peer. In that case we'd automatically force-close all channels with the given peer. This was somewhat surprising to users so we removed the automatic force-close, leaving the flag serving no LDK-internal purpose. Distilling the concept of "can we connect to this peer again in the future" to a simple flag turns out to be ripe with edge cases, so users actually using the flag to force-close channels would likely cause surprising behavior. Thus, there's really not a lot of reason to keep the flag, especially given its untested and likely to be broken in subtle ways anyway.
TheBlueMatt
force-pushed
the
2023-02-fix-no-con-discon
branch
from
f97a450
to
be6f263
Compare
jkczyz
approved these changes
Feb 21, 2023
wpaulino
approved these changes
Feb 21, 2023
k0k0ne
pushed a commit
to bitlightlabs/rust-lightning
that referenced
this pull request
Sep 30, 2024
0.0.114 - Mar 3, 2023 - "Faster Async BOLT12 Retries" API Updates =========== * `InvoicePayer` has been removed and its features moved directly into `ChannelManager`. As such it now requires a simplified `Router` and supports `send_payment_with_retry` (and friends). `ChannelManager::retry_payment` was removed in favor of the automated retries. Invoice payment utilities in `lightning-invoice` now call the new code (lightningdevkit#1812, lightningdevkit#1916, lightningdevkit#1929, lightningdevkit#2007, etc). * `Sign`/`BaseSign` has been renamed `ChannelSigner`, with `EcdsaChannelSigner` split out in anticipation of future schnorr/taproot support (lightningdevkit#1967). * The catch-all `KeysInterface` was split into `EntropySource`, `NodeSigner`, and `SignerProvider`. `KeysManager` implements all three (lightningdevkit#1910, lightningdevkit#1930). * `KeysInterface::get_node_secret` is now `KeysManager::get_node_secret_key` and is no longer required for external signers (lightningdevkit#1951, lightningdevkit#2070). * A `lightning-transaction-sync` crate has been added which implements keeping LDK in sync with the chain via an esplora server (lightningdevkit#1870). Note that it can only be used on nodes that *never* ran a previous version of LDK. * `Score` is updated in `BackgroundProcessor` instead of via `Router` (lightningdevkit#1996). * `ChainAccess::get_utxo` (now `UtxoAccess`) can now be resolved async (lightningdevkit#1980). * BOLT12 `Offer`, `InvoiceRequest`, `Invoice` and `Refund` structs as well as associated builders have been added. Such invoices cannot yet be paid due to missing support for blinded path payments (lightningdevkit#1927, lightningdevkit#1908, lightningdevkit#1926). * A `lightning-custom-message` crate has been added to make combining multiple custom messages into one enum/handler easier (lightningdevkit#1832). * `Event::PaymentPathFailure` is now generated for failure to send an HTLC over the first hop on our local channel (lightningdevkit#2014, lightningdevkit#2043). * `lightning-net-tokio` no longer requires an `Arc` on `PeerManager` (lightningdevkit#1968). * `ChannelManager::list_recent_payments` was added (lightningdevkit#1873). * `lightning-background-processor` `std` is now optional in async mode (lightningdevkit#1962). * `create_phantom_invoice` can now be used in `no-std` (lightningdevkit#1985). * The required final CLTV delta on inbound payments is now configurable (lightningdevkit#1878) * bitcoind RPC error code and message are now surfaced in `block-sync` (lightningdevkit#2057). * Get `historical_estimated_channel_liquidity_probabilities` was added (lightningdevkit#1961). * `ChannelManager::fail_htlc_backwards_with_reason` was added (lightningdevkit#1948). * Macros which implement serialization using TLVs or straight writing of struct fields are now public (lightningdevkit#1823, lightningdevkit#1976, lightningdevkit#1977). Backwards Compatibility ======================= * Any inbound payments with a custom final CLTV delta will be rejected by LDK if you downgrade prior to receipt (lightningdevkit#1878). * `Event::PaymentPathFailed::network_update` will always be `None` if an 0.0.114-generated event is read by a prior version of LDK (lightningdevkit#2043). * `Event::PaymentPathFailed::all_paths_removed` will always be false if an 0.0.114-generated event is read by a prior version of LDK. Users who rely on it to determine payment retries should migrate to `Event::PaymentFailed`, in a separate release prior to upgrading to LDK 0.0.114 if downgrading is supported (lightningdevkit#2043). Performance Improvements ======================== * Channel data is now stored per-peer and channel updates across multiple peers can be operated on simultaneously (lightningdevkit#1507). * Routefinding is roughly 1.5x faster (lightningdevkit#1799). * Deserializing a `NetworkGraph` is roughly 6x faster (lightningdevkit#2016). * Memory usage for a `NetworkGraph` has been reduced substantially (lightningdevkit#2040). * `KeysInterface::get_secure_random_bytes` is roughly 200x faster (lightningdevkit#1974). Bug Fixes ========= * Fixed a bug where a delay in processing a `PaymentSent` event longer than the time taken to persist a `ChannelMonitor` update, when occurring immediately prior to a crash, may result in the `PaymentSent` event being lost (lightningdevkit#2048). * Fixed spurious rejections of rapid gossip sync data when the graph has been updated by other means between gossip syncs (lightningdevkit#2046). * Fixed a panic in `KeysManager` when the high bit of `starting_time_nanos` is set (lightningdevkit#1935). * Resolved an issue where the `ChannelManager::get_persistable_update_future` future would fail to wake until a second notification occurs (lightningdevkit#2064). * Resolved a memory leak when using `ChannelManager::send_probe` (lightningdevkit#2037). * Fixed a deadlock on some platforms at least when using async `ChannelMonitor` updating (lightningdevkit#2006). * Removed debug-only assertions which were reachable in threaded code (lightningdevkit#1964). * In some cases when payment sending fails on our local channel retries no longer take the same path and thus never succeed (lightningdevkit#2014). * Retries for spontaneous payments have been fixed (lightningdevkit#2002). * Return an `Err` if `lightning-persister` fails to read the directory listing rather than panicing (lightningdevkit#1943). * `peer_disconnected` will now never be called without `peer_connected` (lightningdevkit#2035) Security ======== 0.0.114 fixes several denial-of-service vulnerabilities which are reachable from untrusted input from channel counterparties or in deployments accepting inbound connections or channels. It also fixes a denial-of-service vulnerability in rare cases in the route finding logic. * The number of pending un-funded channels as well as peers without funded channels is now limited to avoid denial of service (lightningdevkit#1988). * A second `channel_ready` message received immediately after the first could lead to a spurious panic (lightningdevkit#2071). This issue was introduced with 0conf support in LDK 0.0.107. * A division-by-zero issue was fixed in the `ProbabilisticScorer` if the amount being sent (including previous-hop fees) is equal to a channel's capacity while walking the graph (lightningdevkit#2072). The division-by-zero was introduced with historical data tracking in LDK 0.0.112. In total, this release features 130 files changed, 21457 insertions, 10113 deletions in 343 commits from 18 authors, in alphabetical order: * Alec Chen * Allan Douglas R. de Oliveira * Andrei * Arik Sosman * Daniel Granhão * Duncan Dean * Elias Rohrer * Jeffrey Czyz * John Cantrell * Kurtsley * Matt Corallo * Max Fang * Omer Yacine * Valentine Wallace * Viktor Tigerström * Wilmer Paulino * benthecarman * jurvis
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.
If we have a peer that sends a non-
Initfirst message, we'll callpeer_disconnectedwithout ever having calledpeer_connected(which has to wait until we have an
Initmessage). This is aviolation of our API guarantees, though should generally not be an
issue.
Because this bug was repeated in a few places, we also take this
opportunity to DRY up the logic which checks the peer state before
calling
peer_disconnected.Found by the new
ChannelManagerassertions and thefull_stack_targetfuzzer.