diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 83ae38a1b9e..7077cb7c55b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -179,29 +179,6 @@ jobs: cd bench RUSTFLAGS="--cfg=ldk_bench --cfg=require_route_graph_test" cargo bench - check_commits: - runs-on: ubuntu-latest - env: - TOOLCHAIN: stable - steps: - - name: Checkout source code - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Install Rust ${{ env.TOOLCHAIN }} toolchain - run: | - curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile=minimal --default-toolchain ${{ env.TOOLCHAIN }} - rustup override set ${{ env.TOOLCHAIN }} - - name: Fetch full tree and rebase on upstream - run: | - git remote add upstream https://github.com/lightningdevkit/rust-lightning - git fetch upstream - export GIT_COMMITTER_EMAIL="rl-ci@example.com" - export GIT_COMMITTER_NAME="RL CI" - git rebase upstream/main - - name: For each commit, run cargo check (including in fuzz) - run: ci/check-each-commit.sh upstream/main - check_release: runs-on: ubuntu-latest env: @@ -231,6 +208,24 @@ jobs: RUSTFLAGS: '--cfg=taproot' RUSTDOCFLAGS: '--cfg=taproot' + check_docs: + runs-on: self-hosted + env: + # While docs.rs builds using a nightly compiler (and we use some nightly features), + # nightly ends up randomly breaking builds occasionally, so we instead use beta + # and set RUSTC_BOOTSTRAP in check-docsrs.sh + TOOLCHAIN: beta + steps: + - name: Checkout source code + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Install Rust ${{ env.TOOLCHAIN }} toolchain + run: | + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile=minimal --default-toolchain ${{ env.TOOLCHAIN }} + - name: Simulate docs.rs build + run: ci/check-docsrs.sh + fuzz: runs-on: ubuntu-latest env: @@ -291,20 +286,3 @@ jobs: rustup component add rustfmt - name: Run rustfmt checks run: ci/rustfmt.sh - - incremental-mutants: - runs-on: ubuntu-latest - if: github.ref_name != 'main' # `main` has no diff with itself - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Relative diff - run: | - git branch -av - git diff origin/main.. | tee git.diff - - uses: Swatinem/rust-cache@v2 - - name: Mutants - run: | - cargo install cargo-mutants - cargo mutants --no-shuffle -j 2 -vV --in-diff git.diff diff --git a/.github/workflows/check_commits.yml b/.github/workflows/check_commits.yml new file mode 100644 index 00000000000..2fb44f669c6 --- /dev/null +++ b/.github/workflows/check_commits.yml @@ -0,0 +1,34 @@ +name: CI check_commits + +on: + pull_request: + branches-ignore: + - master + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + check_commits: + runs-on: ubuntu-latest + env: + TOOLCHAIN: stable + steps: + - name: Checkout source code + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Install Rust ${{ env.TOOLCHAIN }} toolchain + run: | + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile=minimal --default-toolchain ${{ env.TOOLCHAIN }} + rustup override set ${{ env.TOOLCHAIN }} + - name: Fetch full tree and rebase on upstream + run: | + git remote add upstream https://github.com/lightningdevkit/rust-lightning + git fetch upstream + export GIT_COMMITTER_EMAIL="rl-ci@example.com" + export GIT_COMMITTER_NAME="RL CI" + git rebase upstream/${{ github.base_ref }} + - name: For each commit, run cargo check (including in fuzz) + run: ci/check-each-commit.sh upstream/${{ github.base_ref }} diff --git a/CHANGELOG.md b/CHANGELOG.md index e5edcd8eab6..1ee4b667635 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,60 @@ -# 0.1.5 - Jul XXX, 2025 - "Async Path Reduction" +# 0.1.8 - Dec 2, 2025 - "Async Update Completion" + +## Bug Fixes + * In cases where an MPP payment is claimed while one channel is waiting on a + counterparty's `revoke_and_ack` message and the `revoke_and_ack` message is + received prior to the asynchronous completion of the MPP-claim + `ChannelMonitorUpdate`, the channel will no longer hang (#4236). + * Deserializing invalid `Duration`s can no longer panic (#4172). + + +# 0.1.7 - Oct 21, 2025 - "Unstable Release CI" + +## Bug Fixes + * Builds with the `docsrs` cfg flag (set automatically for builds on docs.rs + but otherwise not used) were fixed. + + +# 0.1.6 - Oct 10, 2025 - "Async Preimage Claims" + +## Performance Improvements + * `NetworkGraph::remove_stale_channels_and_tracking` has been sped up by more + than 20x in cases where many entries need to be removed (such as after + initial gossip sync, #4080). + +## Bug Fixes + * Delivery of on-chain resolutions of HTLCs to `ChannelManager` has been made + more robust to prevent loss in some exceedingly rare crash cases. This may + marginally increase payment resolution event replays on startup (#3984). + * Corrected forwarding of new gossip to peers which we are sending an initial + gossip sync to (#4107). + * A rare race condition may have resulted in outbound BOLT12 payments + spuriously failing while processing the `Bolt12Invoice` message (#4078). + * If a channel is updated multiple times after a payment is claimed while using + async persistence of the `ChannelMonitorUpdate`s, and the node then restarts + with a stale copy of its `ChannelManager`, the `PaymentClaimed` may have been + lost (#3988). + * If an async-persisted `ChannelMonitorUpdate` for one part of an MPP claim + does not complete before multiple `ChannelMonitorUpdate`s for another channel + in the same MPP claim complete, and the node restarts twice, the preimage may + be lost and the MPP payment part may not be claimed (#3928). + +## Security +0.1.6 fixes a denial of service vulnerability and a funds-theft vulnerability. + * When a channel has been force-closed, we have already claimed some of its + HTLCs on-chain, and we later learn a new preimage allowing us to claim + further HTLCs on-chain, we could in some cases generate invalid claim + transactions leading to loss of funds (#4154). + * When a `ChannelMonitor` is created for a channel which is never funded with + a real transaction, `ChannelMonitor::get_claimable_balances` would never be + empty. As a result, `ChannelMonitor::check_and_update_full_resolution_status` + would never indicate the monitor is prunable, and thus + `ChainMonitor::archive_fully_resolved_channel_monitors` would never remove + it. This allows a peer which opens channels without funding them to bloat our + memory and disk space, eventually leading to denial-of-service (#4081). + + +# 0.1.5 - Jul 16, 2025 - "Async Path Reduction" ## Performance Improvements * `NetworkGraph`'s expensive internal consistency checks have now been diff --git a/ci/check-docsrs.sh b/ci/check-docsrs.sh new file mode 100755 index 00000000000..b8776f8ac57 --- /dev/null +++ b/ci/check-docsrs.sh @@ -0,0 +1,42 @@ +#!/bin/bash +#shellcheck disable=SC2002,SC2086,SC2207 + +set -ex + +# Attempt to simulate the docsrs builds. Sadly its not entirely trivial as +# docs.rs reads metadata out of Cargo.toml which we don't want to have a whole +# parser for. + +WORKSPACE_MEMBERS=( $(cat Cargo.toml | tr '\n' '\r' | sed 's/\r //g' | tr '\r' '\n' | grep '^members =' | sed 's/members.*=.*\[//' | tr -d '"' | tr ',' '\n') ) +echo "${WORKSPACE_MEMBERS[@]}" +for CRATE in "${WORKSPACE_MEMBERS[@]}"; do + pushd "$CRATE" + CARGO_ARGS="" + RUSTDOC_ARGS="" + cat Cargo.toml | grep -A 100 '\[package.metadata.docs.rs\]' | tail -n +2 > /tmp/ldk-docsrs-rustdoc-config.txt + while read -r LINE; do + case "$LINE" in + "["*) break;; + "features"*) + OG_IFS="$IFS" + IFS=',' + for FEATURE in $(echo "$LINE" | sed 's/features.*=.*\[//g' | tr -d '"] '); do + export CARGO_ARGS="$CARGO_ARGS --features $FEATURE" + done + IFS="$OG_IFS" + ;; + "all-features = true") + export CARGO_ARGS="$CARGO_ARGS --all-features" + ;; + "rustdoc-args"*) + RUSTDOC_ARGS="$(echo "$LINE" | sed 's/rustdoc-args.*=.*\[//g' | tr -d '"],')" + ;; + esac + done < /tmp/ldk-docsrs-rustdoc-config.txt + rm /tmp/ldk-docsrs-rustdoc-config.txt + echo "Building $CRATE with args $CARGO_ARGS and flags $RUSTDOC_ARGS" + # We rely on nightly features but want to use a stable release in CI to avoid + # spurous breakage, thus we set RUSTC_BOOTSTRAP=1 here. + RUSTC_BOOTSTRAP=1 cargo rustdoc $CARGO_ARGS -- $RUSTDOC_ARGS + popd +done diff --git a/ci/ci-tests.sh b/ci/ci-tests.sh index 20ecd0fa2c9..137f2c53d63 100755 --- a/ci/ci-tests.sh +++ b/ci/ci-tests.sh @@ -1,4 +1,5 @@ #!/bin/bash +#shellcheck disable=SC2002,SC2207 set -eox pipefail RUSTC_MINOR_VERSION=$(rustc --version | awk '{ split($2,a,"."); print a[2] }') @@ -7,6 +8,12 @@ RUSTC_MINOR_VERSION=$(rustc --version | awk '{ split($2,a,"."); print a[2] }') # which we do here. # Further crates which appear only as dev-dependencies are pinned further down. function PIN_RELEASE_DEPS { + # Starting with version 2.0.107, the `syn` crate has an MSRV of rustc 1.68 + [ "$RUSTC_MINOR_VERSION" -lt 68 ] && cargo update -p syn --precise "2.0.106" --verbose + + # Starting with version 1.0.42, the `quote` crate has an MSRV of rustc 1.68 + [ "$RUSTC_MINOR_VERSION" -lt 68 ] && cargo update -p quote --precise "1.0.41" --verbose + # Starting with version 1.39.0, the `tokio` crate has an MSRV of rustc 1.70.0 [ "$RUSTC_MINOR_VERSION" -lt 70 ] && cargo update -p tokio --precise "1.38.1" --verbose @@ -27,28 +34,21 @@ PIN_RELEASE_DEPS # pin the release dependencies in our main workspace # proptest 1.3.0 requires rustc 1.64.0 [ "$RUSTC_MINOR_VERSION" -lt 64 ] && cargo update -p proptest --precise "1.2.0" --verbose +# parking_lot 0.12.4 requires rustc 1.64.0 +[ "$RUSTC_MINOR_VERSION" -lt 64 ] && cargo update -p parking_lot --precise "0.12.3" --verbose + +# parking_lot_core 0.9.11 requires rustc 1.64.0 +[ "$RUSTC_MINOR_VERSION" -lt 64 ] && cargo update -p parking_lot_core --precise "0.9.10" --verbose + +# lock_api 0.4.13 requires rustc 1.64.0 +[ "$RUSTC_MINOR_VERSION" -lt 64 ] && cargo update -p lock_api --precise "0.4.12" --verbose + export RUST_BACKTRACE=1 echo -e "\n\nChecking the workspace, except lightning-transaction-sync." cargo check --verbose --color always -# When the workspace members change, make sure to update the list here as well -# as in `Cargo.toml`. -WORKSPACE_MEMBERS=( - lightning - lightning-types - lightning-block-sync - lightning-invoice - lightning-net-tokio - lightning-persister - lightning-background-processor - lightning-rapid-gossip-sync - lightning-custom-message - lightning-macros - lightning-dns-resolver - lightning-liquidity - possiblyrandom -) +WORKSPACE_MEMBERS=( $(cat Cargo.toml | tr '\n' '\r' | sed 's/\r //g' | tr '\r' '\n' | grep '^members =' | sed 's/members.*=.*\[//' | tr -d '"' | tr ',' ' ') ) echo -e "\n\nChecking, testing, and building docs for all workspace members individually..." for DIR in "${WORKSPACE_MEMBERS[@]}"; do @@ -59,8 +59,11 @@ done echo -e "\n\nTesting upgrade from prior versions of LDK" pushd lightning-tests +[ "$RUSTC_MINOR_VERSION" -lt 68 ] && cargo update -p syn --precise "2.0.106" --verbose +[ "$RUSTC_MINOR_VERSION" -lt 68 ] && cargo update -p quote --precise "1.0.41" --verbose [ "$RUSTC_MINOR_VERSION" -lt 65 ] && cargo update -p regex --precise "1.9.6" --verbose cargo test +[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean popd echo -e "\n\nChecking and testing Block Sync Clients with features" diff --git a/ci/ci-tx-sync-tests.sh b/ci/ci-tx-sync-tests.sh index 3ca2fae6725..5f926a8e37b 100755 --- a/ci/ci-tx-sync-tests.sh +++ b/ci/ci-tx-sync-tests.sh @@ -17,6 +17,9 @@ PIN_RELEASE_DEPS # pin the release dependencies # Starting with version 0.5.11, the `home` crate has an MSRV of rustc 1.81.0. [ "$RUSTC_MINOR_VERSION" -lt 81 ] && cargo update -p home --precise "0.5.9" --verbose +# Starting with version 1.2.0, the `idna_adapter` crate has an MSRV of rustc 1.81.0. +[ "$RUSTC_MINOR_VERSION" -lt 81 ] && cargo update -p idna_adapter --precise "1.1.0" --verbose + export RUST_BACKTRACE=1 echo -e "\n\nChecking Transaction Sync Clients with features." diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index 2e3b3e17f56..9ec30ffd740 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -5,7 +5,7 @@ #![deny(rustdoc::private_intra_doc_links)] #![deny(missing_docs)] #![cfg_attr(not(feature = "futures"), deny(unsafe_code))] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #![cfg_attr(all(not(feature = "std"), not(test)), no_std)] #[cfg(any(test, feature = "std"))] diff --git a/lightning-block-sync/src/lib.rs b/lightning-block-sync/src/lib.rs index 3f981cd8786..22c05ed6a12 100644 --- a/lightning-block-sync/src/lib.rs +++ b/lightning-block-sync/src/lib.rs @@ -17,7 +17,7 @@ #![deny(rustdoc::private_intra_doc_links)] #![deny(missing_docs)] #![deny(unsafe_code)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #[cfg(any(feature = "rest-client", feature = "rpc-client"))] pub mod http; diff --git a/lightning-invoice/src/lib.rs b/lightning-invoice/src/lib.rs index 9a99d05929b..79c9620fc2b 100644 --- a/lightning-invoice/src/lib.rs +++ b/lightning-invoice/src/lib.rs @@ -5,7 +5,7 @@ #![deny(non_camel_case_types)] #![deny(non_snake_case)] #![deny(unused_mut)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #![cfg_attr(all(not(feature = "std"), not(test)), no_std)] //! This crate provides data structures to represent diff --git a/lightning-liquidity/src/lib.rs b/lightning-liquidity/src/lib.rs index 909590eac96..73189e1f3ee 100644 --- a/lightning-liquidity/src/lib.rs +++ b/lightning-liquidity/src/lib.rs @@ -45,7 +45,7 @@ #![allow(bare_trait_objects)] #![allow(ellipsis_inclusive_range_patterns)] #![allow(clippy::drop_non_drop)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #![cfg_attr(not(feature = "std"), no_std)] #[macro_use] diff --git a/lightning-macros/src/lib.rs b/lightning-macros/src/lib.rs index fd4707e5856..62fbd316087 100644 --- a/lightning-macros/src/lib.rs +++ b/lightning-macros/src/lib.rs @@ -16,7 +16,7 @@ #![forbid(unsafe_code)] #![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::private_intra_doc_links)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] use proc_macro::TokenStream; use quote::quote; diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs index 944033102c6..a15700e60f6 100644 --- a/lightning-net-tokio/src/lib.rs +++ b/lightning-net-tokio/src/lib.rs @@ -25,7 +25,7 @@ #![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::private_intra_doc_links)] #![deny(missing_docs)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] use bitcoin::secp256k1::PublicKey; diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index 9d4df264d24..0e3541e1b27 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -3,7 +3,7 @@ #![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::private_intra_doc_links)] #![deny(missing_docs)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #[cfg(ldk_bench)] extern crate criterion; diff --git a/lightning-transaction-sync/src/lib.rs b/lightning-transaction-sync/src/lib.rs index dc5e52f1e45..05db6a063c7 100644 --- a/lightning-transaction-sync/src/lib.rs +++ b/lightning-transaction-sync/src/lib.rs @@ -69,7 +69,7 @@ #![deny(rustdoc::private_intra_doc_links)] #![deny(missing_docs)] #![deny(unsafe_code)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #[cfg(any(feature = "esplora-blocking", feature = "esplora-async"))] pub mod esplora; diff --git a/lightning-types/src/lib.rs b/lightning-types/src/lib.rs index 1539401d383..49e7e59084e 100644 --- a/lightning-types/src/lib.rs +++ b/lightning-types/src/lib.rs @@ -18,7 +18,7 @@ #![forbid(unsafe_code)] #![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::private_intra_doc_links)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] extern crate alloc; extern crate core; diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml index 75835c92edc..8644864c78c 100644 --- a/lightning/Cargo.toml +++ b/lightning/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning" -version = "0.1.5" +version = "0.1.8" authors = ["Matt Corallo"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning/" diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 467f2500eae..b59977daa95 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -59,7 +59,7 @@ use crate::prelude::*; use core::{cmp, mem}; use crate::io::{self, Error}; use core::ops::Deref; -use crate::sync::{Mutex, LockTestExt}; +use crate::sync::Mutex; /// An update generated by the underlying channel itself which contains some new information the /// [`ChannelMonitor`] should be made aware of. @@ -567,6 +567,20 @@ pub(crate) enum ChannelMonitorUpdateStep { ShutdownScript { scriptpubkey: ScriptBuf, }, + /// When a payment is finally resolved by the user handling an [`Event::PaymentSent`] or + /// [`Event::PaymentFailed`] event, the `ChannelManager` no longer needs to hear about it on + /// startup (which would cause it to re-hydrate the payment information even though the user + /// already learned about the payment's result). + /// + /// This will remove the HTLC from [`ChannelMonitor::get_all_current_outbound_htlcs`] and + /// [`ChannelMonitor::get_onchain_failed_outbound_htlcs`]. + /// + /// Note that this is only generated for closed channels as this is implicit in the + /// [`Self::CommitmentSecret`] update which clears the payment information from all un-revoked + /// counterparty commitment transactions. + ReleasePaymentComplete { + htlc: SentHTLCId, + }, } impl ChannelMonitorUpdateStep { @@ -578,6 +592,7 @@ impl ChannelMonitorUpdateStep { ChannelMonitorUpdateStep::CommitmentSecret { .. } => "CommitmentSecret", ChannelMonitorUpdateStep::ChannelForceClosed { .. } => "ChannelForceClosed", ChannelMonitorUpdateStep::ShutdownScript { .. } => "ShutdownScript", + ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => "ReleasePaymentComplete", } } } @@ -612,6 +627,9 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, (5, ShutdownScript) => { (0, scriptpubkey, required), }, + (7, ReleasePaymentComplete) => { + (1, htlc, required), + }, ); /// Indicates whether the balance is derived from a cooperative close, a force-close @@ -1000,6 +1018,14 @@ pub(crate) struct ChannelMonitorImpl { /// spending CSV for revocable outputs). htlcs_resolved_on_chain: Vec, + /// When a payment is resolved through an on-chain transaction, we tell the `ChannelManager` + /// about this via [`ChannelMonitor::get_onchain_failed_outbound_htlcs`] and + /// [`ChannelMonitor::get_all_current_outbound_htlcs`] at startup. We'll keep repeating the + /// same payments until they're eventually fully resolved by the user processing a + /// `PaymentSent` or `PaymentFailed` event, at which point the `ChannelManager` will inform of + /// this and we'll store the set of fully resolved payments here. + htlcs_resolved_to_user: HashSet, + /// The set of `SpendableOutput` events which we have already passed upstream to be claimed. /// These are tracked explicitly to ensure that we don't generate the same events redundantly /// if users duplicatively confirm old transactions. Specifically for transactions claiming a @@ -1040,12 +1066,21 @@ pub type TransactionOutputs = (Txid, Vec<(u32, TxOut)>); impl PartialEq for ChannelMonitor where Signer: PartialEq { fn eq(&self, other: &Self) -> bool { + use crate::sync::LockTestExt; // We need some kind of total lockorder. Absent a better idea, we sort by position in // memory and take locks in that order (assuming that we can't move within memory while a // lock is held). let ord = ((self as *const _) as usize) < ((other as *const _) as usize); - let a = if ord { self.inner.unsafe_well_ordered_double_lock_self() } else { other.inner.unsafe_well_ordered_double_lock_self() }; - let b = if ord { other.inner.unsafe_well_ordered_double_lock_self() } else { self.inner.unsafe_well_ordered_double_lock_self() }; + let a = if ord { + self.inner.unsafe_well_ordered_double_lock_self() + } else { + other.inner.unsafe_well_ordered_double_lock_self() + }; + let b = if ord { + other.inner.unsafe_well_ordered_double_lock_self() + } else { + self.inner.unsafe_well_ordered_double_lock_self() + }; a.eq(&b) } } @@ -1246,6 +1281,7 @@ impl Writeable for ChannelMonitorImpl { (21, self.balances_empty_height, option), (23, self.holder_pays_commitment_tx_fee, option), (25, self.payment_preimages, required), + (33, self.htlcs_resolved_to_user, required), }); Ok(()) @@ -1449,6 +1485,7 @@ impl ChannelMonitor { funding_spend_confirmed: None, confirmed_commitment_tx_counterparty_output: None, htlcs_resolved_on_chain: Vec::new(), + htlcs_resolved_to_user: new_hash_set(), spendable_txids_confirmed: Vec::new(), best_block, @@ -2041,6 +2078,16 @@ impl ChannelMonitor { let current_height = self.current_best_block().height; let mut inner = self.inner.lock().unwrap(); + if inner.is_closed_without_updates() + && is_all_funds_claimed + && !inner.funding_spend_seen + { + // We closed the channel without ever advancing it and didn't have any funds in it. + // We should immediately archive this monitor as there's nothing for us to ever do with + // it. + return (true, false); + } + if is_all_funds_claimed && !inner.funding_spend_seen { debug_assert!(false, "We should see funding spend by the time a monitor clears out"); is_all_funds_claimed = false; @@ -2500,18 +2547,28 @@ impl ChannelMonitor { } } } - res.push(Balance::ClaimableOnChannelClose { - amount_satoshis: us.current_holder_commitment_tx.to_self_value_sat + claimable_inbound_htlc_value_sat, - transaction_fee_satoshis: if us.holder_pays_commitment_tx_fee.unwrap_or(true) { - chan_utils::commit_tx_fee_sat( - us.current_holder_commitment_tx.feerate_per_kw, nondust_htlc_count, - us.onchain_tx_handler.channel_type_features()) - } else { 0 }, - outbound_payment_htlc_rounded_msat, - outbound_forwarded_htlc_rounded_msat, - inbound_claiming_htlc_rounded_msat, - inbound_htlc_rounded_msat, - }); + // Only push a primary balance if either the channel isn't closed or we've advanced the + // channel state machine at least once (implying there are multiple previous commitment + // transactions) or we actually have a balance. + // Avoiding including a `Balance` if none of these are true allows us to prune monitors + // for chanels that were opened inbound to us but where the funding transaction never + // confirmed at all. + let amount_satoshis = + us.current_holder_commitment_tx.to_self_value_sat + claimable_inbound_htlc_value_sat; + if !us.is_closed_without_updates() || amount_satoshis != 0 { + res.push(Balance::ClaimableOnChannelClose { + amount_satoshis, + transaction_fee_satoshis: if us.holder_pays_commitment_tx_fee.unwrap_or(true) { + chan_utils::commit_tx_fee_sat( + us.current_holder_commitment_tx.feerate_per_kw, nondust_htlc_count, + us.onchain_tx_handler.channel_type_features()) + } else { 0 }, + outbound_payment_htlc_rounded_msat, + outbound_forwarded_htlc_rounded_msat, + inbound_claiming_htlc_rounded_msat, + inbound_htlc_rounded_msat, + }); + } } res @@ -2520,118 +2577,152 @@ impl ChannelMonitor { /// Gets the set of outbound HTLCs which can be (or have been) resolved by this /// `ChannelMonitor`. This is used to determine if an HTLC was removed from the channel prior /// to the `ChannelManager` having been persisted. - /// - /// This is similar to [`Self::get_pending_or_resolved_outbound_htlcs`] except it includes - /// HTLCs which were resolved on-chain (i.e. where the final HTLC resolution was done by an - /// event from this `ChannelMonitor`). - pub(crate) fn get_all_current_outbound_htlcs(&self) -> HashMap)> { + pub(crate) fn get_all_current_outbound_htlcs( + &self, + ) -> HashMap)> { let mut res = new_hash_map(); // Just examine the available counterparty commitment transactions. See docs on // `fail_unbroadcast_htlcs`, below, for justification. let us = self.inner.lock().unwrap(); - macro_rules! walk_counterparty_commitment { - ($txid: expr) => { - if let Some(ref latest_outpoints) = us.counterparty_claimable_outpoints.get($txid) { - for &(ref htlc, ref source_option) in latest_outpoints.iter() { - if let &Some(ref source) = source_option { - res.insert((**source).clone(), (htlc.clone(), - us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned())); + let mut walk_counterparty_commitment = |txid| { + if let Some(latest_outpoints) = us.counterparty_claimable_outpoints.get(txid) { + for &(ref htlc, ref source_option) in latest_outpoints.iter() { + if let &Some(ref source) = source_option { + let htlc_id = SentHTLCId::from_source(source); + if !us.htlcs_resolved_to_user.contains(&htlc_id) { + let preimage_opt = + us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned(); + res.insert((**source).clone(), (htlc.clone(), preimage_opt)); } } } } - } + }; if let Some(ref txid) = us.current_counterparty_commitment_txid { - walk_counterparty_commitment!(txid); + walk_counterparty_commitment(txid); } if let Some(ref txid) = us.prev_counterparty_commitment_txid { - walk_counterparty_commitment!(txid); + walk_counterparty_commitment(txid); } res } - /// Gets the set of outbound HTLCs which are pending resolution in this channel or which were - /// resolved with a preimage from our counterparty. - /// - /// This is used to reconstruct pending outbound payments on restart in the ChannelManager. - /// - /// Currently, the preimage is unused, however if it is present in the relevant internal state - /// an HTLC is always included even if it has been resolved. - pub(crate) fn get_pending_or_resolved_outbound_htlcs(&self) -> HashMap)> { + /// Gets the set of outbound HTLCs which hit the chain and ultimately were claimed by us via + /// the timeout path and reached [`ANTI_REORG_DELAY`] confirmations. This is used to determine + /// if an HTLC has failed without the `ChannelManager` having seen it prior to being persisted. + pub(crate) fn get_onchain_failed_outbound_htlcs(&self) -> HashMap { + let mut res = new_hash_map(); let us = self.inner.lock().unwrap(); - // We're only concerned with the confirmation count of HTLC transactions, and don't - // actually care how many confirmations a commitment transaction may or may not have. Thus, - // we look for either a FundingSpendConfirmation event or a funding_spend_confirmed. + + // We only want HTLCs with ANTI_REORG_DELAY confirmations, which implies the commitment + // transaction has least ANTI_REORG_DELAY confirmations for any dependent HTLC transactions + // to have been confirmed. let confirmed_txid = us.funding_spend_confirmed.or_else(|| { us.onchain_events_awaiting_threshold_conf.iter().find_map(|event| { if let OnchainEvent::FundingSpendConfirmation { .. } = event.event { - Some(event.txid) - } else { None } + if event.height + ANTI_REORG_DELAY - 1 <= us.best_block.height { + Some(event.txid) + } else { + None + } + } else { + None + } }) }); - if confirmed_txid.is_none() { - // If we have not seen a commitment transaction on-chain (ie the channel is not yet - // closed), just get the full set. - mem::drop(us); - return self.get_all_current_outbound_htlcs(); - } + let confirmed_txid = if let Some(txid) = confirmed_txid { + txid + } else { + return res; + }; - let mut res = new_hash_map(); macro_rules! walk_htlcs { - ($holder_commitment: expr, $htlc_iter: expr) => { - for (htlc, source) in $htlc_iter { - if us.htlcs_resolved_on_chain.iter().any(|v| v.commitment_tx_output_idx == htlc.transaction_output_index) { - // We should assert that funding_spend_confirmed is_some() here, but we - // have some unit tests which violate HTLC transaction CSVs entirely and - // would fail. - // TODO: Once tests all connect transactions at consensus-valid times, we - // should assert here like we do in `get_claimable_balances`. - } else if htlc.offered == $holder_commitment { - // If the payment was outbound, check if there's an HTLCUpdate - // indicating we have spent this HTLC with a timeout, claiming it back - // and awaiting confirmations on it. - let htlc_update_confd = us.onchain_events_awaiting_threshold_conf.iter().any(|event| { - if let OnchainEvent::HTLCUpdate { commitment_tx_output_idx: Some(commitment_tx_output_idx), .. } = event.event { - // If the HTLC was timed out, we wait for ANTI_REORG_DELAY blocks - // before considering it "no longer pending" - this matches when we - // provide the ChannelManager an HTLC failure event. - Some(commitment_tx_output_idx) == htlc.transaction_output_index && - us.best_block.height >= event.height + ANTI_REORG_DELAY - 1 - } else if let OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, .. } = event.event { - // If the HTLC was fulfilled with a preimage, we consider the HTLC - // immediately non-pending, matching when we provide ChannelManager - // the preimage. - Some(commitment_tx_output_idx) == htlc.transaction_output_index - } else { false } - }); - let counterparty_resolved_preimage_opt = - us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned(); - if !htlc_update_confd || counterparty_resolved_preimage_opt.is_some() { - res.insert(source.clone(), (htlc.clone(), counterparty_resolved_preimage_opt)); + ($htlc_iter: expr) => { + let mut walk_candidate_htlcs = |htlcs| { + for &(ref candidate_htlc, ref candidate_source) in htlcs { + let candidate_htlc: &HTLCOutputInCommitment = &candidate_htlc; + let candidate_source: &Option> = &candidate_source; + + let source: &HTLCSource = if let Some(source) = candidate_source { + source + } else { + continue; + }; + let htlc_id = SentHTLCId::from_source(source); + if us.htlcs_resolved_to_user.contains(&htlc_id) { + continue; + } + + let confirmed = $htlc_iter.find(|(_, conf_src)| Some(source) == *conf_src); + if let Some((confirmed_htlc, _)) = confirmed { + let filter = |v: &&IrrevocablyResolvedHTLC| { + v.commitment_tx_output_idx + == confirmed_htlc.transaction_output_index + }; + + // The HTLC was included in the confirmed commitment transaction, so we + // need to see if it has been irrevocably failed yet. + if confirmed_htlc.transaction_output_index.is_none() { + // Dust HTLCs are always implicitly failed once the commitment + // transaction reaches ANTI_REORG_DELAY confirmations. + res.insert(source.clone(), confirmed_htlc.payment_hash); + } else if let Some(state) = + us.htlcs_resolved_on_chain.iter().filter(filter).next() + { + if state.payment_preimage.is_none() { + res.insert(source.clone(), confirmed_htlc.payment_hash); + } + } + } else { + // The HTLC was not included in the confirmed commitment transaction, + // which has now reached ANTI_REORG_DELAY confirmations and thus the + // HTLC has been failed. + res.insert(source.clone(), candidate_htlc.payment_hash); } } + }; + + // We walk the set of HTLCs in the unrevoked counterparty commitment transactions (see + // `fail_unbroadcast_htlcs` for a description of why). + if let Some(ref txid) = us.current_counterparty_commitment_txid { + let htlcs = us.counterparty_claimable_outpoints.get(txid); + walk_candidate_htlcs(htlcs.expect("Missing tx info for latest tx")); } - } + if let Some(ref txid) = us.prev_counterparty_commitment_txid { + let htlcs = us.counterparty_claimable_outpoints.get(txid); + walk_candidate_htlcs(htlcs.expect("Missing tx info for previous tx")); + } + }; } - let txid = confirmed_txid.unwrap(); - if Some(txid) == us.current_counterparty_commitment_txid || Some(txid) == us.prev_counterparty_commitment_txid { - walk_htlcs!(false, us.counterparty_claimable_outpoints.get(&txid).unwrap().iter().filter_map(|(a, b)| { + if Some(confirmed_txid) == us.current_counterparty_commitment_txid + || Some(confirmed_txid) == us.prev_counterparty_commitment_txid + { + let htlcs = us.counterparty_claimable_outpoints.get(&confirmed_txid).unwrap(); + walk_htlcs!(htlcs.iter().filter_map(|(a, b)| { if let &Some(ref source) = b { - Some((a, &**source)) - } else { None } - })); - } else if txid == us.current_holder_commitment_tx.txid { - walk_htlcs!(true, us.current_holder_commitment_tx.htlc_outputs.iter().filter_map(|(a, _, c)| { - if let Some(source) = c { Some((a, source)) } else { None } + Some((a, Some(&**source))) + } else { + None + } })); - } else if let Some(prev_commitment) = &us.prev_holder_signed_commitment_tx { - if txid == prev_commitment.txid { - walk_htlcs!(true, prev_commitment.htlc_outputs.iter().filter_map(|(a, _, c)| { - if let Some(source) = c { Some((a, source)) } else { None } - })); + } else if confirmed_txid == us.current_holder_commitment_tx.txid { + let mut htlcs = + us.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())); + walk_htlcs!(htlcs); + } else if let Some(prev_commitment_tx) = &us.prev_holder_signed_commitment_tx { + if confirmed_txid == prev_commitment_tx.txid { + let mut htlcs = + prev_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())); + walk_htlcs!(htlcs); + } else { + let htlcs_confirmed: &[(&HTLCOutputInCommitment, _)] = &[]; + walk_htlcs!(htlcs_confirmed.iter()); } + } else { + let htlcs_confirmed: &[(&HTLCOutputInCommitment, _)] = &[]; + walk_htlcs!(htlcs_confirmed.iter()); } res @@ -3027,7 +3118,7 @@ impl ChannelMonitorImpl { // First check if a counterparty commitment transaction has been broadcasted: macro_rules! claim_htlcs { ($commitment_number: expr, $txid: expr, $htlcs: expr) => { - let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs, confirmed_spend_height); + let htlc_claim_reqs = self.get_counterparty_output_claims_for_preimage(*payment_preimage, $commitment_number, $txid, $htlcs, confirmed_spend_height); let conf_target = self.closure_conf_target(); self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger); } @@ -3182,6 +3273,7 @@ impl ChannelMonitorImpl { if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID || self.lockdown_from_offchain { assert_eq!(updates.updates.len(), 1); match updates.updates[0] { + ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {}, ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {}, // We should have already seen a `ChannelForceClosed` update if we're trying to // provide a preimage at this point. @@ -3256,6 +3348,10 @@ impl ChannelMonitorImpl { panic!("Attempted to replace shutdown script {} with {}", shutdown_script, scriptpubkey); } }, + ChannelMonitorUpdateStep::ReleasePaymentComplete { htlc } => { + log_trace!(logger, "HTLC {htlc:?} permanently and fully resolved"); + self.htlcs_resolved_to_user.insert(*htlc); + }, } } @@ -3277,11 +3373,13 @@ impl ChannelMonitorImpl { |ChannelMonitorUpdateStep::CommitmentSecret { .. } => is_pre_close_update = true, // After a channel is closed, we don't communicate with our peer about it, so the - // only things we will update is getting a new preimage (from a different channel) - // or being told that the channel is closed. All other updates are generated while - // talking to our peer. + // only things we will update is getting a new preimage (from a different channel), + // being told that the channel is closed, or being told a payment which was + // resolved on-chain has had its resolution communicated to the user. All other + // updates are generated while talking to our peer. ChannelMonitorUpdateStep::PaymentPreimage { .. } => {}, ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {}, + ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {}, } } @@ -3291,6 +3389,16 @@ impl ChannelMonitorImpl { } else { ret } } + /// Returns true if the channel has been closed (i.e. no further updates are allowed) and no + /// commitment state updates ever happened. + fn is_closed_without_updates(&self) -> bool { + let mut commitment_not_advanced = + self.current_counterparty_commitment_number == INITIAL_COMMITMENT_NUMBER; + commitment_not_advanced &= + self.current_holder_commitment_number == INITIAL_COMMITMENT_NUMBER; + (self.holder_tx_signed || self.lockdown_from_offchain) && commitment_not_advanced + } + fn no_further_updates_allowed(&self) -> bool { self.funding_spend_seen || self.lockdown_from_offchain || self.holder_tx_signed } @@ -3620,7 +3728,7 @@ impl ChannelMonitorImpl { (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) ), logger); let (htlc_claim_reqs, counterparty_output_info) = - self.get_counterparty_output_claim_info(commitment_number, commitment_txid, Some(tx), per_commitment_option, Some(height)); + self.get_counterparty_output_claim_info(commitment_number, commitment_txid, tx, per_commitment_claimable_data, Some(height)); to_counterparty_output_info = counterparty_output_info; for req in htlc_claim_reqs { claimable_outpoints.push(req); @@ -3630,78 +3738,162 @@ impl ChannelMonitorImpl { (claimable_outpoints, to_counterparty_output_info) } - /// Returns the HTLC claim package templates and the counterparty output info - fn get_counterparty_output_claim_info(&self, commitment_number: u64, commitment_txid: Txid, tx: Option<&Transaction>, per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, confirmation_height: Option) - -> (Vec, CommitmentTxCounterpartyOutputInfo) { - let mut claimable_outpoints = Vec::new(); - let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None; + fn get_point_for_commitment_number(&self, commitment_number: u64) -> Option { + let per_commitment_points = &self.their_cur_per_commitment_points?; + + // If the counterparty commitment tx is the latest valid state, use their latest + // per-commitment point + if per_commitment_points.0 == commitment_number { + Some(per_commitment_points.1) + } else if let Some(point) = per_commitment_points.2.as_ref() { + // If counterparty commitment tx is the state previous to the latest valid state, use + // their previous per-commitment point (non-atomicity of revocation means it's valid for + // them to temporarily have two valid commitment txns from our viewpoint) + if per_commitment_points.0 == commitment_number + 1 { + Some(*point) + } else { + None + } + } else { + None + } + } + fn get_counterparty_output_claims_for_preimage( + &self, preimage: PaymentPreimage, commitment_number: u64, + commitment_txid: Txid, + per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, + confirmation_height: Option, + ) -> Vec { let per_commitment_claimable_data = match per_commitment_option { Some(outputs) => outputs, - None => return (claimable_outpoints, to_counterparty_output_info), + None => return Vec::new(), }; - let per_commitment_points = match self.their_cur_per_commitment_points { - Some(points) => points, - None => return (claimable_outpoints, to_counterparty_output_info), + let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) { + Some(point) => point, + None => return Vec::new(), }; - let per_commitment_point = - // If the counterparty commitment tx is the latest valid state, use their latest - // per-commitment point - if per_commitment_points.0 == commitment_number { &per_commitment_points.1 } - else if let Some(point) = per_commitment_points.2.as_ref() { - // If counterparty commitment tx is the state previous to the latest valid state, use - // their previous per-commitment point (non-atomicity of revocation means it's valid for - // them to temporarily have two valid commitment txns from our viewpoint) - if per_commitment_points.0 == commitment_number + 1 { - point - } else { return (claimable_outpoints, to_counterparty_output_info); } - } else { return (claimable_outpoints, to_counterparty_output_info); }; - - if let Some(transaction) = tx { - let revocation_pubkey = RevocationKey::from_basepoint( - &self.onchain_tx_handler.secp_ctx, &self.holder_revocation_basepoint, &per_commitment_point); - - let delayed_key = DelayedPaymentKey::from_basepoint(&self.onchain_tx_handler.secp_ctx, &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, &per_commitment_point); - - let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, - self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key).to_p2wsh(); - for (idx, outp) in transaction.output.iter().enumerate() { - if outp.script_pubkey == revokeable_p2wsh { - to_counterparty_output_info = - Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); + let matching_payment_hash = PaymentHash::from(preimage); + per_commitment_claimable_data + .iter() + .filter_map(|(htlc, _)| { + if let Some(transaction_output_index) = htlc.transaction_output_index { + if htlc.offered && htlc.payment_hash == matching_payment_hash { + let htlc_data = PackageSolvingData::CounterpartyOfferedHTLCOutput( + CounterpartyOfferedHTLCOutput::build( + per_commitment_point, + self.counterparty_commitment_params.counterparty_delayed_payment_base_key, + self.counterparty_commitment_params.counterparty_htlc_base_key, + preimage, + htlc.clone(), + self.onchain_tx_handler.channel_type_features().clone(), + confirmation_height, + ), + ); + Some(PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + htlc_data, + htlc.cltv_expiry, + )) + } else { + None + } + } else { + None } + }) + .collect() + } + + /// Returns the HTLC claim package templates and the counterparty output info + fn get_counterparty_output_claim_info( + &self, commitment_number: u64, commitment_txid: Txid, + tx: &Transaction, + per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option>)], + confirmation_height: Option, + ) -> (Vec, CommitmentTxCounterpartyOutputInfo) { + let mut claimable_outpoints = Vec::new(); + let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None; + + let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) { + Some(point) => point, + None => return (claimable_outpoints, to_counterparty_output_info), + }; + + let revocation_pubkey = RevocationKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.holder_revocation_basepoint, + &per_commitment_point, + ); + let delayed_key = DelayedPaymentKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, + &per_commitment_point, + ); + let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript( + &revocation_pubkey, + self.counterparty_commitment_params.on_counterparty_tx_csv, + &delayed_key, + ) + .to_p2wsh(); + for (idx, outp) in tx.output.iter().enumerate() { + if outp.script_pubkey == revokeable_p2wsh { + to_counterparty_output_info = + Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); } } - for &(ref htlc, _) in per_commitment_claimable_data.iter() { + for &(ref htlc, _) in per_commitment_claimable_data.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { - if let Some(transaction) = tx { - if transaction_output_index as usize >= transaction.output.len() || - transaction.output[transaction_output_index as usize].value != htlc.to_bitcoin_amount() { - // per_commitment_data is corrupt or our commitment signing key leaked! - return (claimable_outpoints, to_counterparty_output_info); - } + if transaction_output_index as usize >= tx.output.len() + || tx.output[transaction_output_index as usize].value + != htlc.to_bitcoin_amount() + { + // per_commitment_data is corrupt or our commitment signing key leaked! + return (claimable_outpoints, to_counterparty_output_info); } - let preimage = if htlc.offered { if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None }; + let preimage = if htlc.offered { + if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { + Some(*p) + } else { + None + } + } else { + None + }; if preimage.is_some() || !htlc.offered { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( - CounterpartyOfferedHTLCOutput::build(*per_commitment_point, + CounterpartyOfferedHTLCOutput::build( + per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, - preimage.unwrap(), htlc.clone(), self.onchain_tx_handler.channel_type_features().clone(), - confirmation_height)) + preimage.unwrap(), + htlc.clone(), + self.onchain_tx_handler.channel_type_features().clone(), + confirmation_height, + ), + ) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( - CounterpartyReceivedHTLCOutput::build(*per_commitment_point, + CounterpartyReceivedHTLCOutput::build( + per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, - htlc.clone(), self.onchain_tx_handler.channel_type_features().clone(), - confirmation_height)) + htlc.clone(), + self.onchain_tx_handler.channel_type_features().clone(), + confirmation_height, + ), + ) }; - let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry); + let counterparty_package = PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + counterparty_htlc_outp, + htlc.cltv_expiry, + ); claimable_outpoints.push(counterparty_package); } } @@ -4656,6 +4848,7 @@ impl ChannelMonitorImpl { on_to_local_output_csv: None, }, }); + self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { source, payment_preimage: Some(payment_preimage), @@ -4679,6 +4872,7 @@ impl ChannelMonitorImpl { on_to_local_output_csv: None, }, }); + self.counterparty_fulfilled_htlcs.insert(SentHTLCId::from_source(&source), payment_preimage); self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate { source, payment_preimage: Some(payment_preimage), @@ -5030,6 +5224,7 @@ impl<'a, 'b, ES: EntropySource, Signer: EcdsaChannelSigner, SP: SignerProvider, @@ -849,6 +849,50 @@ pub struct PackageTemplate { height_timer: u32, } +impl PartialEq for PackageTemplate { + fn eq(&self, o: &Self) -> bool { + if self.inputs != o.inputs + || self.malleability != o.malleability + || self.feerate_previous != o.feerate_previous + || self.height_timer != o.height_timer + { + return false; + } + #[cfg(test)] + { + // In some cases we may reset `counterparty_spendable_height` to zero on reload, which + // can cause our test assertions that ChannelMonitors round-trip exactly to trip. Here + // we allow exactly the same case as we tweak in the `PackageTemplate` `Readable` + // implementation. + if self.counterparty_spendable_height == 0 { + for (_, input) in self.inputs.iter() { + if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { + htlc, .. + }) = input + { + if !htlc.offered && htlc.cltv_expiry != 0 { + return true; + } + } + } + } + if o.counterparty_spendable_height == 0 { + for (_, input) in o.inputs.iter() { + if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { + htlc, .. + }) = input + { + if !htlc.offered && htlc.cltv_expiry != 0 { + return true; + } + } + } + } + } + self.counterparty_spendable_height == o.counterparty_spendable_height + } +} + impl PackageTemplate { pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32) -> bool { match (self.malleability, other.malleability) { diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 5bc446f9724..4b492b0607a 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -1364,7 +1364,7 @@ pub enum Event { /// To accept the request (and in the case of a dual-funded channel, not contribute funds), /// call [`ChannelManager::accept_inbound_channel`]. /// To reject the request, call [`ChannelManager::force_close_without_broadcasting_txn`]. - /// Note that a ['ChannelClosed`] event will _not_ be triggered if the channel is rejected. + /// Note that a [`ChannelClosed`] event will _not_ be triggered if the channel is rejected. /// /// The event is only triggered when a new open channel request is received and the /// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. @@ -1374,6 +1374,7 @@ pub enum Event { /// returning `Err(ReplayEvent ())`) and won't be persisted across restarts. /// /// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel + /// [`ChannelClosed`]: Event::ChannelClosed /// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn /// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels OpenChannelRequest { diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index 4fa2871ddcb..c35908ecf5e 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -41,7 +41,7 @@ #![allow(bare_trait_objects)] #![allow(ellipsis_inclusive_range_patterns)] -#![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![cfg_attr(docsrs, feature(doc_cfg))] #![cfg_attr(all(not(feature = "std"), not(test)), no_std)] diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 657e089d293..ef645e71d63 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -19,11 +19,12 @@ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor}; use crate::chain::transaction::OutPoint; use crate::chain::{ChannelMonitorUpdateStatus, Listen, Watch}; use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination}; -use crate::ln::channelmanager::{PaymentId, RAACommitmentOrder, RecipientOnionFields}; +use crate::ln::channelmanager::{PaymentId, RAACommitmentOrder, RecipientOnionFields, Retry}; use crate::ln::channel::{AnnouncementSigsState, ChannelPhase}; use crate::ln::msgs; use crate::ln::types::ChannelId; use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler}; +use crate::routing::router::{PaymentParameters, RouteParameters}; use crate::util::test_channel_signer::TestChannelSigner; use crate::util::ser::{ReadableArgs, Writeable}; use crate::util::test_utils::TestBroadcaster; @@ -3030,7 +3031,7 @@ fn do_test_blocked_chan_preimage_release(completion_mode: BlockedUpdateComplMode assert!(a.is_none()); nodes[1].node.handle_revoke_and_ack(node_a_id, &raa); - check_added_monitors(&nodes[1], 0); + check_added_monitors(&nodes[1], 1); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); } @@ -3040,8 +3041,8 @@ fn do_test_blocked_chan_preimage_release(completion_mode: BlockedUpdateComplMode if let Event::PaymentPathSuccessful { .. } = events[2] {} else { panic!(); } if let Event::PaymentForwarded { .. } = events[1] {} else { panic!(); } - // The event processing should release the last RAA updates on both channels. - check_added_monitors(&nodes[1], 2); + // The event processing should release the last RAA update. + check_added_monitors(&nodes[1], 1); // When we fetch the next update the message getter will generate the next update for nodes[2], // generating a further monitor update. @@ -3314,6 +3315,7 @@ fn do_test_durable_preimages_on_closed_channel(close_chans_before_reload: bool, nodes[0].node.force_close_broadcasting_latest_txn(&chan_id_ab, &nodes[1].node.get_our_node_id(), error_message.to_string()).unwrap(); check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, false, &[nodes[1].node.get_our_node_id()], 100000); + check_added_monitors(&nodes[0], 1); let as_closing_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); assert_eq!(as_closing_tx.len(), 1); @@ -3361,22 +3363,25 @@ fn do_test_durable_preimages_on_closed_channel(close_chans_before_reload: bool, reconnect_nodes(reconnect_args); - // Once the blocked `ChannelMonitorUpdate` *finally* completes, the pending - // `PaymentForwarded` event will finally be released. - let (outpoint, ab_update_id, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone(); - nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, ab_update_id); - - // If the A<->B channel was closed before we reload, we'll replay the claim against it on - // reload, causing the `PaymentForwarded` event to get replayed. - let evs = nodes[1].node.get_and_clear_pending_events(); - assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 }); - for ev in evs { - if let Event::PaymentForwarded { .. } = ev { } - else { - panic!(); - } + } + + // Once the blocked `ChannelMonitorUpdate` *finally* completes, the pending + // `PaymentForwarded` event will finally be released. + let (outpoint, ab_update_id, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone(); + nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, ab_update_id); + + // If the A<->B channel was closed before we reload, we'll replay the claim against it on + // reload, causing the `PaymentForwarded` event to get replayed. + let evs = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 }); + for ev in evs { + if let Event::PaymentForwarded { .. } = ev { } + else { + panic!(); } + } + if !close_chans_before_reload || close_only_a { // Once we call `process_pending_events` the final `ChannelMonitor` for the B<->C channel // will fly, removing the payment preimage from it. check_added_monitors(&nodes[1], 1); @@ -3597,8 +3602,11 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) { let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); - create_announced_chan_between_nodes(&nodes, 0, 1); - create_announced_chan_between_nodes(&nodes, 1, 2); + let node_a_id = nodes[0].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + let chan_id_ab = create_announced_chan_between_nodes(&nodes, 0, 1).2; + let _chan_id_bc = create_announced_chan_between_nodes(&nodes, 1, 2).2; // Route a payment from A, through B, to C, then claim it on C. Replay the // `update_fulfill_htlc` twice on B to check that B doesn't hang. @@ -3610,7 +3618,7 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) { let cs_updates = get_htlc_update_msgs(&nodes[2], &nodes[1].node.get_our_node_id()); if hold_chan_a { - // The first update will be on the A <-> B channel, which we allow to complete. + // The first update will be on the A <-> B channel, which we optionally allow to complete. chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); } nodes[1].node.handle_update_fulfill_htlc(nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]); @@ -3637,14 +3645,51 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); - let (route, payment_hash_2, _, payment_secret_2) = get_route_and_payment_hash!(&nodes[1], nodes[2], 1_000_000); + let (route, payment_hash_2, payment_preimage_2, payment_secret_2) = + get_route_and_payment_hash!(&nodes[1], nodes[2], 1_000_000); + // With the A<->B preimage persistence not yet complete, the B<->C channel is stuck + // waiting. nodes[1].node.send_payment_with_route(route, payment_hash_2, RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)).unwrap(); check_added_monitors(&nodes[1], 0); assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // ...but once we complete the A<->B channel preimage persistence, the B<->C channel + // unlocks and we send both peers commitment updates. + let (outpoint, ab_update_id, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone(); + assert!(nodes[1].chain_monitor.chain_monitor.channel_monitor_updated(outpoint, ab_update_id).is_ok()); + + let mut msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 2); + check_added_monitors(&nodes[1], 2); + + let mut c_update = msg_events.iter() + .filter(|ev| matches!(ev, MessageSendEvent::UpdateHTLCs { node_id, .. } if *node_id == node_c_id)) + .cloned().collect::>(); + let a_filtermap = |ev| if let MessageSendEvent::UpdateHTLCs { node_id, updates } = ev { + if node_id == node_a_id { + Some(updates) + } else { + None + } + } else { + None + }; + let a_update = msg_events.drain(..).filter_map(|ev| a_filtermap(ev)).collect::>(); + + assert_eq!(a_update.len(), 1); + assert_eq!(c_update.len(), 1); + + nodes[0].node.handle_update_fulfill_htlc(nodes[1].node.get_our_node_id(), &a_update[0].update_fulfill_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], a_update[0].commitment_signed, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false); + + pass_along_path(&nodes[1], &[&nodes[2]], 1_000_000, payment_hash_2, Some(payment_secret_2), c_update.pop().unwrap(), true, None); + claim_payment(&nodes[1], &[&nodes[2]], payment_preimage_2); } } @@ -3654,13 +3699,17 @@ fn test_glacial_peer_cant_hang() { do_test_glacial_peer_cant_hang(true); } -#[test] -fn test_partial_claim_mon_update_compl_actions() { +fn do_test_partial_claim_mon_update_compl_actions(reload_a: bool, reload_b: bool) { // Test that if we have an MPP claim that we ensure the preimage for the claim is retained in // all the `ChannelMonitor`s until the preimage reaches every `ChannelMonitor` for a channel // which was a part of the MPP. let chanmon_cfgs = create_chanmon_cfgs(4); let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + + let (persister, persister_2, persister_3); + let (new_chain_mon, new_chain_mon_2, new_chain_mon_3); + let (nodes_3_reload, nodes_3_reload_2, nodes_3_reload_3); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs); @@ -3682,6 +3731,8 @@ fn test_partial_claim_mon_update_compl_actions() { route.paths[1].hops[1].short_channel_id = chan_4_scid; send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], 200_000, payment_hash, payment_secret); + // Store the monitor for channel 4 without the preimage to use on reload + let chan_4_monitor_serialized = get_monitor!(nodes[3], chan_4_id).encode(); // Claim along both paths, but only complete one of the two monitor updates. chanmon_cfgs[3].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); chanmon_cfgs[3].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); @@ -3693,7 +3744,13 @@ fn test_partial_claim_mon_update_compl_actions() { // Complete the 1<->3 monitor update and play the commitment_signed dance forward until it // blocks. nodes[3].chain_monitor.complete_sole_pending_chan_update(&chan_3_id); - expect_payment_claimed!(&nodes[3], payment_hash, 200_000); + let payment_claimed = nodes[3].node.get_and_clear_pending_events(); + assert_eq!(payment_claimed.len(), 1, "{payment_claimed:?}"); + if let Event::PaymentClaimed { payment_hash: ev_hash, .. } = &payment_claimed[0] { + assert_eq!(*ev_hash, payment_hash); + } else { + panic!("{payment_claimed:?}"); + } let updates = get_htlc_update_msgs(&nodes[3], &nodes[1].node.get_our_node_id()); nodes[1].node.handle_update_fulfill_htlc(nodes[3].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]); @@ -3712,15 +3769,41 @@ fn test_partial_claim_mon_update_compl_actions() { check_added_monitors(&nodes[3], 0); assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + if reload_a { + // After a reload (with the monitor not yet fully updated), the RAA should still be blocked + // waiting until the monitor update completes. + let node_ser = nodes[3].node.encode(); + let chan_3_monitor_serialized = get_monitor!(nodes[3], chan_3_id).encode(); + let mons = &[&chan_3_monitor_serialized[..], &chan_4_monitor_serialized[..]]; + reload_node!(nodes[3], &node_ser, mons, persister, new_chain_mon, nodes_3_reload); + // The final update to channel 4 should be replayed. + persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + check_added_monitors(&nodes[3], 1); + + // Because the HTLCs aren't yet cleared, the PaymentClaimed event will be replayed on + // restart. + let second_payment_claimed = nodes[3].node.get_and_clear_pending_events(); + assert_eq!(payment_claimed, second_payment_claimed); + + nodes[1].node.peer_disconnected(nodes[3].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[3].node.get_our_node_id()); + reconnect_nodes(ReconnectArgs::new(&nodes[1], &nodes[3])); + reconnect_nodes(ReconnectArgs::new(&nodes[2], &nodes[3])); + + assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + } + // Now double-check that the preimage is still in the 1<->3 channel and complete the pending // monitor update, allowing node 3 to claim the payment on the 2<->3 channel. This also // unblocks the 1<->3 channel, allowing node 3 to release the two blocked monitor updates and // respond to the final commitment_signed. assert!(get_monitor!(nodes[3], chan_3_id).get_stored_preimages().contains_key(&payment_hash)); + assert!(nodes[3].node.get_and_clear_pending_events().is_empty()); nodes[3].chain_monitor.complete_sole_pending_chan_update(&chan_4_id); let mut ds_msgs = nodes[3].node.get_and_clear_pending_msg_events(); - assert_eq!(ds_msgs.len(), 2); + assert_eq!(ds_msgs.len(), 2, "{ds_msgs:?}"); check_added_monitors(&nodes[3], 2); match remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut ds_msgs) { @@ -3761,14 +3844,87 @@ fn test_partial_claim_mon_update_compl_actions() { assert!(get_monitor!(nodes[3], chan_3_id).get_stored_preimages().contains_key(&payment_hash)); assert!(get_monitor!(nodes[3], chan_4_id).get_stored_preimages().contains_key(&payment_hash)); + if reload_b { + // Ensure that the channel pause logic doesn't accidentally get restarted after a second + // reload once the HTLCs for the first payment have been removed and the monitors + // completed. + let node_ser = nodes[3].node.encode(); + let chan_3_monitor_serialized = get_monitor!(nodes[3], chan_3_id).encode(); + let chan_4_monitor_serialized = get_monitor!(nodes[3], chan_4_id).encode(); + let mons = &[&chan_3_monitor_serialized[..], &chan_4_monitor_serialized[..]]; + reload_node!(nodes[3], &node_ser, mons, persister_2, new_chain_mon_2, nodes_3_reload_2); + check_added_monitors(&nodes[3], 0); + + nodes[1].node.peer_disconnected(nodes[3].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[3].node.get_our_node_id()); + reconnect_nodes(ReconnectArgs::new(&nodes[1], &nodes[3])); + reconnect_nodes(ReconnectArgs::new(&nodes[2], &nodes[3])); + + assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + + // Because the HTLCs aren't yet cleared, the PaymentClaimed event will be replayed on + // restart. + let third_payment_claimed = nodes[3].node.get_and_clear_pending_events(); + assert_eq!(payment_claimed, third_payment_claimed); + } + send_payment(&nodes[1], &[&nodes[3]], 100_000); assert!(!get_monitor!(nodes[3], chan_3_id).get_stored_preimages().contains_key(&payment_hash)); + if reload_b { + // Ensure that the channel pause logic doesn't accidentally get restarted after a second + // reload once the HTLCs for the first payment have been removed and the monitors + // completed, even if only one of the two monitors still knows about the first payment. + let node_ser = nodes[3].node.encode(); + let chan_3_monitor_serialized = get_monitor!(nodes[3], chan_3_id).encode(); + let chan_4_monitor_serialized = get_monitor!(nodes[3], chan_4_id).encode(); + let mons = &[&chan_3_monitor_serialized[..], &chan_4_monitor_serialized[..]]; + reload_node!(nodes[3], &node_ser, mons, persister_3, new_chain_mon_3, nodes_3_reload_3); + check_added_monitors(&nodes[3], 0); + + nodes[1].node.peer_disconnected(nodes[3].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[3].node.get_our_node_id()); + reconnect_nodes(ReconnectArgs::new(&nodes[1], &nodes[3])); + reconnect_nodes(ReconnectArgs::new(&nodes[2], &nodes[3])); + + assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + + // Because the HTLCs aren't yet cleared, the PaymentClaimed events for both payments will + // be replayed on restart. + // Use this as an opportunity to check the payment_ids are unique. + let mut events = nodes[3].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + events.retain(|ev| *ev != payment_claimed[0]); + assert_eq!(events.len(), 1); + if let Event::PaymentClaimed { payment_id: original_payment_id, .. } = &payment_claimed[0] { + assert!(original_payment_id.is_some()); + if let Event::PaymentClaimed { amount_msat, payment_id, .. } = &events[0] { + assert!(payment_id.is_some()); + assert_ne!(original_payment_id, payment_id); + assert_eq!(*amount_msat, 100_000); + } else { + panic!("{events:?}"); + } + } else { + panic!("{events:?}"); + } + + send_payment(&nodes[1], &[&nodes[3]], 100_000); + } + send_payment(&nodes[2], &[&nodes[3]], 100_000); assert!(!get_monitor!(nodes[3], chan_4_id).get_stored_preimages().contains_key(&payment_hash)); } +#[test] +fn test_partial_claim_mon_update_compl_actions() { + do_test_partial_claim_mon_update_compl_actions(true, true); + do_test_partial_claim_mon_update_compl_actions(true, false); + do_test_partial_claim_mon_update_compl_actions(false, true); + do_test_partial_claim_mon_update_compl_actions(false, false); +} + #[test] fn test_claim_to_closed_channel_blocks_forwarded_preimage_removal() { // One of the last features for async persistence we implemented was the correct blocking of @@ -3964,6 +4120,23 @@ fn test_single_channel_multiple_mpp() { // `update_fulfill_htlc`/`commitment_signed` pair to pass to our counterparty. do_a_write.send(()).unwrap(); + let event_node: &'static TestChannelManager<'static, 'static> = + unsafe { std::mem::transmute(nodes[8].node as &TestChannelManager) }; + let thrd_event = std::thread::spawn(move || { + let mut have_event = false; + while !have_event { + let mut events = event_node.get_and_clear_pending_events(); + assert!(events.len() == 1 || events.len() == 0); + if events.len() == 1 { + if let Event::PaymentClaimed { .. } = events[0] { + } else { + panic!("Unexpected event {events:?}"); + } + have_event = true; + } + } + }); + // Then fetch the `update_fulfill_htlc`/`commitment_signed`. Note that the // `get_and_clear_pending_msg_events` will immediately hang trying to take a peer lock which // `claim_funds` is holding. Thus, we release a second write after a small sleep in the @@ -3983,7 +4156,11 @@ fn test_single_channel_multiple_mpp() { }); block_thrd2.store(false, Ordering::Release); let first_updates = get_htlc_update_msgs(&nodes[8], &nodes[7].node.get_our_node_id()); + + // Thread 2 could unblock first, or it could get blocked waiting on us to process a + // `PaymentClaimed` event. Either way, wait until both have finished. thrd2.join().unwrap(); + thrd_event.join().unwrap(); // Disconnect node 6 from all its peers so it doesn't bother to fail the HTLCs back nodes[7].node.peer_disconnected(nodes[1].node.get_our_node_id()); @@ -4029,8 +4206,6 @@ fn test_single_channel_multiple_mpp() { thrd4.join().unwrap(); thrd.join().unwrap(); - expect_payment_claimed!(nodes[8], payment_hash, 50_000_000); - // At the end, we should have 7 ChannelMonitorUpdates - 6 for HTLC claims, and one for the // above `revoke_and_ack`. check_added_monitors(&nodes[8], 7); @@ -4097,3 +4272,121 @@ fn test_single_channel_multiple_mpp() { nodes[7].node.handle_revoke_and_ack(node_8_id, &raa); check_added_monitors(&nodes[7], 1); } + +#[test] +fn test_mpp_claim_to_holding_cell() { + // Previously, if an MPP payment was claimed while one channel was AwaitingRAA (causing the + // HTLC claim to go into the holding cell), and the RAA came in before the async monitor + // update with the preimage completed, the channel could hang waiting on itself. + // This tests that behavior. + let chanmon_cfgs = create_chanmon_cfgs(4); + let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); + let nodes = create_network(4, &node_cfgs, &node_chanmgrs); + + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + let node_d_id = nodes[3].node.get_our_node_id(); + + // First open channels in a diamond and deliver the MPP payment. + let chan_1_scid = create_announced_chan_between_nodes(&nodes, 0, 1).0.contents.short_channel_id; + let chan_2_scid = create_announced_chan_between_nodes(&nodes, 0, 2).0.contents.short_channel_id; + let (chan_3_update, _, chan_3_id, ..) = create_announced_chan_between_nodes(&nodes, 1, 3); + let chan_3_scid = chan_3_update.contents.short_channel_id; + let (chan_4_update, _, chan_4_id, ..) = create_announced_chan_between_nodes(&nodes, 2, 3); + let chan_4_scid = chan_4_update.contents.short_channel_id; + + let (mut route, payment_hash, preimage_1, payment_secret) = + get_route_and_payment_hash!(&nodes[0], nodes[3], 500_000); + let path = route.paths[0].clone(); + route.paths.push(path); + route.paths[0].hops[0].pubkey = node_b_id; + route.paths[0].hops[0].short_channel_id = chan_1_scid; + route.paths[0].hops[1].short_channel_id = chan_3_scid; + route.paths[0].hops[1].fee_msat = 250_000; + route.paths[1].hops[0].pubkey = node_c_id; + route.paths[1].hops[0].short_channel_id = chan_2_scid; + route.paths[1].hops[1].short_channel_id = chan_4_scid; + route.paths[1].hops[1].fee_msat = 250_000; + let paths = &[&[&nodes[1], &nodes[3]][..], &[&nodes[2], &nodes[3]][..]]; + send_along_route_with_secret(&nodes[0], route, paths, 500_000, payment_hash, payment_secret); + + // Put the C <-> D channel into AwaitingRaa + let (preimage_2, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[3]); + let onion = RecipientOnionFields::secret_only(payment_secret_2); + let id = PaymentId([42; 32]); + let pay_params = PaymentParameters::from_node_id(node_d_id, TEST_FINAL_CLTV); + let route_params = RouteParameters::from_payment_params_and_value(pay_params, 400_000); + nodes[2].node.send_payment(payment_hash_2, onion, id, route_params, Retry::Attempts(0)).unwrap(); + check_added_monitors(&nodes[2], 1); + + let mut payment_event = SendEvent::from_node(&nodes[2]); + nodes[3].node.handle_update_add_htlc(node_c_id, &payment_event.msgs[0]); + nodes[3].node.handle_commitment_signed(node_c_id, &payment_event.commitment_msg); + check_added_monitors(&nodes[3], 1); + + let (raa, cs) = get_revoke_commit_msgs(&nodes[3], &node_c_id); + nodes[2].node.handle_revoke_and_ack(node_d_id, &raa); + check_added_monitors(&nodes[2], 1); + + nodes[2].node.handle_commitment_signed(node_d_id, &cs); + check_added_monitors(&nodes[2], 1); + + let cs_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, node_d_id); + + // Now claim the payment, completing both channel monitor updates async + // In the current code, the C <-> D channel happens to be the `durable_preimage_channel`, + // improving coverage somewhat but it isn't strictly critical to the test. + chanmon_cfgs[3].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + chanmon_cfgs[3].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + nodes[3].node.claim_funds(preimage_1); + check_added_monitors(&nodes[3], 2); + + // Complete the B <-> D monitor update, freeing the first fulfill. + let (outpoint, latest_id, _) = nodes[3].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_3_id).unwrap().clone(); + nodes[3].chain_monitor.chain_monitor.channel_monitor_updated(outpoint, latest_id).unwrap(); + + let mut b_claim = get_htlc_update_msgs(&nodes[3], &node_b_id); + + // When we deliver the pre-claim RAA, node D will shove the monitor update into the blocked + // state since we have a pending MPP payment which is blocking RAA monitor updates. + nodes[3].node.handle_revoke_and_ack(node_c_id, &cs_raa); + check_added_monitors(&nodes[3], 0); + + // Finally, complete the C <-> D monitor update. Previously, this unlock failed to be processed + // due to the existence of the blocked RAA update above. + let (outpoint, latest_id, _) = nodes[3].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_4_id).unwrap().clone(); + nodes[3].chain_monitor.chain_monitor.channel_monitor_updated(outpoint, latest_id).unwrap(); + // Once we process monitor events (in this case by checking for the `PaymentClaimed` event, the + // RAA monitor update blocked above will be released. + let events = nodes[3].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + if let Event::PaymentClaimed { .. } = &events[0] { + } else { + panic!("Unexpected event: {events:?}"); + } + if let Event::PendingHTLCsForwardable { .. } = &events[1] { + } else { + panic!("Unexpected event: {events:?}"); + } + check_added_monitors(&nodes[3], 1); + + // After the RAA monitor update completes, the C <-> D channel will be able to generate its + // fulfill updates as well. + let mut c_claim = get_htlc_update_msgs(&nodes[3], &node_c_id); + check_added_monitors(&nodes[3], 1); + + // Finally, clear all the pending payments. + let path = [&[&nodes[1], &nodes[3]][..], &[&nodes[2], &nodes[3]][..]]; + let mut args = ClaimAlongRouteArgs::new(&nodes[0], &path[..], preimage_1); + let b_claim_msgs = (b_claim.update_fulfill_htlcs.pop().unwrap(), b_claim.commitment_signed); + let c_claim_msgs = (c_claim.update_fulfill_htlcs.pop().unwrap(), c_claim.commitment_signed); + let claims = vec![(b_claim_msgs, node_b_id), (c_claim_msgs, node_c_id)]; + pass_claimed_payment_along_route_from_ev(250_000, claims, args); + + expect_payment_sent(&nodes[0], preimage_1, None, true, true); + + nodes[3].node.process_pending_htlc_forwards(); + expect_payment_claimable!(nodes[3], payment_hash_2, payment_secret_2, 400_000); + claim_payment(&nodes[2], &[&nodes[3]], preimage_2); +} diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 350d3125118..6c2e8af31b8 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4392,7 +4392,7 @@ impl Channel where Ok((closing_transaction, total_fee_satoshis)) } - fn funding_outpoint(&self) -> OutPoint { + pub fn funding_outpoint(&self) -> OutPoint { self.context.channel_transaction_parameters.funding_outpoint.unwrap() } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f8783b9b88f..f5b567c458c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -346,7 +346,7 @@ pub(super) struct PendingAddHTLCInfo { // Note that this may be an outbound SCID alias for the associated channel. prev_short_channel_id: u64, prev_htlc_id: u64, - prev_counterparty_node_id: Option, + prev_counterparty_node_id: PublicKey, prev_channel_id: ChannelId, prev_funding_outpoint: OutPoint, prev_user_channel_id: u128, @@ -864,9 +864,19 @@ struct ClaimingPayment { sender_intended_value: Option, onion_fields: Option, payment_id: Option, + /// When we claim and generate a [`Event::PaymentClaimed`], we want to block any + /// payment-preimage-removing RAA [`ChannelMonitorUpdate`]s until the [`Event::PaymentClaimed`] + /// is handled, ensuring we can regenerate the event on restart. We pick a random channel to + /// block and store it here. + /// + /// Note that once we disallow downgrades to 0.1 we should be able to simply use + /// [`Self::htlcs`] to generate this rather than storing it here (as we won't need the funding + /// outpoint), allowing us to remove this field. + durable_preimage_channel: Option<(OutPoint, PublicKey, ChannelId)>, } impl_writeable_tlv_based!(ClaimingPayment, { (0, amount_msat, required), + (1, durable_preimage_channel, option), (2, payment_purpose, required), (4, receiver_node_id, required), (5, htlcs, optional_vec), @@ -1002,6 +1012,16 @@ impl ClaimablePayments { .or_insert_with(|| { let htlcs = payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(); let sender_intended_value = payment.htlcs.first().map(|htlc| htlc.total_msat); + // Pick an "arbitrary" channel to block RAAs on until the `PaymentSent` + // event is processed, specifically the last channel to get claimed. + let durable_preimage_channel = payment.htlcs.last().map_or(None, |htlc| { + if let Some(node_id) = htlc.prev_hop.counterparty_node_id { + Some((htlc.prev_hop.outpoint, node_id, htlc.prev_hop.channel_id)) + } else { + None + } + }); + debug_assert!(durable_preimage_channel.is_some()); ClaimingPayment { amount_msat: payment.htlcs.iter().map(|source| source.value).sum(), payment_purpose: payment.purpose, @@ -1010,6 +1030,7 @@ impl ClaimablePayments { sender_intended_value, onion_fields: payment.onion_fields, payment_id: Some(payment_id), + durable_preimage_channel, } }).clone(); @@ -1099,6 +1120,10 @@ impl MaybeReadable for EventUnblockedChannel { } #[derive(Debug)] +/// Note that these run after all *non-blocked* [`ChannelMonitorUpdate`]s have been persisted. +/// Thus, they're primarily useful for (and currently only used for) claims, where the +/// [`ChannelMonitorUpdate`] we care about is a preimage update, which bypass the monitor update +/// blocking logic entirely and can never be blocked. pub(crate) enum MonitorUpdateCompletionAction { /// Indicates that a payment ultimately destined for us was claimed and we should emit an /// [`events::Event::PaymentClaimed`] to the user if we haven't yet generated such an event for @@ -1137,7 +1162,6 @@ pub(crate) enum MonitorUpdateCompletionAction { /// stored for later processing. FreeOtherChannelImmediately { downstream_counterparty_node_id: PublicKey, - downstream_funding_outpoint: OutPoint, blocking_action: RAAMonitorUpdateBlockingAction, downstream_channel_id: ChannelId, }, @@ -1152,11 +1176,8 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, // *immediately*. However, for simplicity we implement read/write here. (1, FreeOtherChannelImmediately) => { (0, downstream_counterparty_node_id, required), - (2, downstream_funding_outpoint, required), (4, blocking_action, upgradable_required), - // Note that by the time we get past the required read above, downstream_funding_outpoint will be - // filled in, so we can safely unwrap it here. - (5, downstream_channel_id, (default_value, ChannelId::v1_from_funding_outpoint(downstream_funding_outpoint.0.unwrap()))), + (5, downstream_channel_id, required), }, (2, EmitEventAndFreeOtherChannel) => { (0, event, upgradable_required), @@ -1169,22 +1190,48 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction, }, ); +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct PaymentCompleteUpdate { + counterparty_node_id: PublicKey, + channel_funding_outpoint: OutPoint, + channel_id: ChannelId, + htlc_id: SentHTLCId, +} + +impl_writeable_tlv_based!(PaymentCompleteUpdate, { + (1, channel_funding_outpoint, required), + (3, counterparty_node_id, required), + (5, channel_id, required), + (7, htlc_id, required), +}); + #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum EventCompletionAction { ReleaseRAAChannelMonitorUpdate { counterparty_node_id: PublicKey, - channel_funding_outpoint: OutPoint, + // Was required until LDK 0.2. Always filled in as `Some`. + channel_funding_outpoint: Option, channel_id: ChannelId, }, + + /// When a payment's resolution is communicated to the downstream logic via + /// [`Event::PaymentSent`] or [`Event::PaymentFailed`] we may want to mark the payment as + /// fully-resolved in the [`ChannelMonitor`], which we do via this action. + /// Note that this action will be dropped on downgrade to LDK prior to 0.2! + ReleasePaymentCompleteChannelMonitorUpdate(PaymentCompleteUpdate), } impl_writeable_tlv_based_enum!(EventCompletionAction, (0, ReleaseRAAChannelMonitorUpdate) => { - (0, channel_funding_outpoint, required), + (0, channel_funding_outpoint, option), (2, counterparty_node_id, required), - // Note that by the time we get past the required read above, channel_funding_outpoint will be - // filled in, so we can safely unwrap it here. - (3, channel_id, (default_value, ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.0.unwrap()))), + (3, channel_id, (default_value, { + if channel_funding_outpoint.is_none() { + Err(DecodeError::InvalidValue)? + } + ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.unwrap()) + })), } + {1, ReleasePaymentCompleteChannelMonitorUpdate} => (), ); /// The source argument which is passed to [`ChannelManager::claim_mpp_part`]. @@ -1194,7 +1241,7 @@ impl_writeable_tlv_based_enum!(EventCompletionAction, /// drop this and merge the two, however doing so may break upgrades for nodes which have pending /// forwarded payments. struct HTLCClaimSource { - counterparty_node_id: Option, + counterparty_node_id: PublicKey, funding_txo: OutPoint, channel_id: ChannelId, htlc_id: u64, @@ -1203,7 +1250,7 @@ struct HTLCClaimSource { impl From<&MPPClaimHTLCSource> for HTLCClaimSource { fn from(o: &MPPClaimHTLCSource) -> HTLCClaimSource { HTLCClaimSource { - counterparty_node_id: Some(o.counterparty_node_id), + counterparty_node_id: o.counterparty_node_id, funding_txo: o.funding_txo, channel_id: o.channel_id, htlc_id: o.htlc_id, @@ -1213,8 +1260,8 @@ impl From<&MPPClaimHTLCSource> for HTLCClaimSource { #[derive(Debug)] pub(crate) struct PendingMPPClaim { - channels_without_preimage: Vec<(PublicKey, OutPoint, ChannelId)>, - channels_with_preimage: Vec<(PublicKey, OutPoint, ChannelId)>, + channels_without_preimage: Vec<(PublicKey, ChannelId)>, + channels_with_preimage: Vec<(PublicKey, ChannelId)>, } #[derive(Clone, Debug, Hash, PartialEq, Eq)] @@ -1357,6 +1404,11 @@ pub(super) struct PeerState where SP::Target: SignerProvider { /// same `temporary_channel_id` (or final `channel_id` in the case of 0conf channels or prior /// to funding appearing on-chain), the downstream `ChannelMonitor` set is required to ensure /// duplicates do not occur, so such channels should fail without a monitor update completing. + /// + /// Note that these run after all *non-blocked* [`ChannelMonitorUpdate`]s have been persisted. + /// Thus, they're primarily useful for (and currently only used for) claims, where the + /// [`ChannelMonitorUpdate`] we care about is a preimage update, which bypass the monitor + /// update blocking logic entirely and can never be blocked. monitor_update_blocked_actions: BTreeMap>, /// If another channel's [`ChannelMonitorUpdate`] needs to complete before a channel we have /// with this peer can complete an RAA [`ChannelMonitorUpdate`] (e.g. because the RAA update @@ -2331,7 +2383,7 @@ where /// [`get_event_or_persistence_needed_future`]: Self::get_event_or_persistence_needed_future /// [`lightning-block-sync`]: https://docs.rs/lightning_block_sync/latest/lightning_block_sync /// [`lightning-transaction-sync`]: https://docs.rs/lightning_transaction_sync/latest/lightning_transaction_sync -/// [`lightning-background-processor`]: https://docs.rs/lightning_background_processor/lightning_background_processor +/// [`lightning-background-processor`]: https://docs.rs/lightning-background-processor/latest/lightning_background_processor /// [`list_channels`]: Self::list_channels /// [`list_usable_channels`]: Self::list_usable_channels /// [`create_channel`]: Self::create_channel @@ -3189,97 +3241,108 @@ macro_rules! emit_channel_ready_event { macro_rules! handle_monitor_update_completion { ($self: ident, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr) => { { let logger = WithChannelContext::from(&$self.logger, &$chan.context, None); - let mut updates = $chan.monitor_updating_restored(&&logger, - &$self.node_signer, $self.chain_hash, &$self.default_configuration, - $self.best_block.read().unwrap().height); - let counterparty_node_id = $chan.context.get_counterparty_node_id(); - let channel_update = if updates.channel_ready.is_some() && $chan.context.is_usable() { - // We only send a channel_update in the case where we are just now sending a - // channel_ready and the channel is in a usable state. We may re-send a - // channel_update later through the announcement_signatures process for public - // channels, but there's no reason not to just inform our counterparty of our fees - // now. - if let Ok(msg) = $self.get_channel_update_for_unicast($chan) { - Some(events::MessageSendEvent::SendChannelUpdate { - node_id: counterparty_node_id, - msg, - }) - } else { None } - } else { None }; let update_actions = $peer_state.monitor_update_blocked_actions .remove(&$chan.context.channel_id()).unwrap_or(Vec::new()); - let (htlc_forwards, decode_update_add_htlcs) = $self.handle_channel_resumption( - &mut $peer_state.pending_msg_events, $chan, updates.raa, - updates.commitment_update, updates.order, updates.accepted_htlcs, updates.pending_update_adds, - updates.funding_broadcastable, updates.channel_ready, - updates.announcement_sigs, updates.tx_signatures); - if let Some(upd) = channel_update { - $peer_state.pending_msg_events.push(upd); - } - - let channel_id = $chan.context.channel_id(); - let unbroadcasted_batch_funding_txid = $chan.context.unbroadcasted_batch_funding_txid(); - core::mem::drop($peer_state_lock); - core::mem::drop($per_peer_state_lock); - - // If the channel belongs to a batch funding transaction, the progress of the batch - // should be updated as we have received funding_signed and persisted the monitor. - if let Some(txid) = unbroadcasted_batch_funding_txid { - let mut funding_batch_states = $self.funding_batch_states.lock().unwrap(); - let mut batch_completed = false; - if let Some(batch_state) = funding_batch_states.get_mut(&txid) { - let channel_state = batch_state.iter_mut().find(|(chan_id, pubkey, _)| ( - *chan_id == channel_id && - *pubkey == counterparty_node_id - )); - if let Some(channel_state) = channel_state { - channel_state.2 = true; + if $chan.blocked_monitor_updates_pending() != 0 { + mem::drop($peer_state_lock); + mem::drop($per_peer_state_lock); + + log_debug!(logger, "Channel has blocked monitor updates, completing update actions but leaving channel blocked"); + $self.handle_monitor_update_completion_actions(update_actions); + } else { + log_debug!(logger, "Channel is open and awaiting update, resuming it"); + + let mut updates = $chan.monitor_updating_restored(&&logger, + &$self.node_signer, $self.chain_hash, &$self.default_configuration, + $self.best_block.read().unwrap().height); + let counterparty_node_id = $chan.context.get_counterparty_node_id(); + let channel_update = if updates.channel_ready.is_some() && $chan.context.is_usable() { + // We only send a channel_update in the case where we are just now sending a + // channel_ready and the channel is in a usable state. We may re-send a + // channel_update later through the announcement_signatures process for public + // channels, but there's no reason not to just inform our counterparty of our fees + // now. + if let Ok(msg) = $self.get_channel_update_for_unicast($chan) { + Some(events::MessageSendEvent::SendChannelUpdate { + node_id: counterparty_node_id, + msg, + }) + } else { None } + } else { None }; + + let (htlc_forwards, decode_update_add_htlcs) = $self.handle_channel_resumption( + &mut $peer_state.pending_msg_events, $chan, updates.raa, + updates.commitment_update, updates.order, updates.accepted_htlcs, updates.pending_update_adds, + updates.funding_broadcastable, updates.channel_ready, + updates.announcement_sigs, updates.tx_signatures); + if let Some(upd) = channel_update { + $peer_state.pending_msg_events.push(upd); + } + + let channel_id = $chan.context.channel_id(); + let unbroadcasted_batch_funding_txid = $chan.context.unbroadcasted_batch_funding_txid(); + core::mem::drop($peer_state_lock); + core::mem::drop($per_peer_state_lock); + + // If the channel belongs to a batch funding transaction, the progress of the batch + // should be updated as we have received funding_signed and persisted the monitor. + if let Some(txid) = unbroadcasted_batch_funding_txid { + let mut funding_batch_states = $self.funding_batch_states.lock().unwrap(); + let mut batch_completed = false; + if let Some(batch_state) = funding_batch_states.get_mut(&txid) { + let channel_state = batch_state.iter_mut().find(|(chan_id, pubkey, _)| ( + *chan_id == channel_id && + *pubkey == counterparty_node_id + )); + if let Some(channel_state) = channel_state { + channel_state.2 = true; + } else { + debug_assert!(false, "Missing channel batch state for channel which completed initial monitor update"); + } + batch_completed = batch_state.iter().all(|(_, _, completed)| *completed); } else { - debug_assert!(false, "Missing channel batch state for channel which completed initial monitor update"); + debug_assert!(false, "Missing batch state for channel which completed initial monitor update"); } - batch_completed = batch_state.iter().all(|(_, _, completed)| *completed); - } else { - debug_assert!(false, "Missing batch state for channel which completed initial monitor update"); - } - // When all channels in a batched funding transaction have become ready, it is not necessary - // to track the progress of the batch anymore and the state of the channels can be updated. - if batch_completed { - let removed_batch_state = funding_batch_states.remove(&txid).into_iter().flatten(); - let per_peer_state = $self.per_peer_state.read().unwrap(); - let mut batch_funding_tx = None; - for (channel_id, counterparty_node_id, _) in removed_batch_state { - if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { - let mut peer_state = peer_state_mutex.lock().unwrap(); - if let Some(ChannelPhase::Funded(chan)) = peer_state.channel_by_id.get_mut(&channel_id) { - batch_funding_tx = batch_funding_tx.or_else(|| chan.context.unbroadcasted_funding()); - chan.set_batch_ready(); - let mut pending_events = $self.pending_events.lock().unwrap(); - emit_channel_pending_event!(pending_events, chan); + // When all channels in a batched funding transaction have become ready, it is not necessary + // to track the progress of the batch anymore and the state of the channels can be updated. + if batch_completed { + let removed_batch_state = funding_batch_states.remove(&txid).into_iter().flatten(); + let per_peer_state = $self.per_peer_state.read().unwrap(); + let mut batch_funding_tx = None; + for (channel_id, counterparty_node_id, _) in removed_batch_state { + if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { + let mut peer_state = peer_state_mutex.lock().unwrap(); + if let Some(ChannelPhase::Funded(chan)) = peer_state.channel_by_id.get_mut(&channel_id) { + batch_funding_tx = batch_funding_tx.or_else(|| chan.context.unbroadcasted_funding()); + chan.set_batch_ready(); + let mut pending_events = $self.pending_events.lock().unwrap(); + emit_channel_pending_event!(pending_events, chan); + } } } - } - if let Some(tx) = batch_funding_tx { - log_info!($self.logger, "Broadcasting batch funding transaction with txid {}", tx.compute_txid()); - $self.tx_broadcaster.broadcast_transactions(&[&tx]); + if let Some(tx) = batch_funding_tx { + log_info!($self.logger, "Broadcasting batch funding transaction with txid {}", tx.compute_txid()); + $self.tx_broadcaster.broadcast_transactions(&[&tx]); + } } } - } - $self.handle_monitor_update_completion_actions(update_actions); + $self.handle_monitor_update_completion_actions(update_actions); - if let Some(forwards) = htlc_forwards { - $self.forward_htlcs(&mut [forwards][..]); - } - if let Some(decode) = decode_update_add_htlcs { - $self.push_decode_update_add_htlcs(decode); - } - $self.finalize_claims(updates.finalized_claimed_htlcs); - for failure in updates.failed_htlcs.drain(..) { - let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id }; - $self.fail_htlc_backwards_internal(&failure.0, &failure.1, &failure.2, receiver); + if let Some(forwards) = htlc_forwards { + $self.forward_htlcs(&mut [forwards][..]); + } + if let Some(decode) = decode_update_add_htlcs { + $self.push_decode_update_add_htlcs(decode); + } + $self.finalize_claims(updates.finalized_claimed_htlcs); + for failure in updates.failed_htlcs.drain(..) { + let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id }; + $self.fail_htlc_backwards_internal(&failure.0, &failure.1, &failure.2, receiver, None); + } } } } } @@ -3399,7 +3462,7 @@ macro_rules! handle_new_monitor_update { counterparty_node_id, in_flight_updates, idx, _internal_outer, { let _ = in_flight_updates.remove(idx); - if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 { + if in_flight_updates.is_empty() { handle_monitor_update_completion!($self, $peer_state_lock, $peer_state, $per_peer_state_lock, $chan); } }) @@ -3904,7 +3967,7 @@ where for htlc_source in failed_htlcs.drain(..) { let reason = HTLCFailReason::from_failure_code(0x4000 | 8); let receiver = HTLCDestination::NextHopChannel { node_id: Some(*counterparty_node_id), channel_id: *channel_id }; - self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver); + self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver, None); } if let Some(shutdown_result) = shutdown_result { @@ -3957,7 +4020,7 @@ where /// /// The `shutdown_script` provided will be used as the `scriptPubKey` for the closing transaction. /// Will fail if a shutdown script has already been set for this channel by - /// ['ChannelHandshakeConfig::commit_upfront_shutdown_pubkey`]. The given shutdown script must + /// [`ChannelHandshakeConfig::commit_upfront_shutdown_pubkey`]. The given shutdown script must /// also be compatible with our and the counterparty's features. /// /// May generate a [`SendShutdown`] message event on success, which should be relayed. @@ -3969,6 +4032,7 @@ where /// /// [`ChannelConfig::force_close_avoidance_max_fee_satoshis`]: crate::util::config::ChannelConfig::force_close_avoidance_max_fee_satoshis /// [`NonAnchorChannelFee`]: crate::chain::chaininterface::ConfirmationTarget::NonAnchorChannelFee + /// [`ChannelHandshakeConfig::commit_upfront_shutdown_pubkey`]: crate::util::config::ChannelHandshakeConfig::commit_upfront_shutdown_pubkey /// [`SendShutdown`]: crate::events::MessageSendEvent::SendShutdown pub fn close_channel_with_feerate_and_script(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, target_feerate_sats_per_1000_weight: Option, shutdown_script: Option) -> Result<(), APIError> { self.close_channel_internal(channel_id, counterparty_node_id, target_feerate_sats_per_1000_weight, shutdown_script) @@ -4027,7 +4091,7 @@ where let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source; let reason = HTLCFailReason::from_failure_code(0x4000 | 8); let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } if let Some((_, funding_txo, _channel_id, monitor_update)) = shutdown_res.monitor_update { debug_assert!(false, "This should have been handled in `locked_close_channel`"); @@ -5613,7 +5677,7 @@ where user_channel_id: Some(payment.prev_user_channel_id), outpoint: payment.prev_funding_outpoint, channel_id: payment.prev_channel_id, - counterparty_node_id: payment.prev_counterparty_node_id, + counterparty_node_id: Some(payment.prev_counterparty_node_id), htlc_id: payment.prev_htlc_id, incoming_packet_shared_secret: payment.forward_info.incoming_shared_secret, phantom_shared_secret: None, @@ -5623,7 +5687,7 @@ where let failure_reason = HTLCFailReason::from_failure_code(0x4000 | 10); let destination = HTLCDestination::UnknownNextHop { requested_forward_scid: short_channel_id }; - self.fail_htlc_backwards_internal(&htlc_source, &payment.forward_info.payment_hash, &failure_reason, destination); + self.fail_htlc_backwards_internal(&htlc_source, &payment.forward_info.payment_hash, &failure_reason, destination, None); } else { unreachable!() } // Only `PendingHTLCRouting::Forward`s are intercepted Ok(()) @@ -5738,7 +5802,7 @@ where // Process all of the forwards and failures for the channel in which the HTLCs were // proposed to as a batch. let pending_forwards = ( - incoming_scid, Some(incoming_counterparty_node_id), incoming_funding_txo, + incoming_scid, incoming_counterparty_node_id, incoming_funding_txo, incoming_channel_id, incoming_user_channel_id, htlc_forwards.drain(..).collect() ); self.forward_htlcs_without_forward_event(&mut [pending_forwards]); @@ -5774,7 +5838,7 @@ where let mut new_events = VecDeque::new(); let mut failed_forwards = Vec::new(); - let mut phantom_receives: Vec<(u64, Option, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)> = Vec::new(); + let mut phantom_receives: Vec<(u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)> = Vec::new(); { let mut forward_htlcs = new_hash_map(); mem::swap(&mut forward_htlcs, &mut self.forward_htlcs.lock().unwrap()); @@ -5804,7 +5868,7 @@ where user_channel_id: Some(prev_user_channel_id), channel_id: prev_channel_id, outpoint: prev_funding_outpoint, - counterparty_node_id: prev_counterparty_node_id, + counterparty_node_id: Some(prev_counterparty_node_id), htlc_id: prev_htlc_id, incoming_packet_shared_secret: incoming_shared_secret, phantom_shared_secret: $phantom_ss, @@ -5926,7 +5990,7 @@ where let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id: prev_short_channel_id, user_channel_id: Some(prev_user_channel_id), - counterparty_node_id: prev_counterparty_node_id, + counterparty_node_id: Some(prev_counterparty_node_id), channel_id: prev_channel_id, outpoint: prev_funding_outpoint, htlc_id: prev_htlc_id, @@ -6101,7 +6165,7 @@ where prev_hop: HTLCPreviousHopData { short_channel_id: prev_short_channel_id, user_channel_id: Some(prev_user_channel_id), - counterparty_node_id: prev_counterparty_node_id, + counterparty_node_id: Some(prev_counterparty_node_id), channel_id: prev_channel_id, outpoint: prev_funding_outpoint, htlc_id: prev_htlc_id, @@ -6305,7 +6369,13 @@ where &self.pending_events, &self.logger, |args| self.send_payment_along_path(args)); for (htlc_source, payment_hash, failure_reason, destination) in failed_forwards.drain(..) { - self.fail_htlc_backwards_internal(&htlc_source, &payment_hash, &failure_reason, destination); + self.fail_htlc_backwards_internal( + &htlc_source, + &payment_hash, + &failure_reason, + destination, + None, + ); } self.forward_htlcs(&mut phantom_receives); @@ -6350,9 +6420,7 @@ where let mut peer_state_lock = peer_state_mutex.lock().unwrap(); let peer_state = &mut *peer_state_lock; if let Some(ChannelPhase::Funded(chan)) = peer_state.channel_by_id.get_mut(&channel_id) { - if chan.blocked_monitor_updates_pending() == 0 { - handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); - } + handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); } else { let update_actions = peer_state.monitor_update_blocked_actions .remove(&channel_id).unwrap_or(Vec::new()); @@ -6667,7 +6735,7 @@ where let source = HTLCSource::PreviousHopData(htlc_source.0.clone()); let reason = HTLCFailReason::from_failure_code(23); let receiver = HTLCDestination::FailedPayment { payment_hash: htlc_source.1 }; - self.fail_htlc_backwards_internal(&source, &htlc_source.1, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &htlc_source.1, &reason, receiver, None); } for (err, counterparty_node_id) in handle_errors.drain(..) { @@ -6732,7 +6800,7 @@ where let reason = self.get_htlc_fail_reason_from_failure_code(failure_code, &htlc); let source = HTLCSource::PreviousHopData(htlc.prev_hop); let receiver = HTLCDestination::FailedPayment { payment_hash: *payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } } } @@ -6811,18 +6879,26 @@ where for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) { let reason = HTLCFailReason::reason(failure_code, onion_failure_data.clone()); let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id }; - self.fail_htlc_backwards_internal(&htlc_src, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&htlc_src, &payment_hash, &reason, receiver, None); } } - fn fail_htlc_backwards_internal(&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, destination: HTLCDestination) { - let push_forward_event = self.fail_htlc_backwards_internal_without_forward_event(source, payment_hash, onion_error, destination); + fn fail_htlc_backwards_internal( + &self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, + destination: HTLCDestination, + from_monitor_update_completion: Option, + ) { + let push_forward_event = self.fail_htlc_backwards_internal_without_forward_event(source, payment_hash, onion_error, destination, from_monitor_update_completion); if push_forward_event { self.push_pending_forwards_ev(); } } /// Fails an HTLC backwards to the sender of it to us. /// Note that we do not assume that channels corresponding to failed HTLCs are still available. - fn fail_htlc_backwards_internal_without_forward_event(&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, destination: HTLCDestination) -> bool { + fn fail_htlc_backwards_internal_without_forward_event( + &self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, + destination: HTLCDestination, + mut from_monitor_update_completion: Option, + ) -> bool { // Ensure that no peer state channel storage lock is held when calling this function. // This ensures that future code doesn't introduce a lock-order requirement for // `forward_htlcs` to be locked after the `per_peer_state` peer locks, which calling @@ -6843,9 +6919,37 @@ where let mut push_forward_event; match source { HTLCSource::OutboundRoute { ref path, ref session_priv, ref payment_id, .. } => { - push_forward_event = self.pending_outbound_payments.fail_htlc(source, payment_hash, onion_error, path, - session_priv, payment_id, self.probing_cookie_secret, &self.secp_ctx, - &self.pending_events, &self.logger); + push_forward_event = self.pending_outbound_payments.fail_htlc( + source, + payment_hash, + onion_error, + path, + session_priv, + payment_id, + self.probing_cookie_secret, + &self.secp_ctx, + &self.pending_events, + &self.logger, + &mut from_monitor_update_completion, + ); + if let Some(update) = from_monitor_update_completion { + // If `fail_htlc` didn't `take` the post-event action, we should go ahead and + // complete it here as the failure was duplicative - we've already handled it. + // This can happen in rare cases where a MonitorUpdate is replayed after + // restart because a ChannelMonitor wasn't persisted after it was applied (even + // though the ChannelManager was). + // For such cases, we also check that there's no existing pending event to + // complete this action already, which we let finish instead. + let action = + EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update); + let have_action = { + let pending_events = self.pending_events.lock().unwrap(); + pending_events.iter().any(|(_, act)| act.as_ref() == Some(&action)) + }; + if !have_action { + self.handle_post_event_actions([action]); + } + } }, HTLCSource::PreviousHopData(HTLCPreviousHopData { ref short_channel_id, ref htlc_id, ref incoming_packet_shared_secret, @@ -6960,7 +7064,7 @@ where let reason = self.get_htlc_fail_reason_from_failure_code(FailureCode::InvalidOnionPayload(None), &htlc); let source = HTLCSource::PreviousHopData(htlc.prev_hop); let receiver = HTLCDestination::FailedPayment { payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } return; } @@ -7024,7 +7128,7 @@ where let pending_mpp_claim_ptr_opt = if sources.len() > 1 { let mut channels_without_preimage = Vec::with_capacity(mpp_parts.len()); for part in mpp_parts.iter() { - let chan = (part.counterparty_node_id, part.funding_txo, part.channel_id); + let chan = (part.counterparty_node_id, part.channel_id); if !channels_without_preimage.contains(&chan) { channels_without_preimage.push(chan); } @@ -7066,7 +7170,7 @@ where let source = HTLCSource::PreviousHopData(htlc.prev_hop); let reason = HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data); let receiver = HTLCDestination::FailedPayment { payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver, None); } self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash); } @@ -7088,6 +7192,14 @@ where let short_to_chan_info = self.short_to_chan_info.read().unwrap(); short_to_chan_info.get(&prev_hop.short_channel_id).map(|(cp_id, _)| *cp_id) }); + let counterparty_node_id = if let Some(node_id) = counterparty_node_id { + node_id + } else { + let payment_hash: PaymentHash = payment_preimage.into(); + panic!( + "Prior to upgrading to LDK 0.1, all pending HTLCs forwarded by LDK 0.0.123 or before must be resolved. It appears at least the HTLC with payment_hash {payment_hash} (preimage {payment_preimage}) was not resolved. Please downgrade to LDK 0.0.125 and resolve the HTLC prior to upgrading.", + ); + }; let htlc_source = HTLCClaimSource { counterparty_node_id, @@ -7122,16 +7234,13 @@ where const MISSING_MON_ERROR: &'static str = "If we're going to claim an HTLC against a channel, we should always have *some* state for the channel, even if just the latest ChannelMonitor update_id. This failure indicates we need to claim an HTLC from a channel for which we did not have a ChannelMonitor at startup and didn't create one while running."; - // Note here that `peer_state_opt` is always `Some` if `prev_hop.counterparty_node_id` is - // `Some`. This is relied on in the closed-channel case below. - let mut peer_state_opt = prev_hop.counterparty_node_id.as_ref().map( - |counterparty_node_id| per_peer_state.get(counterparty_node_id) - .map(|peer_mutex| peer_mutex.lock().unwrap()) - .expect(MISSING_MON_ERROR) - ); + let mut peer_state_lock = per_peer_state + .get(&prev_hop.counterparty_node_id) + .map(|peer_mutex| peer_mutex.lock().unwrap()) + .expect(MISSING_MON_ERROR); - if let Some(peer_state_lock) = peer_state_opt.as_mut() { - let peer_state = &mut **peer_state_lock; + { + let peer_state = &mut *peer_state_lock; if let hash_map::Entry::Occupied(mut chan_phase_entry) = peer_state.channel_by_id.entry(chan_id) { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); @@ -7148,8 +7257,15 @@ where if let Some(raa_blocker) = raa_blocker_opt { peer_state.actions_blocking_raa_monitor_updates.entry(chan_id).or_insert_with(Vec::new).push(raa_blocker); } - handle_new_monitor_update!(self, prev_hop.funding_txo, monitor_update, peer_state_opt, - peer_state, per_peer_state, chan); + handle_new_monitor_update!( + self, + prev_hop.funding_txo, + monitor_update, + peer_state_lock, + peer_state, + per_peer_state, + chan + ); } UpdateFulfillCommitFetch::DuplicateClaim {} => { let (action_opt, raa_blocker_opt) = completion_action(None, true); @@ -7158,12 +7274,21 @@ where // payment claim from a `ChannelMonitor`. In some cases (MPP or // if the HTLC was only recently removed) we make such claims // after an HTLC has been removed from a channel entirely, and - // thus the RAA blocker has long since completed. + // thus the RAA blocker may have long since completed. // - // In any other case, the RAA blocker must still be present and - // blocking RAAs. - debug_assert!(during_init || - peer_state.actions_blocking_raa_monitor_updates.get(&chan_id).unwrap().contains(&raa_blocker)); + // However, its possible that the `ChannelMonitorUpdate` containing + // the preimage never completed and is still pending. In that case, + // we need to re-add the RAA blocker, which we do here. Handling + // the post-update action, below, will remove it again. + // + // In any other case (i.e. not during startup), the RAA blocker + // must still be present and blocking RAAs. + let actions = &mut peer_state.actions_blocking_raa_monitor_updates; + let actions_list = actions.entry(chan_id).or_insert_with(Vec::new); + if !actions_list.contains(&raa_blocker) { + debug_assert!(during_init); + actions_list.push(raa_blocker); + } } let action = if let Some(action) = action_opt { action @@ -7171,15 +7296,32 @@ where return; }; - mem::drop(peer_state_opt); + // If there are monitor updates in flight, we may be in the case + // described above, replaying a claim on startup which needs an RAA + // blocker to remain blocked. Thus, in such a case we simply push the + // post-update action to the blocked list and move on. + // In any case, we should err on the side of caution and not process + // the post-update action no matter the situation. + let in_flight_mons = peer_state.in_flight_monitor_updates.get(&prev_hop.funding_txo); + if in_flight_mons.map(|mons| !mons.is_empty()).unwrap_or(false) { + peer_state + .monitor_update_blocked_actions + .entry(chan_id) + .or_insert_with(Vec::new) + .push(action); + return; + } + + mem::drop(peer_state_lock); log_trace!(logger, "Completing monitor update completion action for channel {} as claim was redundant: {:?}", chan_id, action); if let MonitorUpdateCompletionAction::FreeOtherChannelImmediately { downstream_counterparty_node_id: node_id, - downstream_funding_outpoint: _, - blocking_action: blocker, downstream_channel_id: channel_id, - } = action { + blocking_action: blocker, + downstream_channel_id: channel_id, + } = action + { if let Some(peer_state_mtx) = per_peer_state.get(&node_id) { let mut peer_state = peer_state_mtx.lock().unwrap(); if let Some(blockers) = peer_state @@ -7217,16 +7359,7 @@ where } } - if prev_hop.counterparty_node_id.is_none() { - let payment_hash: PaymentHash = payment_preimage.into(); - panic!( - "Prior to upgrading to LDK 0.1, all pending HTLCs forwarded by LDK 0.0.123 or before must be resolved. It appears at least the HTLC with payment_hash {} (preimage {}) was not resolved. Please downgrade to LDK 0.0.125 and resolve the HTLC prior to upgrading.", - payment_hash, - payment_preimage, - ); - } - let counterparty_node_id = prev_hop.counterparty_node_id.expect("Checked immediately above"); - let mut peer_state = peer_state_opt.expect("peer_state_opt is always Some when the counterparty_node_id is Some"); + let peer_state = &mut *peer_state_lock; let update_id = if let Some(latest_update_id) = peer_state.closed_channel_monitor_update_ids.get_mut(&chan_id) { *latest_update_id = latest_update_id.saturating_add(1); @@ -7241,7 +7374,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let preimage_update = ChannelMonitorUpdate { update_id, - counterparty_node_id: Some(counterparty_node_id), + counterparty_node_id: Some(prev_hop.counterparty_node_id), updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage, payment_info, @@ -7266,7 +7399,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ // Given the fact that we're in a bit of a weird edge case, its worth hashing the preimage // to include the `payment_hash` in the log metadata here. let payment_hash = payment_preimage.into(); - let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(chan_id), Some(payment_hash)); + let logger = WithContext::from( + &self.logger, + Some(prev_hop.counterparty_node_id), + Some(chan_id), + Some(payment_hash), + ); if let Some(action) = action_opt { log_trace!(logger, "Tracking monitor update completion action for closed channel {}: {:?}", @@ -7275,8 +7413,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } handle_new_monitor_update!( - self, prev_hop.funding_txo, preimage_update, peer_state, peer_state, per_peer_state, - counterparty_node_id, chan_id, POST_CHANNEL_CLOSE + self, + prev_hop.funding_txo, + preimage_update, + peer_state_lock, + peer_state, + per_peer_state, + prev_hop.counterparty_node_id, + chan_id, + POST_CHANNEL_CLOSE ); } @@ -7289,20 +7434,60 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ startup_replay: bool, next_channel_counterparty_node_id: Option, next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option, ) { + debug_assert_eq!( + startup_replay, + !self.background_events_processed_since_startup.load(Ordering::Acquire) + ); + let htlc_id = SentHTLCId::from_source(&source); match source { HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => { - debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire), + debug_assert!(!startup_replay, "We don't support claim_htlc claims during startup - monitors may not be available yet"); if let Some(pubkey) = next_channel_counterparty_node_id { debug_assert_eq!(pubkey, path.hops[0].pubkey); } - let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint: next_channel_outpoint, channel_id: next_channel_id, - counterparty_node_id: path.hops[0].pubkey, + let mut ev_completion_action = if from_onchain { + if let Some(counterparty_node_id) = next_channel_counterparty_node_id { + let release = PaymentCompleteUpdate { + counterparty_node_id, + channel_funding_outpoint: next_channel_outpoint, + channel_id: next_channel_id, + htlc_id, + }; + Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(release)) + } else { + log_warn!(self.logger, "Missing counterparty node id in monitor when trying to re-claim a payment resolved on chain. This may lead to redundant PaymentSent events on restart"); + None + } + } else { + Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { + channel_funding_outpoint: Some(next_channel_outpoint), + channel_id: next_channel_id, + counterparty_node_id: path.hops[0].pubkey, + }) + }; + self.pending_outbound_payments.claim_htlc( + payment_id, + payment_preimage, + session_priv, + path, + from_onchain, + &mut ev_completion_action, + &self.pending_events, + &self.logger, + ); + // If an event was generated, `claim_htlc` set `ev_completion_action` to None, if + // not, we should go ahead and run it now (as the claim was duplicative), at least + // if a PaymentClaimed event with the same action isn't already pending. + let have_action = if ev_completion_action.is_some() { + let pending_events = self.pending_events.lock().unwrap(); + pending_events.iter().any(|(_, act)| *act == ev_completion_action) + } else { + false }; - self.pending_outbound_payments.claim_htlc(payment_id, payment_preimage, - session_priv, path, from_onchain, ev_completion_action, &self.pending_events, - &self.logger); + if !have_action { + self.handle_post_event_actions(ev_completion_action); + } }, HTLCSource::PreviousHopData(hop_data) => { let prev_channel_id = hop_data.channel_id; @@ -7338,7 +7523,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ if let Some(other_chan) = chan_to_release { (Some(MonitorUpdateCompletionAction::FreeOtherChannelImmediately { downstream_counterparty_node_id: other_chan.counterparty_node_id, - downstream_funding_outpoint: other_chan.funding_txo, downstream_channel_id: other_chan.channel_id, blocking_action: other_chan.blocking_action, }), None) @@ -7398,17 +7582,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ if *pending_claim == claim_ptr { let mut pending_claim_state_lock = pending_claim.0.lock().unwrap(); let pending_claim_state = &mut *pending_claim_state_lock; - pending_claim_state.channels_without_preimage.retain(|(cp, op, cid)| { + pending_claim_state.channels_without_preimage.retain(|(cp, cid)| { let this_claim = *cp == counterparty_node_id && *cid == chan_id; if this_claim { - pending_claim_state.channels_with_preimage.push((*cp, *op, *cid)); + pending_claim_state.channels_with_preimage.push((*cp, *cid)); false } else { true } }); if pending_claim_state.channels_without_preimage.is_empty() { - for (cp, op, cid) in pending_claim_state.channels_with_preimage.iter() { - let freed_chan = (*cp, *op, *cid, blocker.clone()); + for (cp, cid) in pending_claim_state.channels_with_preimage.iter() { + let freed_chan = (*cp, *cid, blocker.clone()); freed_channels.push(freed_chan); } } @@ -7432,6 +7616,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ sender_intended_value: sender_intended_total_msat, onion_fields, payment_id, + durable_preimage_channel, }) = payment { let event = events::Event::PaymentClaimed { payment_hash, @@ -7443,7 +7628,18 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ onion_fields, payment_id, }; - let event_action = (event, None); + let action = if let Some((outpoint, counterparty_node_id, channel_id)) + = durable_preimage_channel + { + Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { + channel_funding_outpoint: Some(outpoint), + counterparty_node_id, + channel_id, + }) + } else { + None + }; + let event_action = (event, action); let mut pending_events = self.pending_events.lock().unwrap(); // If we're replaying a claim on startup we may end up duplicating an event // that's already in our queue, so check before we push another one. The @@ -7460,17 +7656,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ self.pending_events.lock().unwrap().push_back((event, None)); if let Some(unblocked) = downstream_counterparty_and_funding_outpoint { self.handle_monitor_update_release( - unblocked.counterparty_node_id, unblocked.funding_txo, - unblocked.channel_id, Some(unblocked.blocking_action), + unblocked.counterparty_node_id, + unblocked.channel_id, + Some(unblocked.blocking_action), ); } }, MonitorUpdateCompletionAction::FreeOtherChannelImmediately { - downstream_counterparty_node_id, downstream_funding_outpoint, downstream_channel_id, blocking_action, + downstream_counterparty_node_id, downstream_channel_id, blocking_action, } => { self.handle_monitor_update_release( downstream_counterparty_node_id, - downstream_funding_outpoint, downstream_channel_id, Some(blocking_action), ); @@ -7478,8 +7674,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } } - for (node_id, funding_outpoint, channel_id, blocker) in freed_channels { - self.handle_monitor_update_release(node_id, funding_outpoint, channel_id, Some(blocker)); + for (node_id, channel_id, blocker) in freed_channels { + self.handle_monitor_update_release(node_id, channel_id, Some(blocker)); } } @@ -7492,7 +7688,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ funding_broadcastable: Option, channel_ready: Option, announcement_sigs: Option, tx_signatures: Option - ) -> (Option<(u64, Option, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)>, Option<(u64, Vec)>) { + ) -> (Option<(u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)>, Option<(u64, Vec)>) { let logger = WithChannelContext::from(&self.logger, &channel.context, None); log_trace!(logger, "Handling channel resumption for channel {} with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures", &channel.context.channel_id(), @@ -7511,7 +7707,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let mut htlc_forwards = None; if !pending_forwards.is_empty() { htlc_forwards = Some(( - short_channel_id, Some(channel.context.get_counterparty_node_id()), + short_channel_id, channel.context.get_counterparty_node_id(), channel.context.get_funding_txo().unwrap(), channel.context.channel_id(), channel.context.get_user_id(), pending_forwards )); @@ -7630,12 +7826,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ if let Some(ChannelPhase::Funded(chan)) = peer_state.channel_by_id.get_mut(channel_id) { if chan.is_awaiting_monitor_update() { - if chan.blocked_monitor_updates_pending() == 0 { - log_trace!(logger, "Channel is open and awaiting update, resuming it"); - handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); - } else { - log_trace!(logger, "Channel is open and awaiting update, leaving it blocked due to a blocked monitor update"); - } + handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); } else { log_trace!(logger, "Channel is open but not awaiting update"); } @@ -8637,7 +8828,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ for htlc_source in dropped_htlcs.drain(..) { let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id: msg.channel_id }; let reason = HTLCFailReason::from_failure_code(0x4000 | 8); - self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver); + self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver, None); } if let Some(shutdown_res) = finish_shutdown { self.finish_close_channel(shutdown_res); @@ -8960,13 +9151,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } #[inline] - fn forward_htlcs(&self, per_source_pending_forwards: &mut [(u64, Option, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)]) { + fn forward_htlcs(&self, per_source_pending_forwards: &mut [(u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)]) { let push_forward_event = self.forward_htlcs_without_forward_event(per_source_pending_forwards); if push_forward_event { self.push_pending_forwards_ev() } } #[inline] - fn forward_htlcs_without_forward_event(&self, per_source_pending_forwards: &mut [(u64, Option, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)]) -> bool { + fn forward_htlcs_without_forward_event(&self, per_source_pending_forwards: &mut [(u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)]) -> bool { let mut push_forward_event = false; for &mut (prev_short_channel_id, prev_counterparty_node_id, prev_funding_outpoint, prev_channel_id, prev_user_channel_id, ref mut pending_forwards) in per_source_pending_forwards { let mut new_intercept_events = VecDeque::new(); @@ -9017,7 +9208,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id: prev_short_channel_id, user_channel_id: Some(prev_user_channel_id), - counterparty_node_id: prev_counterparty_node_id, + counterparty_node_id: Some(prev_counterparty_node_id), outpoint: prev_funding_outpoint, channel_id: prev_channel_id, htlc_id: prev_htlc_id, @@ -9048,7 +9239,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } for (htlc_source, payment_hash, failure_reason, destination) in failed_intercept_forwards.drain(..) { - push_forward_event |= self.fail_htlc_backwards_internal_without_forward_event(&htlc_source, &payment_hash, &failure_reason, destination); + push_forward_event |= self.fail_htlc_backwards_internal_without_forward_event( + &htlc_source, + &payment_hash, + &failure_reason, + destination, + None, + ); } if !new_intercept_events.is_empty() { @@ -9084,16 +9281,20 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ /// the [`ChannelMonitorUpdate`] in question. fn raa_monitor_updates_held(&self, actions_blocking_raa_monitor_updates: &BTreeMap>, - channel_funding_outpoint: OutPoint, channel_id: ChannelId, counterparty_node_id: PublicKey + channel_id: ChannelId, counterparty_node_id: PublicKey, ) -> bool { actions_blocking_raa_monitor_updates .get(&channel_id).map(|v| !v.is_empty()).unwrap_or(false) || self.pending_events.lock().unwrap().iter().any(|(_, action)| { - action == &Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint, - channel_id, - counterparty_node_id, - }) + if let Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate { + channel_funding_outpoint: _, + channel_id: ev_channel_id, + counterparty_node_id: ev_counterparty_node_id + }) = action { + *ev_channel_id == channel_id && *ev_counterparty_node_id == counterparty_node_id + } else { + false + } }) } @@ -9106,10 +9307,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let mut peer_state_lck = peer_state_mtx.lock().unwrap(); let peer_state = &mut *peer_state_lck; - if let Some(chan) = peer_state.channel_by_id.get(&channel_id) { - return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates, - chan.context().get_funding_txo().unwrap(), channel_id, counterparty_node_id); - } + assert!(peer_state.channel_by_id.contains_key(&channel_id)); + return self.raa_monitor_updates_held( + &peer_state.actions_blocking_raa_monitor_updates, + channel_id, + counterparty_node_id, + ); } false } @@ -9128,11 +9331,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); let funding_txo_opt = chan.context.get_funding_txo(); - let mon_update_blocked = if let Some(funding_txo) = funding_txo_opt { - self.raa_monitor_updates_held( - &peer_state.actions_blocking_raa_monitor_updates, funding_txo, msg.channel_id, - *counterparty_node_id) - } else { false }; + let mon_update_blocked = self.raa_monitor_updates_held( + &peer_state.actions_blocking_raa_monitor_updates, msg.channel_id, + *counterparty_node_id); let (htlcs_to_fail, monitor_update_opt) = try_chan_phase_entry!(self, peer_state, chan.revoke_and_ack(&msg, &self.fee_estimator, &&logger, mon_update_blocked), chan_phase_entry); if let Some(monitor_update) = monitor_update_opt { @@ -9385,7 +9586,25 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ log_trace!(logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash); let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id }; let reason = HTLCFailReason::from_failure_code(0x4000 | 8); - self.fail_htlc_backwards_internal(&htlc_update.source, &htlc_update.payment_hash, &reason, receiver); + let completion_update = + if let Some(counterparty_node_id) = counterparty_node_id { + Some(PaymentCompleteUpdate { + counterparty_node_id, + channel_funding_outpoint: funding_outpoint, + channel_id, + htlc_id: SentHTLCId::from_source(&htlc_update.source), + }) + } else { + log_warn!(self.logger, "Missing counterparty node id in monitor when trying to re-claim a payment resolved on chain. This may lead to redundant PaymentSent events on restart"); + None + }; + self.fail_htlc_backwards_internal( + &htlc_update.source, + &htlc_update.payment_hash, + &reason, + receiver, + completion_update, + ); } }, MonitorEvent::HolderForceClosed(_) | MonitorEvent::HolderForceClosedWithInfo { .. } => { @@ -10270,7 +10489,7 @@ where } /// Pays for an [`Offer`] looked up using [BIP 353] Human Readable Names resolved by the DNS - /// resolver(s) at `dns_resolvers` which resolve names according to bLIP 32. + /// resolver(s) at `dns_resolvers` which resolve names according to [bLIP 32]. /// /// If the wallet supports paying on-chain schemes, you should instead use /// [`OMNameResolver::resolve_name`] and [`OMNameResolver::handle_dnssec_proof_for_uri`] (by @@ -10288,18 +10507,19 @@ where /// /// To revoke the request, use [`ChannelManager::abandon_payment`] prior to receiving the /// invoice. If abandoned, or an invoice isn't received in a reasonable amount of time, the - /// payment will fail with an [`Event::InvoiceRequestFailed`]. + /// payment will fail with an [`PaymentFailureReason::UserAbandoned`] or + /// [`PaymentFailureReason::InvoiceRequestExpired`], respectively. /// /// # Privacy /// /// For payer privacy, uses a derived payer id and uses [`MessageRouter::create_blinded_paths`] - /// to construct a [`BlindedPath`] for the reply path. For further privacy implications, see the + /// to construct a [`BlindedMessagePath`] for the reply path. For further privacy implications, see the /// docs of the parameterized [`Router`], which implements [`MessageRouter`]. /// /// # Limitations /// /// Requires a direct connection to the given [`Destination`] as well as an introduction node in - /// [`Offer::paths`] or to [`Offer::signing_pubkey`], if empty. A similar restriction applies to + /// [`Offer::paths`] or to [`Offer::issuer_signing_pubkey`], if empty. A similar restriction applies to /// the responding [`Bolt12Invoice::payment_paths`]. /// /// # Errors @@ -10307,8 +10527,13 @@ where /// Errors if: /// - a duplicate `payment_id` is provided given the caveats in the aforementioned link, /// + /// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki + /// [bLIP 32]: https://github.com/lightning/blips/blob/master/blip-0032.md /// [`Bolt12Invoice::payment_paths`]: crate::offers::invoice::Bolt12Invoice::payment_paths /// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments + /// [`BlindedMessagePath`]: crate::blinded_path::message::BlindedMessagePath + /// [`PaymentFailureReason::UserAbandoned`]: crate::events::PaymentFailureReason::UserAbandoned + /// [`PaymentFailureReason::InvoiceRequestRejected`]: crate::events::PaymentFailureReason::InvoiceRequestRejected #[cfg(feature = "dnssec")] pub fn pay_for_offer_from_human_readable_name( &self, name: HumanReadableName, amount_msats: u64, payment_id: PaymentId, @@ -10643,14 +10868,37 @@ where self.pending_outbound_payments.clear_pending_payments() } + #[cfg(any(test, feature = "_test_utils"))] + pub(crate) fn get_and_clear_pending_raa_blockers( + &self, + ) -> Vec<(ChannelId, Vec)> { + let per_peer_state = self.per_peer_state.read().unwrap(); + let mut pending_blockers = Vec::new(); + + for (_peer_pubkey, peer_state_mutex) in per_peer_state.iter() { + let mut peer_state = peer_state_mutex.lock().unwrap(); + + for (chan_id, actions) in peer_state.actions_blocking_raa_monitor_updates.iter() { + // Only collect the non-empty actions into `pending_blockers`. + if !actions.is_empty() { + pending_blockers.push((chan_id.clone(), actions.clone())); + } + } + + peer_state.actions_blocking_raa_monitor_updates.clear(); + } + + pending_blockers + } + /// When something which was blocking a channel from updating its [`ChannelMonitor`] (e.g. an /// [`Event`] being handled) completes, this should be called to restore the channel to normal /// operation. It will double-check that nothing *else* is also blocking the same channel from /// making progress and then let any blocked [`ChannelMonitorUpdate`]s fly. - fn handle_monitor_update_release(&self, counterparty_node_id: PublicKey, - channel_funding_outpoint: OutPoint, channel_id: ChannelId, - mut completed_blocker: Option) { - + fn handle_monitor_update_release( + &self, counterparty_node_id: PublicKey, channel_id: ChannelId, + mut completed_blocker: Option + ) { let logger = WithContext::from( &self.logger, Some(counterparty_node_id), Some(channel_id), None ); @@ -10669,7 +10917,7 @@ where } if self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates, - channel_funding_outpoint, channel_id, counterparty_node_id) { + channel_id, counterparty_node_id) { // Check that, while holding the peer lock, we don't have anything else // blocking monitor updates for this channel. If we do, release the monitor // update(s) when those blockers complete. @@ -10681,7 +10929,7 @@ where if let hash_map::Entry::Occupied(mut chan_phase_entry) = peer_state.channel_by_id.entry( channel_id) { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { - debug_assert_eq!(chan.context.get_funding_txo().unwrap(), channel_funding_outpoint); + let channel_funding_outpoint = chan.funding_outpoint(); if let Some((monitor_update, further_update_exists)) = chan.unblock_next_blocked_monitor_update() { log_debug!(logger, "Unlocking monitor updating for channel {} and updating monitor", channel_id); @@ -10707,14 +10955,71 @@ where } } - fn handle_post_event_actions(&self, actions: Vec) { - for action in actions { + fn handle_post_event_actions>(&self, actions: I) { + for action in actions.into_iter() { match action { EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint, channel_id, counterparty_node_id + channel_funding_outpoint: _, + channel_id, + counterparty_node_id, } => { - self.handle_monitor_update_release(counterparty_node_id, channel_funding_outpoint, channel_id, None); - } + let startup_complete = + self.background_events_processed_since_startup.load(Ordering::Acquire); + debug_assert!(startup_complete); + self.handle_monitor_update_release(counterparty_node_id, channel_id, None); + }, + EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate( + PaymentCompleteUpdate { + counterparty_node_id, + channel_funding_outpoint, + channel_id, + htlc_id, + }, + ) => { + let per_peer_state = self.per_peer_state.read().unwrap(); + let mut peer_state = per_peer_state + .get(&counterparty_node_id) + .map(|state| state.lock().unwrap()) + .expect("Channels originating a payment resolution must have peer state"); + let update_id = peer_state + .closed_channel_monitor_update_ids + .get_mut(&channel_id) + .expect("Channels originating a payment resolution must have a monitor"); + *update_id += 1; + + let update = ChannelMonitorUpdate { + update_id: *update_id, + channel_id: Some(channel_id), + counterparty_node_id: Some(counterparty_node_id), + updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete { + htlc: htlc_id, + }], + }; + + let during_startup = + !self.background_events_processed_since_startup.load(Ordering::Acquire); + if during_startup { + let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id, + funding_txo: channel_funding_outpoint, + channel_id, + update, + }; + self.pending_background_events.lock().unwrap().push(event); + } else { + handle_new_monitor_update!( + self, + channel_funding_outpoint, + update, + peer_state, + peer_state, + per_peer_state, + counterparty_node_id, + channel_id, + POST_CHANNEL_CLOSE + ); + } + }, } } } @@ -11083,7 +11388,7 @@ where // Most of our tests were written when we only broadcasted // `channel_announcement`s once and then never re-broadcasted // them again, so disable the re-broadcasting entirely in tests - #[cfg(test)] + #[cfg(any(test, feature = "_test_utils"))] { should_announce = announcement_sigs.is_some(); } @@ -11184,7 +11489,7 @@ where htlc_id: htlc.prev_htlc_id, incoming_packet_shared_secret: htlc.forward_info.incoming_shared_secret, phantom_shared_secret: None, - counterparty_node_id: htlc.prev_counterparty_node_id, + counterparty_node_id: Some(htlc.prev_counterparty_node_id), outpoint: htlc.prev_funding_outpoint, channel_id: htlc.prev_channel_id, blinded_failure: htlc.forward_info.routing.blinded_failure(), @@ -11212,7 +11517,7 @@ where } for (source, payment_hash, reason, destination) in timed_out_htlcs.drain(..) { - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, destination); + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, destination, None); } } @@ -12700,7 +13005,7 @@ impl_writeable_tlv_based!(PendingAddHTLCInfo, { // Note that by the time we get past the required read for type 6 above, prev_funding_outpoint will be // filled in, so we can safely unwrap it here. (7, prev_channel_id, (default_value, ChannelId::v1_from_funding_outpoint(prev_funding_outpoint.0.unwrap()))), - (9, prev_counterparty_node_id, option), + (9, prev_counterparty_node_id, required), }); impl Writeable for HTLCForwardInfo { @@ -13277,7 +13582,7 @@ where log_error!(logger, " The ChannelMonitor for channel {} is at counterparty commitment transaction number {} but the ChannelManager is at counterparty commitment transaction number {}.", &channel.context.channel_id(), monitor.get_cur_counterparty_commitment_number(), channel.get_cur_counterparty_commitment_transaction_number()); } - let mut shutdown_result = channel.context.force_shutdown(true, ClosureReason::OutdatedChannelManager); + let shutdown_result = channel.context.force_shutdown(true, ClosureReason::OutdatedChannelManager); if shutdown_result.unbroadcasted_batch_funding_txid.is_some() { return Err(DecodeError::InvalidValue); } @@ -13298,7 +13603,9 @@ where counterparty_node_id, funding_txo, channel_id, update }); } - failed_htlcs.append(&mut shutdown_result.dropped_outbound_htlcs); + for (source, hash, cp_id, chan_id) in shutdown_result.dropped_outbound_htlcs { + failed_htlcs.push((source, hash, cp_id, chan_id, None)); + } channel_closures.push_back((events::Event::ChannelClosed { channel_id: channel.context.channel_id(), user_channel_id: channel.context.get_user_id(), @@ -13325,7 +13632,13 @@ where log_info!(logger, "Failing HTLC with hash {} as it is missing in the ChannelMonitor for channel {} but was present in the (stale) ChannelManager", &channel.context.channel_id(), &payment_hash); - failed_htlcs.push((channel_htlc_source.clone(), *payment_hash, channel.context.get_counterparty_node_id(), channel.context.channel_id())); + failed_htlcs.push(( + channel_htlc_source.clone(), + *payment_hash, + channel.context.get_counterparty_node_id(), + channel.context.channel_id(), + None, + )); } } } else { @@ -13766,10 +14079,14 @@ where // payments which are still in-flight via their on-chain state. // We only rebuild the pending payments map if we were most recently serialized by // 0.0.102+ + // + // First we rebuild all pending payments, then separately re-claim and re-fail pending + // payments. This avoids edge-cases around MPP payments resulting in redundant actions. for (_, monitor) in args.channel_monitors.iter() { let counterparty_opt = outpoint_to_peer.get(&monitor.get_funding_txo().0); + // outpoint_to_peer missing the funding outpoint implies the channel is closed if counterparty_opt.is_none() { - for (htlc_source, (htlc, _)) in monitor.get_pending_or_resolved_outbound_htlcs() { + for (htlc_source, (htlc, _)) in monitor.get_all_current_outbound_htlcs() { let logger = WithChannelMonitor::from(&args.logger, monitor, Some(htlc.payment_hash)); if let HTLCSource::OutboundRoute { payment_id, session_priv, path, .. } = htlc_source { if path.hops.is_empty() { @@ -13784,8 +14101,14 @@ where ); } } + } + } + for (_, monitor) in args.channel_monitors.iter() { + let counterparty_opt = outpoint_to_peer.get(&monitor.get_funding_txo().0); + if counterparty_opt.is_none() { for (htlc_source, (htlc, preimage_opt)) in monitor.get_all_current_outbound_htlcs() { let logger = WithChannelMonitor::from(&args.logger, monitor, Some(htlc.payment_hash)); + let htlc_id = SentHTLCId::from_source(&htlc_source); match htlc_source { HTLCSource::PreviousHopData(prev_hop_data) => { let pending_forward_matches_htlc = |info: &PendingAddHTLCInfo| { @@ -13837,27 +14160,102 @@ where HTLCSource::OutboundRoute { payment_id, session_priv, path, .. } => { if let Some(preimage) = preimage_opt { let pending_events = Mutex::new(pending_events_read); - // Note that we set `from_onchain` to "false" here, - // deliberately keeping the pending payment around forever. - // Given it should only occur when we have a channel we're - // force-closing for being stale that's okay. - // The alternative would be to wipe the state when claiming, - // generating a `PaymentPathSuccessful` event but regenerating - // it and the `PaymentSent` on every restart until the - // `ChannelMonitor` is removed. - let compl_action = - EventCompletionAction::ReleaseRAAChannelMonitorUpdate { - channel_funding_outpoint: monitor.get_funding_txo().0, - channel_id: monitor.channel_id(), - counterparty_node_id: path.hops[0].pubkey, + let mut compl_action = + if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() { + let update = PaymentCompleteUpdate { + counterparty_node_id, + channel_funding_outpoint: monitor.get_funding_txo().0, + channel_id: monitor.channel_id(), + htlc_id, + }; + Some( + EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update) + ) + } else { + log_warn!(logger, "Missing counterparty node id in monitor when trying to re-claim a payment resolved on chain. This may lead to redundant PaymentSent events on restart"); + None }; - pending_outbounds.claim_htlc(payment_id, preimage, session_priv, - path, false, compl_action, &pending_events, &&logger); + pending_outbounds.claim_htlc( + payment_id, + preimage, + session_priv, + path, + true, + &mut compl_action, + &pending_events, + &&logger, + ); + // If the completion action was not consumed, then there was no + // payment to claim, and we need to tell the `ChannelMonitor` + // we don't need to hear about the HTLC again, at least as long + // as the PaymentSent event isn't still sitting around in our + // event queue. + let have_action = if compl_action.is_some() { + let pending_events = pending_events.lock().unwrap(); + pending_events.iter().any(|(_, act)| *act == compl_action) + } else { + false + }; + if !have_action && compl_action.is_some() { + if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() { + let mut peer_state = per_peer_state + .get(&counterparty_node_id) + .map(|state| state.lock().unwrap()) + .expect("Channels originating a preimage must have peer state"); + let update_id = peer_state + .closed_channel_monitor_update_ids + .get_mut(&monitor.channel_id()) + .expect("Channels originating a preimage must have a monitor"); + *update_id += 1; + + pending_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id: counterparty_node_id, + funding_txo: monitor.get_funding_txo().0, + channel_id: monitor.channel_id(), + update: ChannelMonitorUpdate { + update_id: *update_id, + channel_id: Some(monitor.channel_id()), + updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete { + htlc: htlc_id, + }], + counterparty_node_id: Some(counterparty_node_id), + }, + }); + } + } pending_events_read = pending_events.into_inner().unwrap(); } }, } } + for (htlc_source, payment_hash) in monitor.get_onchain_failed_outbound_htlcs() { + if let Some(node_id) = monitor.get_counterparty_node_id() { + log_info!( + args.logger, + "Failing HTLC with payment hash {} as it was resolved on-chain.", + payment_hash + ); + let completion_action = Some(PaymentCompleteUpdate { + counterparty_node_id: node_id, + channel_funding_outpoint: monitor.get_funding_txo().0, + channel_id: monitor.channel_id(), + htlc_id: SentHTLCId::from_source(&htlc_source), + }); + failed_htlcs.push(( + htlc_source, + payment_hash, + node_id, + monitor.channel_id(), + completion_action, + )); + } else { + log_warn!( + args.logger, + "Unable to fail HTLC with payment hash {} after being resolved on-chain due to incredibly old monitor.", + payment_hash + ); + } + } } // Whether the downstream channel was closed or not, try to re-apply any payment @@ -14290,8 +14688,10 @@ where } } - let mut channels_without_preimage = payment_claim.mpp_parts.iter() - .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id)) + let mut channels_without_preimage = payment_claim + .mpp_parts + .iter() + .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.channel_id)) .collect::>(); // If we have multiple MPP parts which were received over the same channel, // we only track it once as once we get a preimage durably in the @@ -14419,26 +14819,35 @@ where } let mut pending_events = channel_manager.pending_events.lock().unwrap(); let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap()); - pending_events.push_back((events::Event::PaymentClaimed { - receiver_node_id, - payment_hash, - purpose: payment.purpose, - amount_msat: claimable_amt_msat, - htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), - sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), - onion_fields: payment.onion_fields, - payment_id: Some(payment_id), - }, None)); + pending_events.push_back(( + events::Event::PaymentClaimed { + receiver_node_id, + payment_hash, + purpose: payment.purpose, + amount_msat: claimable_amt_msat, + htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), + sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), + onion_fields: payment.onion_fields, + payment_id: Some(payment_id), + }, + // Note that we don't bother adding a EventCompletionAction here to + // ensure the `PaymentClaimed` event is durable processed as this + // should only be hit for particularly old channels and we don't have + // enough information to generate such an action. + None, + )); } } } } - for htlc_source in failed_htlcs.drain(..) { - let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source; - let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id }; + for htlc_source in failed_htlcs { + let (source, hash, counterparty_id, channel_id, ev_action) = htlc_source; + let receiver = + HTLCDestination::NextHopChannel { node_id: Some(counterparty_id), channel_id }; let reason = HTLCFailReason::from_failure_code(0x4000 | 8); - channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + channel_manager + .fail_htlc_backwards_internal(&source, &hash, &reason, receiver, ev_action); } for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding, downstream_channel_id) in pending_claims_to_replay { diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 420978ad5fc..e06940a21d1 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -674,6 +674,11 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> { panic!("Had {} excess added monitors on node {}", added_monitors.len(), self.logger.id); } + let raa_blockers = self.node.get_and_clear_pending_raa_blockers(); + if !raa_blockers.is_empty() { + panic!( "Had excess RAA blockers on node {}: {:?}", self.logger.id, raa_blockers); + } + // Check that if we serialize the network graph, we can deserialize it again. let network_graph = { let mut w = test_utils::TestVecWriter(Vec::new()); @@ -897,7 +902,7 @@ pub fn get_htlc_update_msgs(node: &Node, recipient: &PublicKey) -> msgs::Commitm assert_eq!(node_id, recipient); (*updates).clone() }, - _ => panic!("Unexpected event"), + _ => panic!("Unexpected event {events:?}"), } } @@ -2327,17 +2332,21 @@ macro_rules! expect_payment_claimed { } } -pub fn expect_payment_sent>(node: &H, - expected_payment_preimage: PaymentPreimage, expected_fee_msat_opt: Option>, - expect_per_path_claims: bool, expect_post_ev_mon_update: bool, +pub fn expect_payment_sent>( + node: &H, expected_payment_preimage: PaymentPreimage, + expected_fee_msat_opt: Option>, expect_per_path_claims: bool, + expect_post_ev_mon_update: bool, ) { + if expect_post_ev_mon_update { + check_added_monitors(node, 0); + } let events = node.node().get_and_clear_pending_events(); let expected_payment_hash = PaymentHash( bitcoin::hashes::sha256::Hash::hash(&expected_payment_preimage.0).to_byte_array()); if expect_per_path_claims { - assert!(events.len() > 1); + assert!(events.len() > 1, "{events:?}"); } else { - assert_eq!(events.len(), 1); + assert_eq!(events.len(), 1, "{events:?}"); } if expect_post_ev_mon_update { check_added_monitors(node, 1); @@ -2530,6 +2539,7 @@ pub struct PaymentFailedConditions<'a> { pub(crate) expected_blamed_scid: Option, pub(crate) expected_blamed_chan_closed: Option, pub(crate) expected_mpp_parts_remain: bool, + pub(crate) from_mon_update: bool, } impl<'a> PaymentFailedConditions<'a> { @@ -2539,6 +2549,7 @@ impl<'a> PaymentFailedConditions<'a> { expected_blamed_scid: None, expected_blamed_chan_closed: None, expected_mpp_parts_remain: false, + from_mon_update: false, } } pub fn mpp_parts_remain(mut self) -> Self { @@ -2557,6 +2568,10 @@ impl<'a> PaymentFailedConditions<'a> { self.expected_htlc_error_data = Some((code, data)); self } + pub fn from_mon_update(mut self) -> Self { + self.from_mon_update = true; + self + } } #[cfg(test)] @@ -2642,8 +2657,19 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>( node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e> ) { + if conditions.from_mon_update { + check_added_monitors(node, 0); + } let events = node.node.get_and_clear_pending_events(); - expect_payment_failed_conditions_event(events, expected_payment_hash, expected_payment_failed_permanently, conditions); + if conditions.from_mon_update { + check_added_monitors(node, 1); + } + expect_payment_failed_conditions_event( + events, + expected_payment_hash, + expected_payment_failed_permanently, + conditions, + ); } pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId { @@ -2937,13 +2963,36 @@ impl<'a, 'b, 'c, 'd> ClaimAlongRouteArgs<'a, 'b, 'c, 'd> { } } +macro_rules! single_fulfill_commit_from_ev { + ($ev: expr) => { + match $ev { + &MessageSendEvent::UpdateHTLCs { + ref node_id, + updates: + msgs::CommitmentUpdate { + ref update_add_htlcs, + ref update_fulfill_htlcs, + ref update_fail_htlcs, + ref update_fail_malformed_htlcs, + ref update_fee, + ref commitment_signed, + }, + } => { + assert!(update_add_htlcs.is_empty()); + assert_eq!(update_fulfill_htlcs.len(), 1); + assert!(update_fail_htlcs.is_empty()); + assert!(update_fail_malformed_htlcs.is_empty()); + assert!(update_fee.is_none()); + ((update_fulfill_htlcs[0].clone(), commitment_signed.clone()), node_id.clone()) + }, + _ => panic!("Unexpected event"), + } + }; +} + pub fn pass_claimed_payment_along_route(args: ClaimAlongRouteArgs) -> u64 { - let ClaimAlongRouteArgs { - origin_node, expected_paths, expected_extra_fees, expected_min_htlc_overpay, skip_last, - payment_preimage: our_payment_preimage, allow_1_msat_fee_overpay, custom_tlvs, - } = args; - let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events(); - assert_eq!(claim_event.len(), 1); + let claim_event = args.expected_paths[0].last().unwrap().node.get_and_clear_pending_events(); + assert_eq!(claim_event.len(), 1, "{claim_event:?}"); #[allow(unused)] let mut fwd_amt_msat = 0; match claim_event[0] { @@ -2957,11 +3006,11 @@ pub fn pass_claimed_payment_along_route(args: ClaimAlongRouteArgs) -> u64 { ref onion_fields, .. } => { - assert_eq!(preimage, our_payment_preimage); - assert_eq!(htlcs.len(), expected_paths.len()); // One per path. + assert_eq!(preimage, args.payment_preimage); + assert_eq!(htlcs.len(), args.expected_paths.len()); // One per path. assert_eq!(htlcs.iter().map(|h| h.value_msat).sum::(), amount_msat); - assert_eq!(onion_fields.as_ref().unwrap().custom_tlvs, custom_tlvs); - check_claimed_htlcs_match_route(origin_node, expected_paths, htlcs); + assert_eq!(onion_fields.as_ref().unwrap().custom_tlvs, args.custom_tlvs); + check_claimed_htlcs_match_route(args.origin_node, args.expected_paths, htlcs); fwd_amt_msat = amount_msat; }, Event::PaymentClaimed { @@ -2974,54 +3023,64 @@ pub fn pass_claimed_payment_along_route(args: ClaimAlongRouteArgs) -> u64 { ref onion_fields, .. } => { - assert_eq!(&payment_hash.0, &Sha256::hash(&our_payment_preimage.0)[..]); - assert_eq!(htlcs.len(), expected_paths.len()); // One per path. + assert_eq!(&payment_hash.0, &Sha256::hash(&args.payment_preimage.0)[..]); + assert_eq!(htlcs.len(), args.expected_paths.len()); // One per path. assert_eq!(htlcs.iter().map(|h| h.value_msat).sum::(), amount_msat); - assert_eq!(onion_fields.as_ref().unwrap().custom_tlvs, custom_tlvs); - check_claimed_htlcs_match_route(origin_node, expected_paths, htlcs); + assert_eq!(onion_fields.as_ref().unwrap().custom_tlvs, args.custom_tlvs); + check_claimed_htlcs_match_route(args.origin_node, args.expected_paths, htlcs); fwd_amt_msat = amount_msat; } _ => panic!(), } - check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len()); + check_added_monitors(args.expected_paths[0].last().unwrap(), args.expected_paths.len()); - let mut expected_total_fee_msat = 0; - - macro_rules! msgs_from_ev { - ($ev: expr) => { - match $ev { - &MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => { - assert!(update_add_htlcs.is_empty()); - assert_eq!(update_fulfill_htlcs.len(), 1); - assert!(update_fail_htlcs.is_empty()); - assert!(update_fail_malformed_htlcs.is_empty()); - assert!(update_fee.is_none()); - ((update_fulfill_htlcs[0].clone(), commitment_signed.clone()), node_id.clone()) - }, - _ => panic!("Unexpected event"), - } - } - } - let mut per_path_msgs: Vec<((msgs::UpdateFulfillHTLC, msgs::CommitmentSigned), PublicKey)> = Vec::with_capacity(expected_paths.len()); - let mut events = expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), expected_paths.len()); + let mut per_path_msgs: Vec<( + (msgs::UpdateFulfillHTLC, msgs::CommitmentSigned), + PublicKey, + )> = Vec::with_capacity(args.expected_paths.len()); + let mut events = args.expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), args.expected_paths.len()); if events.len() == 1 { - per_path_msgs.push(msgs_from_ev!(&events[0])); + per_path_msgs.push(single_fulfill_commit_from_ev!(&events[0])); } else { - for expected_path in expected_paths.iter() { + for expected_path in args.expected_paths.iter() { // For MPP payments, we want the fulfill message from the payee to the penultimate hop in the // path. let penultimate_hop_node_id = expected_path.iter().rev().skip(1).next() .map(|n| n.node.get_our_node_id()) - .unwrap_or(origin_node.node.get_our_node_id()); + .unwrap_or(args.origin_node.node.get_our_node_id()); let ev = remove_first_msg_event_to_node(&penultimate_hop_node_id, &mut events); - per_path_msgs.push(msgs_from_ev!(&ev)); + per_path_msgs.push(single_fulfill_commit_from_ev!(&ev)); } } - for (i, (expected_route, (path_msgs, next_hop))) in expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() { + pass_claimed_payment_along_route_from_ev(fwd_amt_msat, per_path_msgs, args) +} + +pub fn pass_claimed_payment_along_route_from_ev( + each_htlc_claim_amt_msat: u64, + mut per_path_msgs: Vec<((msgs::UpdateFulfillHTLC, msgs::CommitmentSigned), PublicKey)>, + args: ClaimAlongRouteArgs, +) -> u64 { + let ClaimAlongRouteArgs { + origin_node, + expected_paths, + expected_extra_fees, + expected_min_htlc_overpay, + skip_last, + payment_preimage: our_payment_preimage, + allow_1_msat_fee_overpay, + .. + } = args; + + let mut fwd_amt_msat = each_htlc_claim_amt_msat; + let mut expected_total_fee_msat = 0; + + for (i, (expected_route, (path_msgs, next_hop))) in + expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() + { let mut next_msgs = Some(path_msgs); let mut expected_next_node = next_hop; @@ -3072,7 +3131,7 @@ pub fn pass_claimed_payment_along_route(args: ClaimAlongRouteArgs) -> u64 { let new_next_msgs = if $new_msgs { let events = $node.node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); - let (res, nexthop) = msgs_from_ev!(&events[0]); + let (res, nexthop) = single_fulfill_commit_from_ev!(&events[0]); expected_next_node = nexthop; Some(res) } else { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 2cbf04a40ff..41a5fb7cd4d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2920,7 +2920,8 @@ fn claim_htlc_outputs() { // ANTI_REORG_DELAY confirmations. mine_transaction(&nodes[1], accepted_claim); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], payment_hash_2, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], payment_hash_2, false, conditions); } get_announce_close_broadcast_events(&nodes, 0, 1); assert_eq!(nodes[0].node.list_channels().len(), 0); @@ -3374,6 +3375,7 @@ fn test_htlc_on_chain_success() { check_closed_broadcast!(nodes[0], true); check_added_monitors!(nodes[0], 1); let events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 2); assert_eq!(events.len(), 5); let mut first_claimed = false; for event in events { @@ -3705,7 +3707,13 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use check_added_monitors!(nodes[1], 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + check_added_monitors(&nodes[1], 0); let events = nodes[1].node.get_and_clear_pending_events(); + if deliver_bs_raa { + check_added_monitors(&nodes[1], 1); + } else { + check_added_monitors(&nodes[1], 0); + } assert_eq!(events.len(), if deliver_bs_raa { 3 + nodes.len() - 1 } else { 4 + nodes.len() }); assert!(events.iter().any(|ev| matches!( ev, @@ -3721,7 +3729,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use ))); nodes[1].node.process_pending_htlc_forwards(); - check_added_monitors!(nodes[1], 1); + check_added_monitors(&nodes[1], 1); let mut events = nodes[1].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), if deliver_bs_raa { 4 } else { 3 }); @@ -5058,7 +5066,8 @@ fn test_static_spendable_outputs_timeout_tx() { mine_transaction(&nodes[1], &node_txn[0]); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], our_payment_hash, false, conditions); let spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager); assert_eq!(spend_txn.len(), 3); // SpendableOutput: remote_commitment_tx.to_remote, timeout_tx.output @@ -5917,7 +5926,8 @@ fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() { mine_transaction(&nodes[0], &htlc_timeout); connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1); - expect_payment_failed!(nodes[0], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, false, conditions); // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor let spend_txn = check_spendable_outputs!(nodes[0], node_cfgs[0].keys_manager); @@ -6004,7 +6014,8 @@ fn test_key_derivation_params() { mine_transaction(&nodes[0], &htlc_timeout); connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1); - expect_payment_failed!(nodes[0], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, false, conditions); // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor let new_keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet); @@ -7479,7 +7490,9 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + check_added_monitors(&nodes[0], 0); let events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 2); // Only 2 PaymentPathFailed events should show up, over-dust HTLC has to be failed by timeout tx assert_eq!(events.len(), 4); let mut first_failed = false; @@ -7539,12 +7552,14 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { // We fail dust-HTLC 1 by broadcast of local commitment tx mine_transaction(&nodes[0], &as_commitment_tx[0]); check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed, [nodes[1].node.get_our_node_id()], 100000); + check_closed_broadcast!(nodes[0], true); + check_added_monitors(&nodes[0], 1); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], dust_hash, false, conditions); connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS - ANTI_REORG_DELAY); - check_closed_broadcast!(nodes[0], true); - check_added_monitors!(nodes[0], 1); + check_added_monitors!(nodes[0], 0); assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); timeout_tx.push(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0].clone()); assert_eq!(timeout_tx[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); @@ -7552,7 +7567,8 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); mine_transaction(&nodes[0], &timeout_tx[0]); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], non_dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], non_dust_hash, false, conditions); } else { // We fail dust-HTLC 1 by broadcast of remote commitment tx. If revoked, fail also non-dust HTLC mine_transaction(&nodes[0], &bs_commitment_tx[0]); @@ -7567,7 +7583,8 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { check_spends!(timeout_tx[0], bs_commitment_tx[0]); // For both a revoked or non-revoked commitment transaction, after ANTI_REORG_DELAY the // dust HTLC should have been failed. - expect_payment_failed!(nodes[0], dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], dust_hash, false, conditions); if !revoked { assert_eq!(timeout_tx[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); @@ -7578,7 +7595,8 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { mine_transaction(&nodes[0], &timeout_tx[0]); assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], non_dust_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], non_dust_hash, false, conditions); } } @@ -8306,7 +8324,11 @@ fn test_channel_conf_timeout() { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let _funding_tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 100_000); + let funding_tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 100_000); + + // Inbound channels which haven't advanced state at all and never were funded will generate + // claimable `Balance`s until they're closed. + assert!(!nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty()); // The outbound node should wait forever for confirmation: // This matches `channel::FUNDING_CONF_DEADLINE_BLOCKS` and BOLT 2's suggested timeout, thus is @@ -8319,6 +8341,10 @@ fn test_channel_conf_timeout() { check_added_monitors!(nodes[1], 0); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors(); + assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1); + assert!(!nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty()); + connect_blocks(&nodes[1], 1); check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::FundingTimedOut, [nodes[0].node.get_our_node_id()], 1000000); @@ -8331,6 +8357,22 @@ fn test_channel_conf_timeout() { }, _ => panic!("Unexpected event"), } + + // Once an inbound never-confirmed channel is closed, it will no longer generate any claimable + // `Balance`s. + assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty()); + + // Once the funding times out the monitor should be immediately archived. + nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors(); + assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 0); + assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty()); + + // Remove the corresponding outputs and transactions the chain source is + // watching. This is to make sure the `Drop` function assertions pass. + nodes[1].chain_source.remove_watched_txn_and_outputs( + OutPoint { txid: funding_tx.compute_txid(), index: 0 }, + funding_tx.output[0].script_pubkey.clone(), + ); } #[test] @@ -9174,7 +9216,8 @@ fn test_htlc_no_detection() { connect_block(&nodes[0], &create_dummy_block(nodes[0].best_block_hash(), 42, vec![htlc_timeout.clone()])); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], our_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, false, conditions); } fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain_before_fulfill: bool) { diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index d105d69edd2..a375d90b9ed 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -10,6 +10,7 @@ //! Further functional tests which test blockchain reorganizations. use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SpendableOutputDescriptor}; +use crate::chain::Watch; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, Balance, BalanceSource, ChannelMonitorUpdateStep}; use crate::chain::transaction::OutPoint; use crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight}; @@ -162,7 +163,8 @@ fn revoked_output_htlc_resolution_timing() { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], payment_hash_1, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], payment_hash_1, false, conditions); } #[test] @@ -267,7 +269,7 @@ fn archive_fully_resolved_monitors() { // Finally, we process the pending `MonitorEvent` from nodes[0], allowing the `ChannelMonitor` // to be archived `ARCHIVAL_DELAY_BLOCKS` blocks later. - expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors(); assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1); connect_blocks(&nodes[0], ARCHIVAL_DELAY_BLOCKS - 1); @@ -694,7 +696,8 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], dust_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], dust_payment_hash, false, conditions); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); // After ANTI_REORG_DELAY, A will consider its balance fully spendable and generate a @@ -717,8 +720,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { mine_transaction(&nodes[0], &b_broadcast_txn[0]); if prev_commitment_tx { expect_payment_path_successful!(nodes[0]); + check_added_monitors(&nodes[0], 1); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); } assert_eq!(sorted_vec(vec![sent_htlc_balance.clone(), sent_htlc_timeout_balance.clone()]), sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); @@ -750,7 +754,8 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); assert_eq!(Vec::::new(), nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); - expect_payment_failed!(nodes[0], timeout_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], timeout_payment_hash, false, conditions); test_spendable_output(&nodes[0], &a_htlc_timeout_tx, false); @@ -965,7 +970,7 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { // Now confirm nodes[1]'s HTLC claim, giving nodes[0] the preimage. Note that the "maybe // claimable" balance remains until we see ANTI_REORG_DELAY blocks. mine_transaction(&nodes[0], &bs_htlc_claim_txn[0]); - expect_payment_sent(&nodes[0], payment_preimage_2, None, true, false); + expect_payment_sent(&nodes[0], payment_preimage_2, None, true, true); assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value, confirmation_height: node_a_commitment_claimable, @@ -1007,7 +1012,8 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { // panicked as described in the test introduction. This will remove the "maybe claimable" // spendable output as nodes[1] has fully claimed the second HTLC. connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[0], payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value, @@ -1238,7 +1244,8 @@ fn test_no_preimage_inbound_htlc_balances() { // Once as_htlc_timeout_claim[0] reaches ANTI_REORG_DELAY confirmations, we should get a // payment failure event. connect_blocks(&nodes[0], ANTI_REORG_DELAY - 2); - expect_payment_failed!(nodes[0], to_b_failed_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], to_b_failed_payment_hash, false, conditions); connect_blocks(&nodes[0], 1); assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { @@ -1286,7 +1293,8 @@ fn test_no_preimage_inbound_htlc_balances() { sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2); - expect_payment_failed!(nodes[1], to_a_failed_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], to_a_failed_payment_hash, false, conditions); assert_eq!(vec![b_received_htlc_balance.clone()], nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); @@ -1547,7 +1555,9 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ connect_blocks(&nodes[1], 3); test_spendable_output(&nodes[1], &as_revoked_txn[0], false); + check_added_monitors(&nodes[1], 0); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 2); expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), missing_htlc_payment_hash, false, PaymentFailedConditions::new()); expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), @@ -1556,7 +1566,9 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ connect_blocks(&nodes[1], 1); if confirm_htlc_spend_first { test_spendable_output(&nodes[1], &claim_txn[0], false); + check_added_monitors(&nodes[1], 0); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 2); expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), live_payment_hash, false, PaymentFailedConditions::new()); expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), @@ -1569,7 +1581,9 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ test_spendable_output(&nodes[1], &claim_txn[1], false); } else { test_spendable_output(&nodes[1], &claim_txn[0], false); + check_added_monitors(&nodes[1], 0); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[1], 2); expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), live_payment_hash, false, PaymentFailedConditions::new()); expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), @@ -2017,7 +2031,7 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { as_revoked_txn[1].clone() }; mine_transaction(&nodes[1], &htlc_success_claim); - expect_payment_sent(&nodes[1], claimed_payment_preimage, None, true, false); + expect_payment_sent(&nodes[1], claimed_payment_preimage, None, true, true); let mut claim_txn_2 = nodes[1].tx_broadcaster.txn_broadcast(); // Once B sees the HTLC-Success transaction it splits its claim transaction into two, though in @@ -2118,7 +2132,8 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); // We shouldn't fail the payment until we spend the output connect_blocks(&nodes[1], 5); - expect_payment_failed!(nodes[1], revoked_payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], revoked_payment_hash, false, conditions); let spendable_output_events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); assert_eq!(spendable_output_events.len(), 2); for event in spendable_output_events { @@ -2591,7 +2606,8 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); - expect_payment_failed!(nodes[0], payment_hash_1.unwrap(), false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash_1.unwrap(), false, conditions); connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32); @@ -3200,3 +3216,562 @@ fn test_update_replay_panics() { monitor.update_monitor(&updates[2], &nodes[1].tx_broadcaster, &nodes[1].fee_estimator, &nodes[1].logger).unwrap(); monitor.update_monitor(&updates[3], &nodes[1].tx_broadcaster, &nodes[1].fee_estimator, &nodes[1].logger).unwrap(); } + +#[test] +fn test_claim_event_never_handled() { + // When a payment is claimed, the `ChannelMonitorUpdate` containing the payment preimage goes + // out and when it completes the `PaymentClaimed` event is generated. If the channel then + // progresses forward a few steps, the payment preimage will then eventually be removed from + // the channel. By that point, we have to make sure that the `PaymentClaimed` event has been + // handled (which ensures the user has maked the payment received). + // Otherwise, it is possible that, on restart, we load with a stale `ChannelManager` which + // doesn't have the `PaymentClaimed` event and it needs to rebuild it from the + // `ChannelMonitor`'s payment information and preimage. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let persister; + let new_chain_mon; + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes_1_reload; + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_a_id = nodes[0].node.get_our_node_id(); + let node_b_id = nodes[1].node.get_our_node_id(); + + let init_node_ser = nodes[1].node.encode(); + + let chan = create_announced_chan_between_nodes(&nodes, 0, 1); + + // Send the payment we'll ultimately test the PaymentClaimed event for. + let (preimage_a, payment_hash_a, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + nodes[1].node.claim_funds(preimage_a); + check_added_monitors(&nodes[1], 1); + + let mut updates = get_htlc_update_msgs(&nodes[1], &node_a_id); + nodes[0].node.handle_update_fulfill_htlc(node_b_id, &updates.update_fulfill_htlcs[0]); + expect_payment_sent(&nodes[0], preimage_a, None, false, false); + + nodes[0].node.handle_commitment_signed(node_b_id, &updates.commitment_signed); + check_added_monitors(&nodes[0], 1); + + // Once the `PaymentClaimed` event is generated, further RAA `ChannelMonitorUpdate`s will be + // blocked until it is handled, ensuring we never get far enough to remove the preimage. + let (raa, cs) = get_revoke_commit_msgs(&nodes[0], &node_b_id); + nodes[1].node.handle_revoke_and_ack(node_a_id, &raa); + nodes[1].node.handle_commitment_signed(node_a_id, &cs); + check_added_monitors(&nodes[1], 0); + + // The last RAA here should be blocked waiting on us to handle the PaymentClaimed event before + // continuing. Otherwise, we'd be able to make enough progress that the payment preimage is + // removed from node A's `ChannelMonitor`. This leaves us unable to make further progress. + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // Finally, reload node B with an empty `ChannelManager` and check that we get the + // `PaymentClaimed` event. + let chan_0_monitor_serialized = get_monitor!(nodes[1], chan.2).encode(); + let mons = &[&chan_0_monitor_serialized[..]]; + reload_node!(nodes[1], &init_node_ser, mons, persister, new_chain_mon, nodes_1_reload); + + expect_payment_claimed!(nodes[1], payment_hash_a, 1_000_000); + // The reload logic spuriously generates a redundant payment preimage-containing + // `ChannelMonitorUpdate`. + check_added_monitors(&nodes[1], 2); +} + +fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool) { + // `MonitorEvent`s aren't delivered to the `ChannelManager` in a durable fashion - if the + // `ChannelManager` fetches the pending `MonitorEvent`s, then the `ChannelMonitor` gets + // persisted (i.e. due to a block update) then the node crashes, prior to persisting the + // `ChannelManager` again, the `MonitorEvent` and its effects on the `ChannelManger` will be + // lost. This isn't likely in a sync persist environment, but in an async one this could be an + // issue. + // + // Note that this is only an issue for closed channels - `MonitorEvent`s only inform the + // `ChannelManager` that a channel is closed (which the `ChannelManager` will learn on startup + // or when it next tries to advance the channel state), that `ChannelMonitorUpdate` writes + // completed (which the `ChannelManager` will detect on startup), or that HTLCs resolved + // on-chain post closure. Of the three, only the last is problematic to lose prior to a reload. + // + // Here we test that losing `MonitorEvent`s that contain HTLC resolution preimages does not + // cause us to lose funds or miss a `PaymentSent` event. + let mut cfg = test_default_channel_config(); + cfg.manually_accept_inbound_channels = true; + cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + let cfgs = [Some(cfg.clone()), Some(cfg.clone()), Some(cfg.clone())]; + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let persister; + let new_chain_mon; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &cfgs); + let node_b_reload; + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let coinbase_tx = provide_anchor_reserves(&nodes); + + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0).2; + let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).2; + + let (preimage_a, hash_a, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000); + let (preimage_b, hash_b, ..) = route_payment(&nodes[1], &[&nodes[2]], 1_000_000); + + nodes[1].node.peer_disconnected(nodes[2].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[1].node.get_our_node_id()); + + nodes[2].node.claim_funds(preimage_a); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], hash_a, 1_000_000); + nodes[2].node.claim_funds(preimage_b); + check_added_monitors(&nodes[2], 1); + expect_payment_claimed!(nodes[2], hash_b, 1_000_000); + + // Force-close the channel, confirming a commitment transaction then letting C claim the HTLCs. + let message = "Closed".to_owned(); + nodes[2] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_b_id, message) + .unwrap(); + check_added_monitors(&nodes[2], 1); + let c_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; + check_closed_event!(nodes[2], 1, c_reason, [node_b_id], 1_000_000); + check_closed_broadcast!(nodes[2], true); + + let cs_commit_tx = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(cs_commit_tx.len(), 1); + + let message = "Closed".to_owned(); + nodes[1] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_c_id, message) + .unwrap(); + check_added_monitors(&nodes[1], 1); + let b_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; + check_closed_event!(nodes[1], 1, b_reason, [node_c_id], 1_000_000); + check_closed_broadcast!(nodes[1], true); + + let bs_commit_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(bs_commit_tx.len(), 1); + + let selected_commit_tx = if on_counterparty_tx { + &cs_commit_tx[0] + } else { + &bs_commit_tx[0] + }; + + mine_transaction(&nodes[2], selected_commit_tx); + let mut cs_transactions = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + let c_replays_commitment = nodes[2].connect_style.borrow().updates_best_block_first(); + let cs_htlc_claims = if on_counterparty_tx { + handle_bump_htlc_event(&nodes[2], 1); + let mut cs_transactions = + nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(cs_transactions.len(), 1); + vec![cs_transactions.pop().unwrap()] + } else { + if c_replays_commitment { + assert_eq!(cs_transactions.len(), 2, "{cs_transactions:?}"); + vec![cs_transactions.pop().unwrap()] + } else { + assert_eq!(cs_transactions.len(), 1, "{cs_transactions:?}"); + cs_transactions + } + }; + + assert_eq!(cs_htlc_claims.len(), 1); + check_spends!(cs_htlc_claims[0], selected_commit_tx, coinbase_tx); + + // Now replay the claims on node B, which should generate preimage-containing `MonitorUpdate`s + mine_transaction(&nodes[1], selected_commit_tx); + mine_transaction(&nodes[1], &cs_htlc_claims[0]); + + // Now simulate a restart where the B<->C ChannelMonitor has been persisted (i.e. because we + // just processed a new block) but the ChannelManager was not. This should be exceedingly rare + // given we have to be connecting a block at the right moment and not manage to get a + // ChannelManager persisted after it does a thing that should immediately precede persistence, + // but with async persist it is more common. + // + // We do this by wiping the `MonitorEvent`s from the monitors and then reloading with the + // latest state. + let mon_events = nodes[1].chain_monitor.chain_monitor.release_pending_monitor_events(); + assert_eq!(mon_events.len(), 1); + assert_eq!(mon_events[0].2.len(), 2); + + let node_ser = nodes[1].node.encode(); + let mon_a_ser = get_monitor!(nodes[1], chan_a).encode(); + let mon_b_ser = get_monitor!(nodes[1], chan_b).encode(); + let mons = &[&mon_a_ser[..], &mon_b_ser[..]]; + reload_node!(nodes[1], cfg, &node_ser, mons, persister, new_chain_mon, node_b_reload); + + check_added_monitors(&nodes[1], 0); + let preimage_events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(preimage_events.len(), 3, "{preimage_events:?}"); + for ev in preimage_events { + match ev { + Event::PaymentSent { payment_hash, .. } => { + assert_eq!(payment_hash, hash_b); + }, + Event::PaymentPathSuccessful { payment_hash, .. } => { + assert_eq!(payment_hash, Some(hash_b)); + }, + Event::PaymentForwarded { claim_from_onchain_tx, .. } => { + assert!(claim_from_onchain_tx); + }, + _ => panic!("Wrong event {ev:?}"), + } + } + + // After the background events are processed in `get_and_clear_pending_events`, above, node B + // will create the requisite `ChannelMontiorUpdate` for claiming the forwarded payment back. + // The HTLC, however, is added to the holding cell for replay after the peer connects, below. + // It will also apply a `ChannelMonitorUpdate` to let the `ChannelMonitor` know that the + // payment can now be forgotten as the `PaymentSent` event was handled. + check_added_monitors(&nodes[1], 2); + + nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); + + let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); + reconnect_args.pending_cell_htlc_claims = (1, 0); + reconnect_nodes(reconnect_args); + expect_payment_sent(&nodes[0], preimage_a, None, true, true); +} + +#[test] +fn test_lost_preimage_monitor_events() { + do_test_lost_preimage_monitor_events(true); + do_test_lost_preimage_monitor_events(false); +} + +#[derive(PartialEq)] +enum CommitmentType { + RevokedCounterparty, + LatestCounterparty, + PreviousCounterparty, + LocalWithoutLastHTLC, + LocalWithLastHTLC, +} + +fn do_test_lost_timeout_monitor_events(confirm_tx: CommitmentType, dust_htlcs: bool) { + // `MonitorEvent`s aren't delivered to the `ChannelManager` in a durable fashion - if the + // `ChannelManager` fetches the pending `MonitorEvent`s, then the `ChannelMonitor` gets + // persisted (i.e. due to a block update) then the node crashes, prior to persisting the + // `ChannelManager` again, the `MonitorEvent` and its effects on the `ChannelManger` will be + // lost. This isn't likely in a sync persist environment, but in an async one this could be an + // issue. + // + // Note that this is only an issue for closed channels - `MonitorEvent`s only inform the + // `ChannelManager` that a channel is closed (which the `ChannelManager` will learn on startup + // or when it next tries to advance the channel state), that `ChannelMonitorUpdate` writes + // completed (which the `ChannelManager` will detect on startup), or that HTLCs resolved + // on-chain post closure. Of the three, only the last is problematic to lose prior to a reload. + // + // Here we test that losing `MonitorEvent`s that contain HTLC resolution via timeouts does not + // cause us to lose a `PaymentFailed` event. + let mut cfg = test_default_channel_config(); + cfg.manually_accept_inbound_channels = true; + cfg.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + let cfgs = [Some(cfg.clone()), Some(cfg.clone()), Some(cfg.clone())]; + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let persister; + let new_chain_mon; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &cfgs); + let node_b_reload; + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + provide_anchor_reserves(&nodes); + + let node_a_id = nodes[0].node.get_our_node_id(); + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0).2; + let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).2; + + // Ensure all nodes are at the same height + let node_max_height = + nodes.iter().map(|node| node.blocks.lock().unwrap().len()).max().unwrap() as u32; + connect_blocks(&nodes[0], node_max_height - nodes[0].best_block_info().1); + connect_blocks(&nodes[1], node_max_height - nodes[1].best_block_info().1); + connect_blocks(&nodes[2], node_max_height - nodes[2].best_block_info().1); + + send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 25_000_000); + + let cs_revoked_commit = get_local_commitment_txn!(nodes[2], chan_b); + assert_eq!(cs_revoked_commit.len(), 1); + + let amt = if dust_htlcs { 1_000 } else { 10_000_000 }; + let (_, hash_a, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], amt); + + let cs_previous_commit = get_local_commitment_txn!(nodes[2], chan_b); + assert_eq!(cs_previous_commit.len(), 1); + + let (route, hash_b, _, payment_secret_b) = + get_route_and_payment_hash!(nodes[1], nodes[2], amt); + let onion = RecipientOnionFields::secret_only(payment_secret_b); + nodes[1].node.send_payment_with_route(route, hash_b, onion, PaymentId(hash_b.0)).unwrap(); + check_added_monitors(&nodes[1], 1); + + let updates = get_htlc_update_msgs(&nodes[1], &node_c_id); + nodes[2].node.handle_update_add_htlc(node_b_id, &updates.update_add_htlcs[0]); + nodes[2].node.handle_commitment_signed(node_b_id, &updates.commitment_signed); + check_added_monitors(&nodes[2], 1); + + let (cs_raa, cs_cs) = get_revoke_commit_msgs!(nodes[2], node_b_id); + if confirm_tx == CommitmentType::LocalWithLastHTLC { + // Only deliver the last RAA + CS if we need to update the local commitment with the third + // HTLC. + nodes[1].node.handle_revoke_and_ack(node_c_id, &cs_raa); + check_added_monitors(&nodes[1], 1); + nodes[1].node.handle_commitment_signed(node_c_id, &cs_cs); + check_added_monitors(&nodes[1], 1); + + let _bs_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, node_c_id); + } + + nodes[1].node.peer_disconnected(nodes[2].node.get_our_node_id()); + nodes[2].node.peer_disconnected(nodes[1].node.get_our_node_id()); + + // Force-close the channel, confirming a commitment transaction then letting C claim the HTLCs. + let message = "Closed".to_owned(); + nodes[2] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_b_id, message) + .unwrap(); + check_added_monitors(&nodes[2], 1); + let c_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; + check_closed_event!(nodes[2], 1, c_reason, [node_b_id], 1_000_000); + check_closed_broadcast!(nodes[2], true); + + let cs_commit_tx = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(cs_commit_tx.len(), 1); + + let message = "Closed".to_owned(); + nodes[1] + .node + .force_close_broadcasting_latest_txn(&chan_b, &node_c_id, message) + .unwrap(); + check_added_monitors(&nodes[1], 1); + let b_reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; + check_closed_event!(nodes[1], 1, b_reason, [node_c_id], 1_000_000); + check_closed_broadcast!(nodes[1], true); + + let bs_commit_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(bs_commit_tx.len(), 1); + + let selected_commit_tx = match confirm_tx { + CommitmentType::RevokedCounterparty => &cs_revoked_commit[0], + CommitmentType::PreviousCounterparty => &cs_previous_commit[0], + CommitmentType::LatestCounterparty => &cs_commit_tx[0], + CommitmentType::LocalWithoutLastHTLC|CommitmentType::LocalWithLastHTLC => &bs_commit_tx[0], + }; + + mine_transaction(&nodes[1], selected_commit_tx); + // If the block gets connected first we may re-broadcast B's commitment transaction before + // seeing the C's confirm. In any case, if we confirmed the revoked counterparty commitment + // transaction, we want to go ahead and confirm the spend of it. + let bs_transactions = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + if confirm_tx == CommitmentType::RevokedCounterparty { + assert!(bs_transactions.len() == 1 || bs_transactions.len() == 2); + mine_transaction(&nodes[1], bs_transactions.last().unwrap()); + } else { + assert!(bs_transactions.len() == 1 || bs_transactions.len() == 0); + } + + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + let mut events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); + match confirm_tx { + CommitmentType::LocalWithoutLastHTLC|CommitmentType::LocalWithLastHTLC => { + assert_eq!(events.len(), 0, "{events:?}"); + }, + CommitmentType::PreviousCounterparty|CommitmentType::LatestCounterparty => { + assert_eq!(events.len(), 1, "{events:?}"); + match events[0] { + Event::SpendableOutputs { .. } => {}, + _ => panic!("Unexpected event {events:?}"), + } + }, + CommitmentType::RevokedCounterparty => { + assert_eq!(events.len(), 2, "{events:?}"); + for event in events { + match event { + Event::SpendableOutputs { .. } => {}, + _ => panic!("Unexpected event {event:?}"), + } + } + }, + } + + if confirm_tx != CommitmentType::RevokedCounterparty { + connect_blocks(&nodes[1], TEST_FINAL_CLTV - ANTI_REORG_DELAY + 1); + if confirm_tx == CommitmentType::LocalWithoutLastHTLC || confirm_tx == CommitmentType::LocalWithLastHTLC { + if !dust_htlcs { + handle_bump_htlc_event(&nodes[1], 1); + } + } + } + + let bs_htlc_timeouts = + nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + if dust_htlcs || confirm_tx == CommitmentType::RevokedCounterparty { + assert_eq!(bs_htlc_timeouts.len(), 0); + } else { + assert_eq!(bs_htlc_timeouts.len(), 1); + + // Now replay the timeouts on node B, which after 6 confirmations should fail the HTLCs via + // `MonitorUpdate`s + mine_transaction(&nodes[1], &bs_htlc_timeouts[0]); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + } + + // Now simulate a restart where the B<->C ChannelMonitor has been persisted (i.e. because we + // just processed a new block) but the ChannelManager was not. This should be exceedingly rare + // given we have to be connecting a block at the right moment and not manage to get a + // ChannelManager persisted after it does a thing that should immediately precede persistence, + // but with async persist it is more common. + // + // We do this by wiping the `MonitorEvent`s from the monitors and then reloading with the + // latest state. + let mon_events = nodes[1].chain_monitor.chain_monitor.release_pending_monitor_events(); + assert_eq!(mon_events.len(), 1); + assert_eq!(mon_events[0].2.len(), 2); + + let node_ser = nodes[1].node.encode(); + let mon_a_ser = get_monitor!(nodes[1], chan_a).encode(); + let mon_b_ser = get_monitor!(nodes[1], chan_b).encode(); + let mons = &[&mon_a_ser[..], &mon_b_ser[..]]; + reload_node!(nodes[1], cfg, &node_ser, mons, persister, new_chain_mon, node_b_reload); + + // After reload, once we process the `PaymentFailed` event, the sent HTLC will be marked + // handled so that we won't ever see the event again. + check_added_monitors(&nodes[1], 0); + let timeout_events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(timeout_events.len(), 4, "{timeout_events:?}"); + check_added_monitors(&nodes[1], 1); + for ev in timeout_events { + match ev { + Event::PendingHTLCsForwardable { .. } => {}, + Event::PaymentPathFailed { payment_hash, .. } => { + assert_eq!(payment_hash, hash_b); + }, + Event::PaymentFailed { payment_hash, .. } => { + assert_eq!(payment_hash, Some(hash_b)); + }, + Event::HTLCHandlingFailed { prev_channel_id, .. } => { + assert_eq!(prev_channel_id, chan_a); + }, + _ => panic!("Wrong event {ev:?}"), + } + } + + nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); + + reconnect_nodes(ReconnectArgs::new(&nodes[0], &nodes[1])); + + nodes[1].node.process_pending_htlc_forwards(); + check_added_monitors(&nodes[1], 1); + let bs_fail = get_htlc_update_msgs(&nodes[1], &node_a_id); + nodes[0].node.handle_update_fail_htlc(node_b_id, &bs_fail.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], bs_fail.commitment_signed, true, true); + expect_payment_failed!(nodes[0], hash_a, false); +} + +#[test] +fn test_lost_timeout_monitor_events() { + do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, false); + do_test_lost_timeout_monitor_events(CommitmentType::RevokedCounterparty, true); + do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, false); + do_test_lost_timeout_monitor_events(CommitmentType::PreviousCounterparty, true); + do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, false); + do_test_lost_timeout_monitor_events(CommitmentType::LatestCounterparty, true); + do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, false); + do_test_lost_timeout_monitor_events(CommitmentType::LocalWithoutLastHTLC, true); + do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false); + do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true); +} + +#[test] +fn test_ladder_preimage_htlc_claims() { + // Tests that when we learn of a preimage via a monitor update we only claim HTLCs with the + // corresponding payment hash. This test is a reproduction of a scenario that happened in + // production where the second HTLC claim also included the first HTLC (even though it was + // already claimed) resulting in an invalid claim transaction. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + + let (_, _, channel_id, _) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0); + + let (payment_preimage1, payment_hash1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + let (payment_preimage2, payment_hash2, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &node_id_1, "test".to_string()).unwrap(); + check_added_monitors(&nodes[0], 1); + check_closed_broadcast(&nodes[0], 1, true); + let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; + check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 1_000_000); + + let commitment_tx = { + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + txn.remove(0) + }; + mine_transaction(&nodes[0], &commitment_tx); + mine_transaction(&nodes[1], &commitment_tx); + + check_closed_broadcast(&nodes[1], 1, true); + check_added_monitors(&nodes[1], 1); + check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[node_id_0], 1_000_000); + + nodes[1].node.claim_funds(payment_preimage1); + expect_payment_claimed!(&nodes[1], payment_hash1, 1_000_000); + check_added_monitors(&nodes[1], 1); + + let (htlc1, htlc_claim_tx1) = { + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + let htlc_claim_tx = txn.remove(0); + assert_eq!(htlc_claim_tx.input.len(), 1); + check_spends!(htlc_claim_tx, commitment_tx); + (htlc_claim_tx.input[0].previous_output, htlc_claim_tx) + }; + mine_transaction(&nodes[0], &htlc_claim_tx1); + mine_transaction(&nodes[1], &htlc_claim_tx1); + + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + expect_payment_sent(&nodes[0], payment_preimage1, None, true, false); + check_added_monitors(&nodes[0], 1); + + nodes[1].node.claim_funds(payment_preimage2); + expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000); + check_added_monitors(&nodes[1], 1); + + let (htlc2, htlc_claim_tx2) = { + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1, "{:?}", txn.iter().map(|tx| tx.compute_txid()).collect::>()); + let htlc_claim_tx = txn.remove(0); + assert_eq!(htlc_claim_tx.input.len(), 1); + check_spends!(htlc_claim_tx, commitment_tx); + (htlc_claim_tx.input[0].previous_output, htlc_claim_tx) + }; + assert_ne!(htlc1, htlc2); + + mine_transaction(&nodes[0], &htlc_claim_tx2); + mine_transaction(&nodes[1], &htlc_claim_tx2); + + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + expect_payment_sent(&nodes[0], payment_preimage2, None, true, false); + check_added_monitors(&nodes[0], 1); +} diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 1719b47dab3..b3a8bd9ffee 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -17,7 +17,9 @@ use crate::blinded_path::{IntroductionNode, NodeIdLookUp}; use crate::events::{self, PaymentFailureReason}; use crate::types::payment::{PaymentHash, PaymentPreimage, PaymentSecret}; use crate::ln::channel_state::ChannelDetails; -use crate::ln::channelmanager::{EventCompletionAction, HTLCSource, PaymentId}; +use crate::ln::channelmanager::{ + EventCompletionAction, HTLCSource, PaymentCompleteUpdate, PaymentId, +}; use crate::types::features::Bolt12InvoiceFeatures; use crate::ln::onion_utils; use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason}; @@ -182,9 +184,13 @@ impl PendingOutboundPayment { params.insert_previously_failed_blinded_path(blinded_tail); } } - fn is_awaiting_invoice(&self) -> bool { + // Used for payments to BOLT 12 offers where we are either waiting for an invoice or have an + // invoice but have not locked in HTLCs for the payment yet. + fn is_pre_htlc_lock_in(&self) -> bool { match self { - PendingOutboundPayment::AwaitingInvoice { .. } => true, + PendingOutboundPayment::AwaitingInvoice { .. } + | PendingOutboundPayment::InvoiceReceived { .. } + | PendingOutboundPayment::StaticInvoiceReceived { .. } => true, _ => false, } } @@ -1128,7 +1134,7 @@ impl OutboundPayments { let mut outbounds = self.pending_outbound_payments.lock().unwrap(); outbounds.retain(|pmt_id, pmt| { let mut retain = true; - if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_awaiting_invoice() { + if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_pre_htlc_lock_in() { pmt.mark_abandoned(PaymentFailureReason::RetriesExhausted); if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = pmt { pending_events.lock().unwrap().push_back((events::Event::PaymentFailed { @@ -1147,7 +1153,7 @@ impl OutboundPayments { let outbounds = self.pending_outbound_payments.lock().unwrap(); outbounds.iter().any(|(_, pmt)| !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled() && - !pmt.is_awaiting_invoice()) + !pmt.is_pre_htlc_lock_in()) } fn find_initial_route( @@ -1927,7 +1933,7 @@ impl OutboundPayments { pub(super) fn claim_htlc( &self, payment_id: PaymentId, payment_preimage: PaymentPreimage, session_priv: SecretKey, - path: Path, from_onchain: bool, ev_completion_action: EventCompletionAction, + path: Path, from_onchain: bool, ev_completion_action: &mut Option, pending_events: &Mutex)>>, logger: &L, ) where L::Target: Logger { @@ -1945,7 +1951,7 @@ impl OutboundPayments { payment_preimage, payment_hash, fee_paid_msat, - }, Some(ev_completion_action.clone()))); + }, ev_completion_action.take())); payment.get_mut().mark_fulfilled(); } @@ -1954,15 +1960,13 @@ impl OutboundPayments { // This could potentially lead to removing a pending payment too early, // with a reorg of one block causing us to re-add the fulfilled payment on // restart. - // TODO: We should have a second monitor event that informs us of payments - // irrevocably fulfilled. if payment.get_mut().remove(&session_priv_bytes, Some(&path)) { let payment_hash = Some(PaymentHash(Sha256::hash(&payment_preimage.0).to_byte_array())); pending_events.push_back((events::Event::PaymentPathSuccessful { payment_id, payment_hash, path, - }, Some(ev_completion_action))); + }, ev_completion_action.take())); } } } else { @@ -2087,7 +2091,8 @@ impl OutboundPayments { &self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, path: &Path, session_priv: &SecretKey, payment_id: &PaymentId, probing_cookie_secret: [u8; 32], secp_ctx: &Secp256k1, - pending_events: &Mutex)>>, logger: &L, + pending_events: &Mutex)>>, + logger: &L, completion_action: &mut Option, ) -> bool where L::Target: Logger { #[cfg(test)] let DecodedOnionFailure { @@ -2210,8 +2215,15 @@ impl OutboundPayments { } }; let mut pending_events = pending_events.lock().unwrap(); - pending_events.push_back((path_failure, None)); - if let Some(ev) = full_failure_ev { pending_events.push_back((ev, None)); } + let completion_action = completion_action + .take() + .map(|act| EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(act)); + if let Some(ev) = full_failure_ev { + pending_events.push_back((path_failure, None)); + pending_events.push_back((ev, completion_action)); + } else { + pending_events.push_back((path_failure, completion_action)); + } pending_retry_ev } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 348cace949d..47c12c358eb 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -11,7 +11,7 @@ //! serialization ordering between ChannelManager/ChannelMonitors and ensuring we can still retry //! payments thereafter. -use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen}; +use crate::chain::{Confirm, Listen}; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS}; use crate::sign::EntropySource; use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose}; @@ -754,7 +754,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { assert_eq!(txn[0].compute_txid(), as_commitment_tx.compute_txid()); } mine_transaction(&nodes[0], &bs_htlc_claim_txn); - expect_payment_sent(&nodes[0], payment_preimage_1, None, true, false); + expect_payment_sent(&nodes[0], payment_preimage_1, None, true, true); connect_blocks(&nodes[0], TEST_FINAL_CLTV*4 + 20); let (first_htlc_timeout_tx, second_htlc_timeout_tx) = { let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); @@ -769,7 +769,8 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { confirm_transaction(&nodes[0], &first_htlc_timeout_tx); } nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); - expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new()); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); // Finally, retry the payment (which was reloaded from the ChannelMonitor when nodes[0] was // reloaded) via a route over the new channel, which work without issue and eventually be @@ -956,7 +957,8 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { // (which should also still work). connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new()); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); let chan_1_monitor_serialized = get_monitor!(nodes[0], chan_id_3).encode(); @@ -1016,7 +1018,10 @@ fn test_completed_payment_not_retryable_on_reload() { do_test_completed_payment_not_retryable_on_reload(false); } -fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bool, confirm_commitment_tx: bool, payment_timeout: bool) { +fn do_test_dup_htlc_onchain_doesnt_fail_on_reload( + persist_manager_post_event: bool, persist_monitor_after_events: bool, + confirm_commitment_tx: bool, payment_timeout: bool, +) { // When a Channel is closed, any outbound HTLCs which were relayed through it are simply // dropped. From there, the ChannelManager relies on the ChannelMonitor having a copy of the // relevant fail-/claim-back data and processes the HTLC fail/claim when the ChannelMonitor tells @@ -1086,16 +1091,15 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo connect_blocks(&nodes[0], ANTI_REORG_DELAY - 2); } - // Now connect the HTLC claim transaction with the ChainMonitor-generated ChannelMonitor update - // returning InProgress. This should cause the claim event to never make its way to the - // ChannelManager. - chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); - + // Now connect the HTLC claim transaction. Note that ChannelMonitors aren't re-persisted on + // each block connection (as the block being reconnected on startup should get us the same + // result). if payment_timeout { connect_blocks(&nodes[0], 1); } else { connect_block(&nodes[0], &claim_block); } + check_added_monitors(&nodes[0], 0); // Note that we skip persisting ChannelMonitors. We should still be generating the payment sent // event without ChannelMonitor persistence. If we reset to a previous state on reload, the block @@ -1108,28 +1112,65 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo chan_manager_serialized = nodes[0].node.encode(); } - let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); + let mut mon_ser = Vec::new(); + if !persist_monitor_after_events { + mon_ser = get_monitor!(nodes[0], chan_id).encode(); + } if payment_timeout { - expect_payment_failed!(nodes[0], payment_hash, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + expect_payment_sent(&nodes[0], payment_preimage, None, true, true); + } + // Note that if we persist the monitor before processing the events, above, we'll always get + // them replayed on restart no matter what + if persist_monitor_after_events { + mon_ser = get_monitor!(nodes[0], chan_id).encode(); } // If we persist the ChannelManager after we get the PaymentSent event, we shouldn't get it // twice. if persist_manager_post_event { chan_manager_serialized = nodes[0].node.encode(); + } else if persist_monitor_after_events { + // Persisting the monitor after the events (resulting in a new monitor being persisted) but + // didn't persist the manager will result in an FC, which we don't test here. + panic!(); } // Now reload nodes[0]... - reload_node!(nodes[0], &chan_manager_serialized, &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized); + reload_node!(nodes[0], &chan_manager_serialized, &[&mon_ser], persister, new_chain_monitor, nodes_0_deserialized); - if persist_manager_post_event { + check_added_monitors(&nodes[0], 0); + if persist_manager_post_event && persist_monitor_after_events { assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + check_added_monitors(&nodes[0], 0); } else if payment_timeout { - expect_payment_failed!(nodes[0], payment_hash, false); + let mut conditions = PaymentFailedConditions::new(); + if !persist_monitor_after_events { + conditions = conditions.from_mon_update(); + } + expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions); + check_added_monitors(&nodes[0], 0); } else { - expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + if persist_manager_post_event { + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + } else { + expect_payment_sent(&nodes[0], payment_preimage, None, true, false); + } + if persist_manager_post_event { + // After reload, the ChannelManager identified the failed payment and queued up the + // PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we + // already did that) and corresponding ChannelMonitorUpdate to mark the payment + // handled, but while processing the pending `MonitorEvent`s (which were not processed + // before the monitor was persisted) we will end up with a duplicate + // ChannelMonitorUpdate. + check_added_monitors(&nodes[0], 2); + } else { + // ...unless we got the PaymentSent event, in which case we have de-duplication logic + // preventing a redundant monitor event. + check_added_monitors(&nodes[0], 1); + } } // Note that if we re-connect the block which exposed nodes[0] to the payment preimage (but @@ -1142,12 +1183,15 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo #[test] fn test_dup_htlc_onchain_doesnt_fail_on_reload() { - do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, true, true); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, true, false); - do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, true); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, true); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, true); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, false); + do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false, false); } #[test] @@ -1374,7 +1418,9 @@ fn onchain_failed_probe_yields_event() { check_closed_broadcast!(&nodes[0], true); check_added_monitors!(nodes[0], 1); + check_added_monitors(&nodes[0], 0); let mut events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 1); assert_eq!(events.len(), 2); let mut found_probe_failed = false; for event in events.drain(..) { @@ -3468,10 +3514,28 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); reload_node!(nodes[0], test_default_channel_config(), &nodes_0_serialized, &[&chan_0_monitor_serialized], persister_a, chain_monitor_a, nodes_0_deserialized); + // When we first process background events, we'll apply a channel-closed monitor update... + check_added_monitors(&nodes[0], 0); + nodes[0].node.test_process_background_events(); + check_added_monitors(&nodes[0], 1); + // Then once we process the PaymentSent event we'll apply a monitor update to remove the + // pending payment from being re-hydrated on the next startup. let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 2); - if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[0] {} else { panic!(); } - if let Event::PaymentSent { payment_preimage, .. } = events[1] { assert_eq!(payment_preimage, our_payment_preimage); } else { panic!(); } + check_added_monitors(&nodes[0], 1); + assert_eq!(events.len(), 3, "{events:?}"); + if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[0] { + } else { + panic!(); + } + if let Event::PaymentSent { payment_preimage, .. } = events[1] { + assert_eq!(payment_preimage, our_payment_preimage); + } else { + panic!(); + } + if let Event::PaymentPathSuccessful { .. } = events[2] { + } else { + panic!(); + } // Note that we don't get a PaymentPathSuccessful here as we leave the HTLC pending to avoid // the double-claim that would otherwise appear at the end of this test. nodes[0].node.timer_tick_occurred(); @@ -3487,6 +3551,8 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_b, chain_monitor_b, nodes_0_deserialized_b); + + nodes[0].node.test_process_background_events(); let events = nodes[0].node.get_and_clear_pending_events(); assert!(events.is_empty()); @@ -3497,6 +3563,7 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: let events = nodes[0].node.get_and_clear_pending_events(); assert!(events.is_empty()); + check_added_monitors(&nodes[0], 0); let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_c, chain_monitor_c, nodes_0_deserialized_c); diff --git a/lightning/src/ln/peer_channel_encryptor.rs b/lightning/src/ln/peer_channel_encryptor.rs index 00e45afb4d8..6da697a02a1 100644 --- a/lightning/src/ln/peer_channel_encryptor.rs +++ b/lightning/src/ln/peer_channel_encryptor.rs @@ -564,6 +564,11 @@ impl MessageBuf { res[16 + 2..].copy_from_slice(&encoded_msg); Self(res) } + + #[cfg(test)] + pub(crate) fn fetch_encoded_msg_with_type_pfx(&self) -> Vec { + self.0.clone().split_off(16 + 2) + } } #[cfg(test)] diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 0cb2b050c14..2e4f06ab76d 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -632,7 +632,7 @@ impl Peer { } match self.sync_status { InitSyncTracker::NoSyncRequested => true, - InitSyncTracker::ChannelsSyncing(i) => i < channel_id, + InitSyncTracker::ChannelsSyncing(i) => channel_id < i, InitSyncTracker::NodesSyncing(_) => true, } } @@ -2102,6 +2102,8 @@ impl 0 { // If we're not the first event processor to get here, just return early, the increment @@ -3294,6 +3296,80 @@ mod tests { assert_eq!(cfgs[1].routing_handler.chan_anns_recvd.load(Ordering::Acquire), 54); } + #[test] + fn test_forward_while_syncing() { + use crate::ln::peer_handler::tests::test_utils::get_dummy_channel_update; + + // Test forwarding new channel announcements while we're doing syncing. + let cfgs = create_peermgr_cfgs(2); + cfgs[0].routing_handler.request_full_sync.store(true, Ordering::Release); + cfgs[1].routing_handler.request_full_sync.store(true, Ordering::Release); + cfgs[0].routing_handler.announcement_available_for_sync.store(true, Ordering::Release); + cfgs[1].routing_handler.announcement_available_for_sync.store(true, Ordering::Release); + let peers = create_network(2, &cfgs); + + let (mut fd_a, mut fd_b) = establish_connection(&peers[0], &peers[1]); + + // Iterate a handful of times to exchange some messages + for _ in 0..150 { + peers[1].process_events(); + let a_read_data = fd_b.outbound_data.lock().unwrap().split_off(0); + assert!(!a_read_data.is_empty()); + + peers[0].read_event(&mut fd_a, &a_read_data).unwrap(); + peers[0].process_events(); + + let b_read_data = fd_a.outbound_data.lock().unwrap().split_off(0); + assert!(!b_read_data.is_empty()); + peers[1].read_event(&mut fd_b, &b_read_data).unwrap(); + + peers[0].process_events(); + assert_eq!( + fd_a.outbound_data.lock().unwrap().len(), + 0, + "Until A receives data, it shouldn't send more messages" + ); + } + + // Forward one more gossip backfill message but don't flush it so that we can examine the + // unencrypted message for broadcasts. + fd_b.hang_writes.store(true, Ordering::Relaxed); + peers[1].process_events(); + + { + let peer_lock = peers[1].peers.read().unwrap(); + let peer = peer_lock.get(&fd_b).unwrap().lock().unwrap(); + assert_eq!(peer.pending_outbound_buffer.len(), 1); + assert_eq!(peer.gossip_broadcast_buffer.len(), 0); + } + + // At this point we should have sent channel announcements up to roughly SCID 150. Now + // build an updated update for SCID 100 and SCID 5000 and make sure only the one for SCID + // 100 gets forwarded + let msg_100 = get_dummy_channel_update(100); + let msg_ev_100 = MessageSendEvent::BroadcastChannelUpdate { msg: msg_100.clone() }; + + let msg_5000 = get_dummy_channel_update(5000); + let msg_ev_5000 = MessageSendEvent::BroadcastChannelUpdate { msg: msg_5000 }; + + fd_a.hang_writes.store(true, Ordering::Relaxed); + + cfgs[1].routing_handler.pending_events.lock().unwrap().push(msg_ev_100); + cfgs[1].routing_handler.pending_events.lock().unwrap().push(msg_ev_5000); + peers[1].process_events(); + + { + let peer_lock = peers[1].peers.read().unwrap(); + let peer = peer_lock.get(&fd_b).unwrap().lock().unwrap(); + assert_eq!(peer.pending_outbound_buffer.len(), 1); + assert_eq!(peer.gossip_broadcast_buffer.len(), 1); + + let pending_msg = &peer.gossip_broadcast_buffer[0]; + let expected = encode_msg!(&msg_100); + assert_eq!(expected, pending_msg.fetch_encoded_msg_with_type_pfx()); + } + } + #[test] fn test_handshake_timeout() { // Tests that we time out a peer still waiting on handshake completion after a full timer diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 56760c510a3..03e621d3e37 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -232,12 +232,13 @@ fn test_counterparty_revoked_reorg() { // Connect the HTLC claim transaction for HTLC 3 mine_transaction(&nodes[1], &unrevoked_local_txn[2]); - expect_payment_sent(&nodes[1], payment_preimage_3, None, true, false); + expect_payment_sent(&nodes[1], payment_preimage_3, None, true, true); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); // Connect blocks to confirm the unrevoked commitment transaction connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2); - expect_payment_failed!(nodes[1], payment_hash_4, false); + let conditions = PaymentFailedConditions::new().from_mon_update(); + expect_payment_failed_conditions(&nodes[1], payment_hash_4, false, conditions) } fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_unconfirmed: bool, connect_style: ConnectStyle) { @@ -1001,7 +1002,9 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) { let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); assert_eq!(txn.len(), 0); + check_added_monitors(&nodes[0], 0); let sent_events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 2); assert_eq!(sent_events.len(), 4, "{sent_events:?}"); let mut found_expected_events = [false, false, false, false]; for event in sent_events { @@ -1090,7 +1093,9 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) { // Connect two more blocks to get `as_third_htlc_spend_tx` to `ANTI_REORG_DELAY` confs. connect_blocks(&nodes[0], 2); if use_third_htlc { + check_added_monitors(&nodes[0], 0); let failed_events = nodes[0].node.get_and_clear_pending_events(); + check_added_monitors(&nodes[0], 1); assert_eq!(failed_events.len(), 2); let mut found_expected_events = [false, false]; for event in failed_events { diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 1f89d25022c..3671b1eda74 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -36,7 +36,9 @@ use crate::ln::msgs::{ use crate::ln::types::ChannelId; use crate::routing::utxo::{self, UtxoLookup, UtxoResolver}; use crate::types::features::{ChannelFeatures, InitFeatures, NodeFeatures}; -use crate::util::indexed_map::{Entry as IndexedMapEntry, IndexedMap}; +use crate::util::indexed_map::{ + Entry as IndexedMapEntry, IndexedMap, OccupiedEntry as IndexedMapOccupiedEntry, +}; use crate::util::logger::{Level, Logger}; use crate::util::scid_utils::{block_from_scid, scid_from_parts, MAX_SCID_BLOCK}; use crate::util::ser::{MaybeReadable, Readable, ReadableArgs, RequiredWrapper, Writeable, Writer}; @@ -2355,9 +2357,7 @@ where return; } let min_time_unix: u32 = (current_time_unix - STALE_CHANNEL_UPDATE_AGE_LIMIT_SECS) as u32; - // Sadly BTreeMap::retain was only stabilized in 1.53 so we can't switch to it for some - // time. - let mut scids_to_remove = Vec::new(); + let mut scids_to_remove = new_hash_set(); for (scid, info) in channels.unordered_iter_mut() { if info.one_to_two.is_some() && info.one_to_two.as_ref().unwrap().last_update < min_time_unix @@ -2381,19 +2381,24 @@ where if announcement_received_timestamp < min_time_unix as u64 { log_gossip!(self.logger, "Removing channel {} because both directional updates are missing and its announcement timestamp {} being below {}", scid, announcement_received_timestamp, min_time_unix); - scids_to_remove.push(*scid); + scids_to_remove.insert(*scid); } } } if !scids_to_remove.is_empty() { let mut nodes = self.nodes.write().unwrap(); - for scid in scids_to_remove { - let info = channels - .remove(&scid) - .expect("We just accessed this scid, it should be present"); - self.remove_channel_in_nodes(&mut nodes, &info, scid); - self.removed_channels.lock().unwrap().insert(scid, Some(current_time_unix)); + let mut removed_channels_lck = self.removed_channels.lock().unwrap(); + + let channels_removed_bulk = channels.remove_fetch_bulk(&scids_to_remove); + self.removed_node_counters.lock().unwrap().reserve(channels_removed_bulk.len()); + let mut nodes_to_remove = hash_set_with_capacity(channels_removed_bulk.len()); + for (scid, info) in channels_removed_bulk { + self.remove_channel_in_nodes_callback(&mut nodes, &info, scid, |e| { + nodes_to_remove.insert(*e.key()); + }); + removed_channels_lck.insert(scid, Some(current_time_unix)); } + nodes.remove_bulk(&nodes_to_remove); } let should_keep_tracking = |time: &mut Option| { @@ -2632,8 +2637,9 @@ where Ok(()) } - fn remove_channel_in_nodes( + fn remove_channel_in_nodes_callback)>( &self, nodes: &mut IndexedMap, chan: &ChannelInfo, short_channel_id: u64, + mut remove_node: RM, ) { macro_rules! remove_from_node { ($node_id: expr) => { @@ -2641,7 +2647,7 @@ where entry.get_mut().channels.retain(|chan_id| short_channel_id != *chan_id); if entry.get().channels.is_empty() { self.removed_node_counters.lock().unwrap().push(entry.get().node_counter); - entry.remove_entry(); + remove_node(entry); } } else { panic!( @@ -2654,6 +2660,14 @@ where remove_from_node!(chan.node_one); remove_from_node!(chan.node_two); } + + fn remove_channel_in_nodes( + &self, nodes: &mut IndexedMap, chan: &ChannelInfo, short_channel_id: u64, + ) { + self.remove_channel_in_nodes_callback(nodes, chan, short_channel_id, |e| { + e.remove_entry(); + }); + } } impl ReadOnlyNetworkGraph<'_> { diff --git a/lightning/src/util/indexed_map.rs b/lightning/src/util/indexed_map.rs index 34860e3d68a..b97f9dfc119 100644 --- a/lightning/src/util/indexed_map.rs +++ b/lightning/src/util/indexed_map.rs @@ -72,6 +72,26 @@ impl IndexedMap { ret } + /// Removes elements with the given `keys` in bulk, returning the set of removed elements. + pub fn remove_fetch_bulk(&mut self, keys: &HashSet) -> Vec<(K, V)> { + let mut res = Vec::with_capacity(keys.len()); + for key in keys.iter() { + if let Some((k, v)) = self.map.remove_entry(key) { + res.push((k, v)); + } + } + self.keys.retain(|k| !keys.contains(k)); + res + } + + /// Removes elements with the given `keys` in bulk. + pub fn remove_bulk(&mut self, keys: &HashSet) { + for key in keys.iter() { + self.map.remove(key); + } + self.keys.retain(|k| !keys.contains(k)); + } + /// Inserts the given `key`/`value` pair into the map, returning the element that was /// previously stored at the given `key`, if one exists. pub fn insert(&mut self, key: K, value: V) -> Option { @@ -210,6 +230,11 @@ impl<'a, K: Hash + Ord, V> OccupiedEntry<'a, K, V> { res } + /// Get a reference to the key at the position described by this entry. + pub fn key(&self) -> &K { + self.underlying_entry.key() + } + /// Get a reference to the value at the position described by this entry. pub fn get(&self) -> &V { self.underlying_entry.get() diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 281789067ea..3ee80c17413 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -1566,7 +1566,14 @@ impl Readable for Duration { fn read(r: &mut R) -> Result { let secs = Readable::read(r)?; let nanos = Readable::read(r)?; - Ok(Duration::new(secs, nanos)) + // Duration::new panics if the nanosecond part in excess of a second, added to the second + // part, overflows. To ensure this won't happen, we simply reject any case where there are + // nanoseconds in excess of a second, which is invalid anyway. + if nanos >= 1_000_000_000 { + Err(DecodeError::InvalidValue) + } else { + Ok(Duration::new(secs, nanos)) + } } } diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index d3ae9261e12..4a38f06465e 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -991,7 +991,7 @@ fn get_dummy_channel_announcement(short_chan_id: u64) -> msgs::ChannelAnnounceme } } -fn get_dummy_channel_update(short_chan_id: u64) -> msgs::ChannelUpdate { +pub fn get_dummy_channel_update(short_chan_id: u64) -> msgs::ChannelUpdate { use bitcoin::secp256k1::ffi::Signature as FFISignature; let network = Network::Testnet; msgs::ChannelUpdate {