diff --git a/.github/workflows/check_unicode.yml b/.github/workflows/check_unicode.yml new file mode 100644 index 00000000000..a01add3f814 --- /dev/null +++ b/.github/workflows/check_unicode.yml @@ -0,0 +1,26 @@ +name: Unicode listing up to date +on: + workflow_dispatch: + schedule: + - cron: '42 3 * * *' + +jobs: + check-unicode: + runs-on: ubuntu-latest + permissions: + issues: write + steps: + - name: Checkout source code + uses: actions/checkout@v4 + - name: Check unicode file state + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + curl --proto '=https' --tlsv1.2 -fsSL -o /tmp/UnicodeData.txt https://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt + contrib/gen_unicode_general_category.py /tmp/UnicodeData.txt -o /tmp/unicode.rs + if ! diff -u lightning-types/src/unicode.rs /tmp/unicode.rs; then + TITLE="Unicode listing out of date: ${{ github.workflow }}" + RUN_URL="https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}" + BODY="The unicode character listing is out of date, see $RUN_URL" + gh issue create --title "$TITLE" --body "$BODY" + fi diff --git a/contrib/gen_unicode_general_category.py b/contrib/gen_unicode_general_category.py new file mode 100755 index 00000000000..4871e967b55 --- /dev/null +++ b/contrib/gen_unicode_general_category.py @@ -0,0 +1,308 @@ +#!/usr/bin/env python3 +# This file is Copyright its original authors, visible in version control +# history. +# +# This file is licensed under the Apache License, Version 2.0 or the MIT license +# , at your option. +# You may not use this file except in accordance with one or both of these +# licenses. + +"""Generate Unicode general-category predicates from `UnicodeData.txt`. + +Emits two `pub(crate)` functions taking a `char`, split into two disjoint +buckets across the Unicode top-level `C` ("Other") category so callers can +compose them: + + is_unicode_general_category_other — Cc / Cf / Cs / Co (assigned) + is_unicode_general_category_unassigned — Cn (plus codepoints above + U+10FFFF, which aren't + valid codepoints at all) + +`UnicodeData.txt` is the canonical machine-readable listing of every assigned +codepoint in the Unicode Character Database. Each line is `;`-separated; field +0 is the codepoint (hex), field 1 is the name, and field 2 is the two-letter +general category (e.g. `Lu`, `Cf`, `Mn`). Codepoints absent from the file have +category `Cn` (Unassigned) by convention. + +Two encoding details to preserve: + * Large blocks of contiguous same-category codepoints are written as two + consecutive entries whose names end in `, First>` and `, Last>`. Every + codepoint between First and Last (inclusive) shares the listed category. + * The codepoint range is U+0000..=U+10FFFF. + +Each `matches!` arm in the assigned-Other table carries an end-of-line comment +derived from the `UnicodeData.txt` name field — typically the longest common +word prefix or suffix across the names in the range, falling back to the set +of categories when the names share nothing meaningful. The unassigned table +omits per-arm comments since every range there has the same meaning by +construction. + +Usage: + contrib/gen_unicode_general_category.py UnicodeData.txt > out.rs +""" + +import argparse +import sys +from pathlib import Path + +MAX_CODEPOINT = 0x10FFFF + +LICENSE_HEADER = """\ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. +""" + +GENERATED_NOTICE = """\ +// Auto-generated from the Unicode Character Database (UnicodeData.txt) by +// contrib/gen_unicode_general_category.py. Do not edit by hand; rerun the +// generator with an updated UnicodeData.txt to refresh the table. +""" + + +def _normalize_name(name): + """Strip the `<...>` wrapping and `, First` / `, Last` range markers so + that, e.g., `` becomes + `Non Private Use High Surrogate` and `` becomes `control`. + """ + if name.startswith("<") and name.endswith(">"): + inner = name[1:-1] + for suffix in (", First", ", Last"): + if inner.endswith(suffix): + inner = inner[: -len(suffix)] + return inner + return name + + +def parse_categories(path): + """Return `(cats, names)` mapping every codepoint listed in `path` to its + general category and to its (normalised) name. Codepoints absent from the + returned dicts have category `Cn` (Unassigned) and no name. + """ + cats = {} + names = {} + pending_first = None # (first_cp, first_cat, normalised_name) once a range opens. + with path.open() as f: + for lineno, raw in enumerate(f, 1): + line = raw.rstrip("\n") + if not line: + continue + fields = line.split(";") + if len(fields) < 3: + raise ValueError(f"{path}:{lineno}: expected at least 3 fields, got {len(fields)}") + cp = int(fields[0], 16) + name = fields[1] + cat = fields[2] + if pending_first is not None: + first_cp, first_cat, first_name = pending_first + if not name.endswith(", Last>"): + raise ValueError( + f"{path}:{lineno}: expected `, Last>` to close range " + f"opened at U+{first_cp:04X}, got name {name!r}" + ) + if cat != first_cat: + raise ValueError( + f"{path}:{lineno}: range U+{first_cp:04X}..=U+{cp:04X} " + f"has mismatched categories {first_cat!r} / {cat!r}" + ) + for x in range(first_cp, cp + 1): + cats[x] = cat + names[x] = first_name + pending_first = None + elif name.endswith(", First>"): + pending_first = (cp, cat, _normalize_name(name)) + else: + cats[cp] = cat + names[cp] = _normalize_name(name) + if pending_first is not None: + raise ValueError(f"{path}: dangling `, First>` entry at U+{pending_first[0]:04X}") + return cats, names + + +ASSIGNED_OTHER_CATS = frozenset({"Cc", "Cf", "Cs", "Co"}) + + +def coalesce_ranges(cats, names, target_cats, *, label): + """Walk U+0000..=U+10FFFF and return a list of `(start, end, label)` for + every contiguous run of codepoints whose general category is in + `target_cats`. Codepoints absent from `cats` are treated as `Cn`. + + If `label` is `True`, attach a comment summarising the codepoint names in + each range; otherwise every range gets an empty label. + """ + ranges = [] + start = None + for cp in range(MAX_CODEPOINT + 1): + in_target = cats.get(cp, "Cn") in target_cats + if in_target and start is None: + start = cp + elif not in_target and start is not None: + ranges.append((start, cp - 1)) + start = None + if start is not None: + ranges.append((start, MAX_CODEPOINT)) + + if not label: + return [(s, e, "") for s, e in ranges] + + labelled = [] + for s, e in ranges: + range_names = [] + range_cats = set() + for cp in range(s, e + 1): + range_cats.add(cats.get(cp, "Cn")) + n = names.get(cp) + if n is not None: + range_names.append(n) + labelled.append((s, e, _make_label(range_names, range_cats))) + return labelled + + +def _common_word_run(names, *, from_end): + """Return the longest sequence of words shared by every name, taken from + either the start (`from_end=False`) or the end (`from_end=True`) of each + name's whitespace-split tokens. + """ + if not names: + return "" + tokenised = [n.split() for n in names] + if from_end: + tokenised = [list(reversed(t)) for t in tokenised] + limit = min(len(t) for t in tokenised) + common = [] + for i in range(limit): + token = tokenised[0][i] + if all(t[i] == token for t in tokenised): + common.append(token) + else: + break + if from_end: + common.reverse() + return " ".join(common) + + +def _make_label(names, cats_in_range): + """Build a short human-readable label for a coalesced range. Applied to + the assigned-Other buckets only; each range there is `Cc`, `Cf`, `Cs`, + `Co`, or some contiguous union thereof. + + Rules, in order: + 1. All names identical → that name (e.g. `control`). + 2. Common leading or trailing words → the longer of the two. + 3. Otherwise, list the categories present (e.g. `Co / Cs`). + """ + unique = list(dict.fromkeys(names)) + if len(unique) == 1: + return unique[0] + + prefix = _common_word_run(names, from_end=False) + suffix = _common_word_run(names, from_end=True) + # Pick whichever is more informative; when both are non-empty, prefer the + # longer one. A multi-word prefix beats a single-word suffix. + label = prefix if len(prefix) >= len(suffix) else suffix + if label: + return label + return " / ".join(sorted(cats_in_range)) + + +def fmt_codepoint(cp): + # `UnicodeData.txt` uses 4-digit hex for the BMP and wider for higher + # planes; mirror that so the output stays readable next to the source data. + return f"0x{cp:04X}" if cp <= 0xFFFF else f"0x{cp:X}" + + +def _pattern(start, end): + if start == end: + return fmt_codepoint(start) + return f"{fmt_codepoint(start)}..={fmt_codepoint(end)}" + + +def _emit_matches_body(lines, arms): + """Append a `matches!(c as u32, ...)` body to `lines`, with one + `(pattern, label)` tuple per arm. The first arm sits at the `matches!` + argument indent and continuation `| ...` arms indent one level deeper, + matching the rustfmt convention used elsewhere in the tree. + """ + lines.append("\tmatches!(") + lines.append("\t\tc as u32,") + for i, (pattern, label) in enumerate(arms): + prefix = "\t\t" if i == 0 else "\t\t\t| " + comment = f" // {label}" if label else "" + lines.append(f"{prefix}{pattern}{comment}") + lines.append("\t)") + + +def render_rust(other_ranges, unassigned_ranges): + """Render the final Rust source defining both `char`-taking predicates. + + `other_ranges` and `unassigned_ranges` are lists of `(start, end, label)`. + The unassigned function additionally gets a synthetic final arm catching + `u32` values above U+10FFFF — these aren't valid Unicode codepoints, so + by definition they have no general category and the unassigned bucket is + the closest match. + """ + lines = [LICENSE_HEADER, GENERATED_NOTICE] + + lines.append("/// Returns `true` if `c` is in Unicode general category `Cc` (Control), `Cf`") + lines.append("/// (Format), `Cs` (Surrogate), or `Co` (Private Use) — the assigned codepoints") + lines.append("/// in the top-level `C` (\"Other\") category. The `Cs` portion of the table is") + lines.append("/// unreachable for `char` input (a `char` cannot hold a surrogate) but is kept") + lines.append("/// so the table mirrors the source UCD data verbatim. The disjoint `Cn`") + lines.append("/// (Unassigned) bucket is `is_unicode_general_category_unassigned`.") + lines.append("#[allow(dead_code)]") + lines.append("pub(crate) fn is_unicode_general_category_other(c: char) -> bool {") + other_arms = [(_pattern(s, e), label) for s, e, label in other_ranges] + _emit_matches_body(lines, other_arms) + lines.append("}") + lines.append("") + + lines.append("/// Returns `true` if `c` is in Unicode general category `Cn` (Unassigned), or") + lines.append("/// strictly above U+10FFFF. The trailing `0x110000..=u32::MAX` arm is") + lines.append("/// unreachable for `char` input (a `char` is bounded to U+10FFFF) but is kept") + lines.append("/// for defensive coverage of the underlying `u32`. The disjoint Cc / Cf / Cs /") + lines.append("/// Co bucket is `is_unicode_general_category_other`.") + lines.append("#[allow(dead_code)]") + lines.append("pub(crate) fn is_unicode_general_category_unassigned(c: char) -> bool {") + unassigned_arms = [(_pattern(s, e), label) for s, e, label in unassigned_ranges] + unassigned_arms.append(("0x110000..=u32::MAX", "above U+10FFFF — unreachable for `char`")) + _emit_matches_body(lines, unassigned_arms) + lines.append("}") + lines.append("") + + return "\n".join(lines) + + +def main(argv): + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("unicode_data", type=Path, help="Path to UnicodeData.txt") + ap.add_argument( + "-o", "--output", type=Path, default=None, + help="Output Rust file (default: stdout)", + ) + args = ap.parse_args(argv) + + cats, names = parse_categories(args.unicode_data) + other = coalesce_ranges(cats, names, ASSIGNED_OTHER_CATS, label=True) + unassigned = coalesce_ranges(cats, names, frozenset({"Cn"}), label=False) + rust = render_rust(other, unassigned) + + if args.output is None: + sys.stdout.write(rust) + else: + args.output.write_text(rust) + print( + f"Wrote {args.output} " + f"({len(other)} assigned-Other ranges, " + f"{len(unassigned)} unassigned ranges).", + file=sys.stderr, + ) + + +if __name__ == "__main__": + main(sys.argv[1:]) diff --git a/lightning-custom-message/src/lib.rs b/lightning-custom-message/src/lib.rs index fb0ba191cd3..73405109327 100644 --- a/lightning-custom-message/src/lib.rs +++ b/lightning-custom-message/src/lib.rs @@ -312,13 +312,25 @@ macro_rules! composite_custom_message_handler { } fn peer_connected(&self, their_node_id: $crate::bitcoin::secp256k1::PublicKey, msg: &$crate::lightning::ln::msgs::Init, inbound: bool) -> Result<(), ()> { - let mut result = Ok(()); + // Per the `CustomMessageHandler::peer_connected` contract, `peer_disconnected` + // will not be called by `PeerManager` if we return `Err`. To avoid leaking + // per-peer state in sub-handlers that already returned `Ok` when a later one + // errors, record each sub-handler's result and roll back the successful ones + // ourselves before propagating the failure. $( - if let Err(e) = self.$field.peer_connected(their_node_id, msg, inbound) { - result = Err(e); - } + let $field = self.$field.peer_connected(their_node_id, msg, inbound); )* - result + let any_err = false $( || $field.is_err() )*; + if any_err { + $( + if $field.is_ok() { + self.$field.peer_disconnected(their_node_id); + } + )* + Err(()) + } else { + Ok(()) + } } fn provided_node_features(&self) -> $crate::lightning::types::features::NodeFeatures { @@ -376,3 +388,144 @@ macro_rules! composite_custom_message_handler { } } } + +#[cfg(test)] +mod tests { + use bitcoin::io::Read; + use bitcoin::secp256k1::PublicKey; + use core::sync::atomic::{AtomicUsize, Ordering}; + use lightning::io; + use lightning::ln::msgs::{DecodeError, Init, LightningError}; + use lightning::ln::peer_handler::CustomMessageHandler; + use lightning::ln::wire::{CustomMessageReader, Type}; + use lightning::types::features::{InitFeatures, NodeFeatures}; + use lightning::util::ser::{Writeable, Writer}; + + #[derive(Debug)] + pub struct Foo; + impl Type for Foo { + fn type_id(&self) -> u16 { + 32768 + } + } + impl Writeable for Foo { + fn write(&self, _: &mut W) -> Result<(), io::Error> { + Ok(()) + } + } + + pub struct CountingHandler { + pub connect_count: AtomicUsize, + } + impl CustomMessageReader for CountingHandler { + type CustomMessage = Foo; + fn read( + &self, _t: u16, _b: &mut R, + ) -> Result, DecodeError> { + Ok(None) + } + } + impl CustomMessageHandler for CountingHandler { + fn handle_custom_message(&self, _msg: Foo, _: PublicKey) -> Result<(), LightningError> { + Ok(()) + } + fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Foo)> { + vec![] + } + fn peer_disconnected(&self, _: PublicKey) { + self.connect_count.fetch_sub(1, Ordering::SeqCst); + } + fn peer_connected(&self, _: PublicKey, _: &Init, _: bool) -> Result<(), ()> { + self.connect_count.fetch_add(1, Ordering::SeqCst); + Ok(()) + } + fn provided_node_features(&self) -> NodeFeatures { + NodeFeatures::empty() + } + fn provided_init_features(&self, _: PublicKey) -> InitFeatures { + InitFeatures::empty() + } + } + + #[derive(Debug)] + pub struct Bar; + impl Type for Bar { + fn type_id(&self) -> u16 { + 32769 + } + } + impl Writeable for Bar { + fn write(&self, _: &mut W) -> Result<(), io::Error> { + Ok(()) + } + } + + pub struct ErroringHandler; + impl CustomMessageReader for ErroringHandler { + type CustomMessage = Bar; + fn read( + &self, _t: u16, _b: &mut R, + ) -> Result, DecodeError> { + Ok(None) + } + } + impl CustomMessageHandler for ErroringHandler { + fn handle_custom_message(&self, _msg: Bar, _: PublicKey) -> Result<(), LightningError> { + Ok(()) + } + fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Bar)> { + vec![] + } + fn peer_disconnected(&self, _: PublicKey) { + debug_assert!(false); + } + fn peer_connected(&self, _: PublicKey, _: &Init, _: bool) -> Result<(), ()> { + Err(()) + } + fn provided_node_features(&self) -> NodeFeatures { + NodeFeatures::empty() + } + fn provided_init_features(&self, _: PublicKey) -> InitFeatures { + InitFeatures::empty() + } + } + + composite_custom_message_handler!( + pub struct CompositeHandler { + counting: CountingHandler, + erroring: ErroringHandler, + } + + pub enum CompositeMessage { + Foo(32768), + Bar(32769), + } + ); + + #[test] + fn peer_connected_failure_does_not_leak_subhandler_state() { + let composite = CompositeHandler { + counting: CountingHandler { connect_count: AtomicUsize::new(0) }, + erroring: ErroringHandler, + }; + let pk_bytes = [ + 0x02, 0x79, 0xBE, 0x66, 0x7E, 0xF9, 0xDC, 0xBB, 0xAC, 0x55, 0xA0, 0x62, 0x95, 0xCE, + 0x87, 0x0B, 0x07, 0x02, 0x9B, 0xFC, 0xDB, 0x2D, 0xCE, 0x28, 0xD9, 0x59, 0xF2, 0x81, + 0x5B, 0x16, 0xF8, 0x17, 0x98, + ]; + let pk = PublicKey::from_slice(&pk_bytes).unwrap(); + let init = + Init { features: InitFeatures::empty(), networks: None, remote_network_address: None }; + + let result = composite.peer_connected(pk, &init, true); + assert!(result.is_err(), "Composite must propagate the inner Err"); + + let leaked = composite.counting.connect_count.load(Ordering::SeqCst); + assert_eq!( + leaked, 0, + "CountingHandler tracked {leaked} connected peer(s) after the composite \ + returned Err; this state will never be cleaned up because per the trait \ + contract peer_disconnected won't be called when peer_connected returns Err.", + ); + } +} diff --git a/lightning-dns-resolver/src/lib.rs b/lightning-dns-resolver/src/lib.rs index 8f855cb5fb7..03d27e64ec8 100644 --- a/lightning-dns-resolver/src/lib.rs +++ b/lightning-dns-resolver/src/lib.rs @@ -135,8 +135,8 @@ where let contents = DNSResolverMessage::DNSSECProof(DNSSECProof { name: q.0, proof }); let instructions = responder.respond().into_instructions(); us.pending_replies.lock().unwrap().push((contents, instructions)); - us.pending_query_count.fetch_sub(1, Ordering::Relaxed); } + us.pending_query_count.fetch_sub(1, Ordering::Relaxed); }); None } @@ -459,4 +459,92 @@ mod test { expect_payment_sent(&nodes[0], our_payment_preimage, None, true, true); } + + #[tokio::test] + async fn failed_query_does_not_leak_pending_counter() { + use std::sync::atomic::Ordering; + + let secp_ctx = Secp256k1::new(); + + // Resolver points at a port that should refuse TCP, so build_txt_proof_async + // returns Err quickly. + let resolver_keys = Arc::new(KeysManager::new(&[99; 32], 42, 43)); + let resolver_logger = TestLogger { node: "resolver" }; + let resolver = + Arc::new(OMDomainResolver::::ignoring_incoming_proofs( + "127.0.0.1:1".parse().unwrap(), + )); + let resolver_state = Arc::clone(&resolver.state); + let resolver_messenger = OnionMessenger::new( + Arc::clone(&resolver_keys), + Arc::clone(&resolver_keys), + resolver_logger, + DummyNodeLookup {}, + DirectlyConnectedRouter {}, + IgnoringMessageHandler {}, + IgnoringMessageHandler {}, + Arc::clone(&resolver), + IgnoringMessageHandler {}, + ); + let resolver_id = resolver_keys.get_node_id(Recipient::Node).unwrap(); + + let resolver_dest = Destination::Node(resolver_id); + let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_secs(); + + let payment_id = PaymentId([42; 32]); + let name = HumanReadableName::from_encoded("matt@mattcorallo.com").unwrap(); + + let payer_keys = Arc::new(KeysManager::new(&[2; 32], 42, 43)); + let payer_logger = TestLogger { node: "payer" }; + let payer_id = payer_keys.get_node_id(Recipient::Node).unwrap(); + let payer = Arc::new(URIResolver { + resolved_uri: Mutex::new(None), + resolver: OMNameResolver::new(now as u32, 1), + pending_messages: Mutex::new(Vec::new()), + }); + let payer_messenger = Arc::new(OnionMessenger::new( + Arc::clone(&payer_keys), + Arc::clone(&payer_keys), + payer_logger, + DummyNodeLookup {}, + DirectlyConnectedRouter {}, + IgnoringMessageHandler {}, + IgnoringMessageHandler {}, + Arc::clone(&payer), + IgnoringMessageHandler {}, + )); + + let init_msg = get_om_init(); + payer_messenger.peer_connected(resolver_id, &init_msg, true).unwrap(); + resolver_messenger.peer_connected(payer_id, &init_msg, false).unwrap(); + + let (msg, context) = + payer.resolver.resolve_name(payment_id, name.clone(), &*payer_keys).unwrap(); + let query_context = MessageContext::DNSResolver(context); + let reply_path = BlindedMessagePath::one_hop( + payer_id, + query_context, + &*payer_keys, + &secp_ctx, + ).unwrap(); + payer.pending_messages.lock().unwrap().push(( + DNSResolverMessage::DNSSECQuery(msg), + MessageSendInstructions::WithSpecifiedReplyPath { + destination: resolver_dest, + reply_path, + }, + )); + + let query = payer_messenger.next_onion_message_for_peer(resolver_id).unwrap(); + resolver_messenger.handle_onion_message(payer_id, &query); + + let start = Instant::now(); + while resolver_state.pending_query_count.load(Ordering::Relaxed) != 0 { + tokio::time::sleep(Duration::from_millis(50)).await; + assert!( + start.elapsed() < Duration::from_secs(10), + "pending_query_count not decremented after failed proof: counter leaks" + ); + } + } } diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs index 487e1a7ac14..de69c5744a6 100644 --- a/lightning-persister/src/fs_store.rs +++ b/lightning-persister/src/fs_store.rs @@ -331,19 +331,24 @@ impl KVStore for FilesystemStore { } } -fn dir_entry_is_key(p: &Path) -> Result { - if let Some(ext) = p.extension() { - #[cfg(target_os = "windows")] - { - // Clean up any trash files lying around. - if ext == "trash" { - fs::remove_file(p).ok(); - return Ok(false); +fn dir_entry_is_store_artifact(path: &Path) -> bool { + match path.extension().and_then(|ext| ext.to_str()) { + Some("tmp") => true, + Some("trash") => { + #[cfg(target_os = "windows")] + { + // Clean up any trash files lying around. + fs::remove_file(path).ok(); } - } - if ext == "tmp" { - return Ok(false); - } + true + }, + _ => false, + } +} + +fn dir_entry_is_key(p: &Path) -> Result { + if dir_entry_is_store_artifact(&p) { + return Ok(false); } let metadata = p.metadata().map_err(|e| { @@ -436,6 +441,9 @@ impl MigratableKVStore for FilesystemStore { 'primary_loop: for primary_entry in fs::read_dir(prefixed_dest)? { let primary_path = primary_entry?.path(); + if dir_entry_is_store_artifact(&primary_path) { + continue 'primary_loop; + } if dir_entry_is_key(&primary_path)? { let primary_namespace = String::new(); @@ -448,6 +456,9 @@ impl MigratableKVStore for FilesystemStore { // The primary_entry is actually also a directory. 'secondary_loop: for secondary_entry in fs::read_dir(&primary_path)? { let secondary_path = secondary_entry?.path(); + if dir_entry_is_store_artifact(&secondary_path) { + continue 'secondary_loop; + } if dir_entry_is_key(&secondary_path)? { let primary_namespace = get_key_from_dir_entry(&primary_path, prefixed_dest)?; @@ -461,6 +472,9 @@ impl MigratableKVStore for FilesystemStore { for tertiary_entry in fs::read_dir(&secondary_path)? { let tertiary_entry = tertiary_entry?; let tertiary_path = tertiary_entry.path(); + if dir_entry_is_store_artifact(&tertiary_path) { + continue; + } if dir_entry_is_key(&tertiary_path)? { let primary_namespace = @@ -529,6 +543,28 @@ mod tests { do_read_write_remove_list_persist(&fs_store); } + #[test] + fn list_all_keys_skips_leftover_store_artifacts() { + let mut temp_path = std::env::temp_dir(); + temp_path.push("test_list_all_keys_skips_leftover_store_artifacts"); + let fs_store = FilesystemStore::new(temp_path.clone()); + KVStore::write(&fs_store, "primary", "secondary", "key", &[1]).unwrap(); + + fs::write(temp_path.join("top_level.0.tmp"), b"stale").unwrap(); + fs::write(temp_path.join("top_level.0.trash"), b"stale").unwrap(); + + let primary_path = temp_path.join("primary"); + fs::write(primary_path.join("primary_level.0.tmp"), b"stale").unwrap(); + fs::write(primary_path.join("primary_level.0.trash"), b"stale").unwrap(); + + let secondary_path = primary_path.join("secondary"); + fs::write(secondary_path.join("secondary_level.0.tmp"), b"stale").unwrap(); + fs::write(secondary_path.join("secondary_level.0.trash"), b"stale").unwrap(); + + let keys = fs_store.list_all_keys().unwrap(); + assert_eq!(keys, vec![("primary".to_string(), "secondary".to_string(), "key".to_string())]); + } + #[test] fn test_data_migration() { let mut source_temp_path = std::env::temp_dir(); diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs index a442ff9e119..f26f7624983 100644 --- a/lightning-transaction-sync/src/electrum.rs +++ b/lightning-transaction-sync/src/electrum.rs @@ -335,11 +335,11 @@ where let mut filtered_history = script_history.iter().filter(|h| h.tx_hash == **txid); if let Some(history) = filtered_history.next() { - let prob_conf_height = history.height as u32; - if prob_conf_height <= 0 { + if history.height <= 0 { // Skip if it's a an unconfirmed entry. continue; } + let prob_conf_height = history.height as u32; let confirmed_tx = self.get_confirmed_tx(tx, prob_conf_height)?; confirmed_txs.push(confirmed_tx); } diff --git a/lightning-transaction-sync/src/esplora.rs b/lightning-transaction-sync/src/esplora.rs index a191260bc01..e96110cc164 100644 --- a/lightning-transaction-sync/src/esplora.rs +++ b/lightning-transaction-sync/src/esplora.rs @@ -367,8 +367,13 @@ where let mut matches = Vec::new(); let mut indexes = Vec::new(); - let _ = merkle_block.txn.extract_matches(&mut matches, &mut indexes); - if indexes.len() != 1 || matches.len() != 1 || matches[0] != txid { + let computed_merkle_root = + merkle_block.txn.extract_matches(&mut matches, &mut indexes).ok(); + if computed_merkle_root != Some(block_header.merkle_root) + || indexes.len() != 1 + || matches.len() != 1 + || matches[0] != txid + { log_error!(self.logger, "Retrieved Merkle block for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid); return Err(InternalError::Failed); } diff --git a/lightning-types/src/lib.rs b/lightning-types/src/lib.rs index 49e7e59084e..ecd2fff2147 100644 --- a/lightning-types/src/lib.rs +++ b/lightning-types/src/lib.rs @@ -27,3 +27,4 @@ pub mod features; pub mod payment; pub mod routing; pub mod string; +mod unicode; diff --git a/lightning-types/src/string.rs b/lightning-types/src/string.rs index ae5395a5289..a21cad411be 100644 --- a/lightning-types/src/string.rs +++ b/lightning-types/src/string.rs @@ -12,6 +12,8 @@ use alloc::string::String; use core::fmt; +use crate::unicode::*; + /// Struct to `Display` fields in a safe way using `PrintableString` #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Default)] pub struct UntrustedString(pub String); @@ -31,7 +33,13 @@ impl<'a> fmt::Display for PrintableString<'a> { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { use core::fmt::Write; for c in self.0.chars() { - let c = if c.is_control() { core::char::REPLACEMENT_CHARACTER } else { c }; + let is_other = is_unicode_general_category_other(c); + let is_unassigned = is_unicode_general_category_unassigned(c); + let c = if c.is_control() || is_other || is_unassigned { + core::char::REPLACEMENT_CHARACTER + } else { + c + }; f.write_char(c)?; } @@ -50,4 +58,24 @@ mod tests { "I \u{1F496} LDK!\u{FFFD}\u{26A1}", ); } + + #[test] + fn sanitizes_unicode_bidi_override_characters() { + // U+202E RIGHT-TO-LEFT OVERRIDE and friends are Unicode general category + // `Cf` (Format), not `Cc` (Control). They enable "Trojan Source" / + // bidi-spoofing attacks where an attacker-supplied string (e.g. a node + // alias gossiped from a peer) renders to a human reader as something + // other than its byte content. `PrintableString` is the sanitiser used + // for exactly these untrusted strings, so it must replace them. + let rendered = format!("{}", PrintableString("safe\u{202E}cipsxe.exe")); + assert!( + !rendered.contains('\u{202E}'), + "PrintableString left a U+202E RLO override in its output: {:?}", + rendered + ); + + // U+13440 is in the Egyptian Hieroglyph Format Controls block, but its + // general category is `Mn`, not `Cf`, so the `Cf` range ends at U+1343F. + assert_eq!(format!("{}", PrintableString("x\u{1343F}y\u{13440}z")), "x\u{FFFD}y\u{13440}z"); + } } diff --git a/lightning-types/src/unicode.rs b/lightning-types/src/unicode.rs new file mode 100644 index 00000000000..22b21969365 --- /dev/null +++ b/lightning-types/src/unicode.rs @@ -0,0 +1,799 @@ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. + +// Auto-generated from the Unicode Character Database (UnicodeData.txt) by +// contrib/gen_unicode_general_category.py. Do not edit by hand; rerun the +// generator with an updated UnicodeData.txt to refresh the table. + +/// Returns `true` if `c` is in Unicode general category `Cc` (Control), `Cf` +/// (Format), `Cs` (Surrogate), or `Co` (Private Use) — the assigned codepoints +/// in the top-level `C` ("Other") category. The `Cs` portion of the table is +/// unreachable for `char` input (a `char` cannot hold a surrogate) but is kept +/// so the table mirrors the source UCD data verbatim. The disjoint `Cn` +/// (Unassigned) bucket is `is_unicode_general_category_unassigned`. +#[allow(dead_code)] +pub(crate) fn is_unicode_general_category_other(c: char) -> bool { + matches!( + c as u32, + 0x0000..=0x001F // control + | 0x007F..=0x009F // control + | 0x00AD // SOFT HYPHEN + | 0x0600..=0x0605 // ARABIC + | 0x061C // ARABIC LETTER MARK + | 0x06DD // ARABIC END OF AYAH + | 0x070F // SYRIAC ABBREVIATION MARK + | 0x0890..=0x0891 // MARK ABOVE + | 0x08E2 // ARABIC DISPUTED END OF AYAH + | 0x180E // MONGOLIAN VOWEL SEPARATOR + | 0x200B..=0x200F // Cf + | 0x202A..=0x202E // Cf + | 0x2060..=0x2064 // Cf + | 0x2066..=0x206F // Cf + | 0xD800..=0xF8FF // Co / Cs + | 0xFEFF // ZERO WIDTH NO-BREAK SPACE + | 0xFFF9..=0xFFFB // INTERLINEAR ANNOTATION + | 0x110BD // KAITHI NUMBER SIGN + | 0x110CD // KAITHI NUMBER SIGN ABOVE + | 0x13430..=0x1343F // EGYPTIAN HIEROGLYPH + | 0x1BCA0..=0x1BCA3 // SHORTHAND FORMAT + | 0x1D173..=0x1D17A // MUSICAL SYMBOL + | 0xE0001 // LANGUAGE TAG + | 0xE0020..=0xE007F // Cf + | 0xF0000..=0xFFFFD // Plane 15 Private Use + | 0x100000..=0x10FFFD // Plane 16 Private Use + ) +} + +/// Returns `true` if `c` is in Unicode general category `Cn` (Unassigned), or +/// strictly above U+10FFFF. The trailing `0x110000..=u32::MAX` arm is +/// unreachable for `char` input (a `char` is bounded to U+10FFFF) but is kept +/// for defensive coverage of the underlying `u32`. The disjoint Cc / Cf / Cs / +/// Co bucket is `is_unicode_general_category_other`. +#[allow(dead_code)] +pub(crate) fn is_unicode_general_category_unassigned(c: char) -> bool { + matches!( + c as u32, + 0x0378..=0x0379 + | 0x0380..=0x0383 + | 0x038B + | 0x038D + | 0x03A2 + | 0x0530 + | 0x0557..=0x0558 + | 0x058B..=0x058C + | 0x0590 + | 0x05C8..=0x05CF + | 0x05EB..=0x05EE + | 0x05F5..=0x05FF + | 0x070E + | 0x074B..=0x074C + | 0x07B2..=0x07BF + | 0x07FB..=0x07FC + | 0x082E..=0x082F + | 0x083F + | 0x085C..=0x085D + | 0x085F + | 0x086B..=0x086F + | 0x0892..=0x0896 + | 0x0984 + | 0x098D..=0x098E + | 0x0991..=0x0992 + | 0x09A9 + | 0x09B1 + | 0x09B3..=0x09B5 + | 0x09BA..=0x09BB + | 0x09C5..=0x09C6 + | 0x09C9..=0x09CA + | 0x09CF..=0x09D6 + | 0x09D8..=0x09DB + | 0x09DE + | 0x09E4..=0x09E5 + | 0x09FF..=0x0A00 + | 0x0A04 + | 0x0A0B..=0x0A0E + | 0x0A11..=0x0A12 + | 0x0A29 + | 0x0A31 + | 0x0A34 + | 0x0A37 + | 0x0A3A..=0x0A3B + | 0x0A3D + | 0x0A43..=0x0A46 + | 0x0A49..=0x0A4A + | 0x0A4E..=0x0A50 + | 0x0A52..=0x0A58 + | 0x0A5D + | 0x0A5F..=0x0A65 + | 0x0A77..=0x0A80 + | 0x0A84 + | 0x0A8E + | 0x0A92 + | 0x0AA9 + | 0x0AB1 + | 0x0AB4 + | 0x0ABA..=0x0ABB + | 0x0AC6 + | 0x0ACA + | 0x0ACE..=0x0ACF + | 0x0AD1..=0x0ADF + | 0x0AE4..=0x0AE5 + | 0x0AF2..=0x0AF8 + | 0x0B00 + | 0x0B04 + | 0x0B0D..=0x0B0E + | 0x0B11..=0x0B12 + | 0x0B29 + | 0x0B31 + | 0x0B34 + | 0x0B3A..=0x0B3B + | 0x0B45..=0x0B46 + | 0x0B49..=0x0B4A + | 0x0B4E..=0x0B54 + | 0x0B58..=0x0B5B + | 0x0B5E + | 0x0B64..=0x0B65 + | 0x0B78..=0x0B81 + | 0x0B84 + | 0x0B8B..=0x0B8D + | 0x0B91 + | 0x0B96..=0x0B98 + | 0x0B9B + | 0x0B9D + | 0x0BA0..=0x0BA2 + | 0x0BA5..=0x0BA7 + | 0x0BAB..=0x0BAD + | 0x0BBA..=0x0BBD + | 0x0BC3..=0x0BC5 + | 0x0BC9 + | 0x0BCE..=0x0BCF + | 0x0BD1..=0x0BD6 + | 0x0BD8..=0x0BE5 + | 0x0BFB..=0x0BFF + | 0x0C0D + | 0x0C11 + | 0x0C29 + | 0x0C3A..=0x0C3B + | 0x0C45 + | 0x0C49 + | 0x0C4E..=0x0C54 + | 0x0C57 + | 0x0C5B + | 0x0C5E..=0x0C5F + | 0x0C64..=0x0C65 + | 0x0C70..=0x0C76 + | 0x0C8D + | 0x0C91 + | 0x0CA9 + | 0x0CB4 + | 0x0CBA..=0x0CBB + | 0x0CC5 + | 0x0CC9 + | 0x0CCE..=0x0CD4 + | 0x0CD7..=0x0CDB + | 0x0CDF + | 0x0CE4..=0x0CE5 + | 0x0CF0 + | 0x0CF4..=0x0CFF + | 0x0D0D + | 0x0D11 + | 0x0D45 + | 0x0D49 + | 0x0D50..=0x0D53 + | 0x0D64..=0x0D65 + | 0x0D80 + | 0x0D84 + | 0x0D97..=0x0D99 + | 0x0DB2 + | 0x0DBC + | 0x0DBE..=0x0DBF + | 0x0DC7..=0x0DC9 + | 0x0DCB..=0x0DCE + | 0x0DD5 + | 0x0DD7 + | 0x0DE0..=0x0DE5 + | 0x0DF0..=0x0DF1 + | 0x0DF5..=0x0E00 + | 0x0E3B..=0x0E3E + | 0x0E5C..=0x0E80 + | 0x0E83 + | 0x0E85 + | 0x0E8B + | 0x0EA4 + | 0x0EA6 + | 0x0EBE..=0x0EBF + | 0x0EC5 + | 0x0EC7 + | 0x0ECF + | 0x0EDA..=0x0EDB + | 0x0EE0..=0x0EFF + | 0x0F48 + | 0x0F6D..=0x0F70 + | 0x0F98 + | 0x0FBD + | 0x0FCD + | 0x0FDB..=0x0FFF + | 0x10C6 + | 0x10C8..=0x10CC + | 0x10CE..=0x10CF + | 0x1249 + | 0x124E..=0x124F + | 0x1257 + | 0x1259 + | 0x125E..=0x125F + | 0x1289 + | 0x128E..=0x128F + | 0x12B1 + | 0x12B6..=0x12B7 + | 0x12BF + | 0x12C1 + | 0x12C6..=0x12C7 + | 0x12D7 + | 0x1311 + | 0x1316..=0x1317 + | 0x135B..=0x135C + | 0x137D..=0x137F + | 0x139A..=0x139F + | 0x13F6..=0x13F7 + | 0x13FE..=0x13FF + | 0x169D..=0x169F + | 0x16F9..=0x16FF + | 0x1716..=0x171E + | 0x1737..=0x173F + | 0x1754..=0x175F + | 0x176D + | 0x1771 + | 0x1774..=0x177F + | 0x17DE..=0x17DF + | 0x17EA..=0x17EF + | 0x17FA..=0x17FF + | 0x181A..=0x181F + | 0x1879..=0x187F + | 0x18AB..=0x18AF + | 0x18F6..=0x18FF + | 0x191F + | 0x192C..=0x192F + | 0x193C..=0x193F + | 0x1941..=0x1943 + | 0x196E..=0x196F + | 0x1975..=0x197F + | 0x19AC..=0x19AF + | 0x19CA..=0x19CF + | 0x19DB..=0x19DD + | 0x1A1C..=0x1A1D + | 0x1A5F + | 0x1A7D..=0x1A7E + | 0x1A8A..=0x1A8F + | 0x1A9A..=0x1A9F + | 0x1AAE..=0x1AAF + | 0x1ADE..=0x1ADF + | 0x1AEC..=0x1AFF + | 0x1B4D + | 0x1BF4..=0x1BFB + | 0x1C38..=0x1C3A + | 0x1C4A..=0x1C4C + | 0x1C8B..=0x1C8F + | 0x1CBB..=0x1CBC + | 0x1CC8..=0x1CCF + | 0x1CFB..=0x1CFF + | 0x1F16..=0x1F17 + | 0x1F1E..=0x1F1F + | 0x1F46..=0x1F47 + | 0x1F4E..=0x1F4F + | 0x1F58 + | 0x1F5A + | 0x1F5C + | 0x1F5E + | 0x1F7E..=0x1F7F + | 0x1FB5 + | 0x1FC5 + | 0x1FD4..=0x1FD5 + | 0x1FDC + | 0x1FF0..=0x1FF1 + | 0x1FF5 + | 0x1FFF + | 0x2065 + | 0x2072..=0x2073 + | 0x208F + | 0x209D..=0x209F + | 0x20C2..=0x20CF + | 0x20F1..=0x20FF + | 0x218C..=0x218F + | 0x242A..=0x243F + | 0x244B..=0x245F + | 0x2B74..=0x2B75 + | 0x2CF4..=0x2CF8 + | 0x2D26 + | 0x2D28..=0x2D2C + | 0x2D2E..=0x2D2F + | 0x2D68..=0x2D6E + | 0x2D71..=0x2D7E + | 0x2D97..=0x2D9F + | 0x2DA7 + | 0x2DAF + | 0x2DB7 + | 0x2DBF + | 0x2DC7 + | 0x2DCF + | 0x2DD7 + | 0x2DDF + | 0x2E5E..=0x2E7F + | 0x2E9A + | 0x2EF4..=0x2EFF + | 0x2FD6..=0x2FEF + | 0x3040 + | 0x3097..=0x3098 + | 0x3100..=0x3104 + | 0x3130 + | 0x318F + | 0x31E6..=0x31EE + | 0x321F + | 0xA48D..=0xA48F + | 0xA4C7..=0xA4CF + | 0xA62C..=0xA63F + | 0xA6F8..=0xA6FF + | 0xA7DD..=0xA7F0 + | 0xA82D..=0xA82F + | 0xA83A..=0xA83F + | 0xA878..=0xA87F + | 0xA8C6..=0xA8CD + | 0xA8DA..=0xA8DF + | 0xA954..=0xA95E + | 0xA97D..=0xA97F + | 0xA9CE + | 0xA9DA..=0xA9DD + | 0xA9FF + | 0xAA37..=0xAA3F + | 0xAA4E..=0xAA4F + | 0xAA5A..=0xAA5B + | 0xAAC3..=0xAADA + | 0xAAF7..=0xAB00 + | 0xAB07..=0xAB08 + | 0xAB0F..=0xAB10 + | 0xAB17..=0xAB1F + | 0xAB27 + | 0xAB2F + | 0xAB6C..=0xAB6F + | 0xABEE..=0xABEF + | 0xABFA..=0xABFF + | 0xD7A4..=0xD7AF + | 0xD7C7..=0xD7CA + | 0xD7FC..=0xD7FF + | 0xFA6E..=0xFA6F + | 0xFADA..=0xFAFF + | 0xFB07..=0xFB12 + | 0xFB18..=0xFB1C + | 0xFB37 + | 0xFB3D + | 0xFB3F + | 0xFB42 + | 0xFB45 + | 0xFDD0..=0xFDEF + | 0xFE1A..=0xFE1F + | 0xFE53 + | 0xFE67 + | 0xFE6C..=0xFE6F + | 0xFE75 + | 0xFEFD..=0xFEFE + | 0xFF00 + | 0xFFBF..=0xFFC1 + | 0xFFC8..=0xFFC9 + | 0xFFD0..=0xFFD1 + | 0xFFD8..=0xFFD9 + | 0xFFDD..=0xFFDF + | 0xFFE7 + | 0xFFEF..=0xFFF8 + | 0xFFFE..=0xFFFF + | 0x1000C + | 0x10027 + | 0x1003B + | 0x1003E + | 0x1004E..=0x1004F + | 0x1005E..=0x1007F + | 0x100FB..=0x100FF + | 0x10103..=0x10106 + | 0x10134..=0x10136 + | 0x1018F + | 0x1019D..=0x1019F + | 0x101A1..=0x101CF + | 0x101FE..=0x1027F + | 0x1029D..=0x1029F + | 0x102D1..=0x102DF + | 0x102FC..=0x102FF + | 0x10324..=0x1032C + | 0x1034B..=0x1034F + | 0x1037B..=0x1037F + | 0x1039E + | 0x103C4..=0x103C7 + | 0x103D6..=0x103FF + | 0x1049E..=0x1049F + | 0x104AA..=0x104AF + | 0x104D4..=0x104D7 + | 0x104FC..=0x104FF + | 0x10528..=0x1052F + | 0x10564..=0x1056E + | 0x1057B + | 0x1058B + | 0x10593 + | 0x10596 + | 0x105A2 + | 0x105B2 + | 0x105BA + | 0x105BD..=0x105BF + | 0x105F4..=0x105FF + | 0x10737..=0x1073F + | 0x10756..=0x1075F + | 0x10768..=0x1077F + | 0x10786 + | 0x107B1 + | 0x107BB..=0x107FF + | 0x10806..=0x10807 + | 0x10809 + | 0x10836 + | 0x10839..=0x1083B + | 0x1083D..=0x1083E + | 0x10856 + | 0x1089F..=0x108A6 + | 0x108B0..=0x108DF + | 0x108F3 + | 0x108F6..=0x108FA + | 0x1091C..=0x1091E + | 0x1093A..=0x1093E + | 0x1095A..=0x1097F + | 0x109B8..=0x109BB + | 0x109D0..=0x109D1 + | 0x10A04 + | 0x10A07..=0x10A0B + | 0x10A14 + | 0x10A18 + | 0x10A36..=0x10A37 + | 0x10A3B..=0x10A3E + | 0x10A49..=0x10A4F + | 0x10A59..=0x10A5F + | 0x10AA0..=0x10ABF + | 0x10AE7..=0x10AEA + | 0x10AF7..=0x10AFF + | 0x10B36..=0x10B38 + | 0x10B56..=0x10B57 + | 0x10B73..=0x10B77 + | 0x10B92..=0x10B98 + | 0x10B9D..=0x10BA8 + | 0x10BB0..=0x10BFF + | 0x10C49..=0x10C7F + | 0x10CB3..=0x10CBF + | 0x10CF3..=0x10CF9 + | 0x10D28..=0x10D2F + | 0x10D3A..=0x10D3F + | 0x10D66..=0x10D68 + | 0x10D86..=0x10D8D + | 0x10D90..=0x10E5F + | 0x10E7F + | 0x10EAA + | 0x10EAE..=0x10EAF + | 0x10EB2..=0x10EC1 + | 0x10EC8..=0x10ECF + | 0x10ED9..=0x10EF9 + | 0x10F28..=0x10F2F + | 0x10F5A..=0x10F6F + | 0x10F8A..=0x10FAF + | 0x10FCC..=0x10FDF + | 0x10FF7..=0x10FFF + | 0x1104E..=0x11051 + | 0x11076..=0x1107E + | 0x110C3..=0x110CC + | 0x110CE..=0x110CF + | 0x110E9..=0x110EF + | 0x110FA..=0x110FF + | 0x11135 + | 0x11148..=0x1114F + | 0x11177..=0x1117F + | 0x111E0 + | 0x111F5..=0x111FF + | 0x11212 + | 0x11242..=0x1127F + | 0x11287 + | 0x11289 + | 0x1128E + | 0x1129E + | 0x112AA..=0x112AF + | 0x112EB..=0x112EF + | 0x112FA..=0x112FF + | 0x11304 + | 0x1130D..=0x1130E + | 0x11311..=0x11312 + | 0x11329 + | 0x11331 + | 0x11334 + | 0x1133A + | 0x11345..=0x11346 + | 0x11349..=0x1134A + | 0x1134E..=0x1134F + | 0x11351..=0x11356 + | 0x11358..=0x1135C + | 0x11364..=0x11365 + | 0x1136D..=0x1136F + | 0x11375..=0x1137F + | 0x1138A + | 0x1138C..=0x1138D + | 0x1138F + | 0x113B6 + | 0x113C1 + | 0x113C3..=0x113C4 + | 0x113C6 + | 0x113CB + | 0x113D6 + | 0x113D9..=0x113E0 + | 0x113E3..=0x113FF + | 0x1145C + | 0x11462..=0x1147F + | 0x114C8..=0x114CF + | 0x114DA..=0x1157F + | 0x115B6..=0x115B7 + | 0x115DE..=0x115FF + | 0x11645..=0x1164F + | 0x1165A..=0x1165F + | 0x1166D..=0x1167F + | 0x116BA..=0x116BF + | 0x116CA..=0x116CF + | 0x116E4..=0x116FF + | 0x1171B..=0x1171C + | 0x1172C..=0x1172F + | 0x11747..=0x117FF + | 0x1183C..=0x1189F + | 0x118F3..=0x118FE + | 0x11907..=0x11908 + | 0x1190A..=0x1190B + | 0x11914 + | 0x11917 + | 0x11936 + | 0x11939..=0x1193A + | 0x11947..=0x1194F + | 0x1195A..=0x1199F + | 0x119A8..=0x119A9 + | 0x119D8..=0x119D9 + | 0x119E5..=0x119FF + | 0x11A48..=0x11A4F + | 0x11AA3..=0x11AAF + | 0x11AF9..=0x11AFF + | 0x11B0A..=0x11B5F + | 0x11B68..=0x11BBF + | 0x11BE2..=0x11BEF + | 0x11BFA..=0x11BFF + | 0x11C09 + | 0x11C37 + | 0x11C46..=0x11C4F + | 0x11C6D..=0x11C6F + | 0x11C90..=0x11C91 + | 0x11CA8 + | 0x11CB7..=0x11CFF + | 0x11D07 + | 0x11D0A + | 0x11D37..=0x11D39 + | 0x11D3B + | 0x11D3E + | 0x11D48..=0x11D4F + | 0x11D5A..=0x11D5F + | 0x11D66 + | 0x11D69 + | 0x11D8F + | 0x11D92 + | 0x11D99..=0x11D9F + | 0x11DAA..=0x11DAF + | 0x11DDC..=0x11DDF + | 0x11DEA..=0x11EDF + | 0x11EF9..=0x11EFF + | 0x11F11 + | 0x11F3B..=0x11F3D + | 0x11F5B..=0x11FAF + | 0x11FB1..=0x11FBF + | 0x11FF2..=0x11FFE + | 0x1239A..=0x123FF + | 0x1246F + | 0x12475..=0x1247F + | 0x12544..=0x12F8F + | 0x12FF3..=0x12FFF + | 0x13456..=0x1345F + | 0x143FB..=0x143FF + | 0x14647..=0x160FF + | 0x1613A..=0x167FF + | 0x16A39..=0x16A3F + | 0x16A5F + | 0x16A6A..=0x16A6D + | 0x16ABF + | 0x16ACA..=0x16ACF + | 0x16AEE..=0x16AEF + | 0x16AF6..=0x16AFF + | 0x16B46..=0x16B4F + | 0x16B5A + | 0x16B62 + | 0x16B78..=0x16B7C + | 0x16B90..=0x16D3F + | 0x16D7A..=0x16E3F + | 0x16E9B..=0x16E9F + | 0x16EB9..=0x16EBA + | 0x16ED4..=0x16EFF + | 0x16F4B..=0x16F4E + | 0x16F88..=0x16F8E + | 0x16FA0..=0x16FDF + | 0x16FE5..=0x16FEF + | 0x16FF7..=0x16FFF + | 0x18CD6..=0x18CFE + | 0x18D1F..=0x18D7F + | 0x18DF3..=0x1AFEF + | 0x1AFF4 + | 0x1AFFC + | 0x1AFFF + | 0x1B123..=0x1B131 + | 0x1B133..=0x1B14F + | 0x1B153..=0x1B154 + | 0x1B156..=0x1B163 + | 0x1B168..=0x1B16F + | 0x1B2FC..=0x1BBFF + | 0x1BC6B..=0x1BC6F + | 0x1BC7D..=0x1BC7F + | 0x1BC89..=0x1BC8F + | 0x1BC9A..=0x1BC9B + | 0x1BCA4..=0x1CBFF + | 0x1CCFD..=0x1CCFF + | 0x1CEB4..=0x1CEB9 + | 0x1CED1..=0x1CEDF + | 0x1CEF1..=0x1CEFF + | 0x1CF2E..=0x1CF2F + | 0x1CF47..=0x1CF4F + | 0x1CFC4..=0x1CFFF + | 0x1D0F6..=0x1D0FF + | 0x1D127..=0x1D128 + | 0x1D1EB..=0x1D1FF + | 0x1D246..=0x1D2BF + | 0x1D2D4..=0x1D2DF + | 0x1D2F4..=0x1D2FF + | 0x1D357..=0x1D35F + | 0x1D379..=0x1D3FF + | 0x1D455 + | 0x1D49D + | 0x1D4A0..=0x1D4A1 + | 0x1D4A3..=0x1D4A4 + | 0x1D4A7..=0x1D4A8 + | 0x1D4AD + | 0x1D4BA + | 0x1D4BC + | 0x1D4C4 + | 0x1D506 + | 0x1D50B..=0x1D50C + | 0x1D515 + | 0x1D51D + | 0x1D53A + | 0x1D53F + | 0x1D545 + | 0x1D547..=0x1D549 + | 0x1D551 + | 0x1D6A6..=0x1D6A7 + | 0x1D7CC..=0x1D7CD + | 0x1DA8C..=0x1DA9A + | 0x1DAA0 + | 0x1DAB0..=0x1DEFF + | 0x1DF1F..=0x1DF24 + | 0x1DF2B..=0x1DFFF + | 0x1E007 + | 0x1E019..=0x1E01A + | 0x1E022 + | 0x1E025 + | 0x1E02B..=0x1E02F + | 0x1E06E..=0x1E08E + | 0x1E090..=0x1E0FF + | 0x1E12D..=0x1E12F + | 0x1E13E..=0x1E13F + | 0x1E14A..=0x1E14D + | 0x1E150..=0x1E28F + | 0x1E2AF..=0x1E2BF + | 0x1E2FA..=0x1E2FE + | 0x1E300..=0x1E4CF + | 0x1E4FA..=0x1E5CF + | 0x1E5FB..=0x1E5FE + | 0x1E600..=0x1E6BF + | 0x1E6DF + | 0x1E6F6..=0x1E6FD + | 0x1E700..=0x1E7DF + | 0x1E7E7 + | 0x1E7EC + | 0x1E7EF + | 0x1E7FF + | 0x1E8C5..=0x1E8C6 + | 0x1E8D7..=0x1E8FF + | 0x1E94C..=0x1E94F + | 0x1E95A..=0x1E95D + | 0x1E960..=0x1EC70 + | 0x1ECB5..=0x1ED00 + | 0x1ED3E..=0x1EDFF + | 0x1EE04 + | 0x1EE20 + | 0x1EE23 + | 0x1EE25..=0x1EE26 + | 0x1EE28 + | 0x1EE33 + | 0x1EE38 + | 0x1EE3A + | 0x1EE3C..=0x1EE41 + | 0x1EE43..=0x1EE46 + | 0x1EE48 + | 0x1EE4A + | 0x1EE4C + | 0x1EE50 + | 0x1EE53 + | 0x1EE55..=0x1EE56 + | 0x1EE58 + | 0x1EE5A + | 0x1EE5C + | 0x1EE5E + | 0x1EE60 + | 0x1EE63 + | 0x1EE65..=0x1EE66 + | 0x1EE6B + | 0x1EE73 + | 0x1EE78 + | 0x1EE7D + | 0x1EE7F + | 0x1EE8A + | 0x1EE9C..=0x1EEA0 + | 0x1EEA4 + | 0x1EEAA + | 0x1EEBC..=0x1EEEF + | 0x1EEF2..=0x1EFFF + | 0x1F02C..=0x1F02F + | 0x1F094..=0x1F09F + | 0x1F0AF..=0x1F0B0 + | 0x1F0C0 + | 0x1F0D0 + | 0x1F0F6..=0x1F0FF + | 0x1F1AE..=0x1F1E5 + | 0x1F203..=0x1F20F + | 0x1F23C..=0x1F23F + | 0x1F249..=0x1F24F + | 0x1F252..=0x1F25F + | 0x1F266..=0x1F2FF + | 0x1F6D9..=0x1F6DB + | 0x1F6ED..=0x1F6EF + | 0x1F6FD..=0x1F6FF + | 0x1F7DA..=0x1F7DF + | 0x1F7EC..=0x1F7EF + | 0x1F7F1..=0x1F7FF + | 0x1F80C..=0x1F80F + | 0x1F848..=0x1F84F + | 0x1F85A..=0x1F85F + | 0x1F888..=0x1F88F + | 0x1F8AE..=0x1F8AF + | 0x1F8BC..=0x1F8BF + | 0x1F8C2..=0x1F8CF + | 0x1F8D9..=0x1F8FF + | 0x1FA58..=0x1FA5F + | 0x1FA6E..=0x1FA6F + | 0x1FA7D..=0x1FA7F + | 0x1FA8B..=0x1FA8D + | 0x1FAC7 + | 0x1FAC9..=0x1FACC + | 0x1FADD..=0x1FADE + | 0x1FAEB..=0x1FAEE + | 0x1FAF9..=0x1FAFF + | 0x1FB93 + | 0x1FBFB..=0x1FFFF + | 0x2A6E0..=0x2A6FF + | 0x2B81E..=0x2B81F + | 0x2CEAE..=0x2CEAF + | 0x2EBE1..=0x2EBEF + | 0x2EE5E..=0x2F7FF + | 0x2FA1E..=0x2FFFF + | 0x3134B..=0x3134F + | 0x3347A..=0xE0000 + | 0xE0002..=0xE001F + | 0xE0080..=0xE00FF + | 0xE01F0..=0xEFFFF + | 0xFFFFE..=0xFFFFF + | 0x10FFFE..=0x10FFFF + | 0x110000..=u32::MAX // above U+10FFFF — unreachable for `char` + ) +} diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index f1b9f729cb3..5136b07c0f3 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -327,7 +327,7 @@ where C::Target: chain::Filter, let funding_txid_hash = funding_outpoint.txid.to_raw_hash(); let funding_txid_hash_bytes = funding_txid_hash.as_byte_array(); let funding_txid_u32 = u32::from_be_bytes([funding_txid_hash_bytes[0], funding_txid_hash_bytes[1], funding_txid_hash_bytes[2], funding_txid_hash_bytes[3]]); - funding_txid_u32.wrapping_add(best_height.unwrap_or_default()) + best_height.map(|height| funding_txid_u32.wrapping_add(height)) }; let partition_factor = if channel_count < 15 { @@ -337,7 +337,7 @@ where C::Target: chain::Filter, }; let has_pending_claims = monitor_state.monitor.has_pending_claims(); - if has_pending_claims || get_partition_key(funding_outpoint) % partition_factor == 0 { + if has_pending_claims || get_partition_key(funding_outpoint).is_some_and(|key| key % partition_factor == 0) { log_trace!(logger, "Syncing Channel Monitor for channel {}", log_funding_info!(monitor)); // Even though we don't track monitor updates from chain-sync as pending, we still want // updates per-channel to be well-ordered so that users don't see a diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 4b492b0607a..4db61271c36 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -930,7 +930,8 @@ pub enum Event { /// If the recipient or an intermediate node misbehaves and gives us free money, this may /// overstate the amount paid, though this is unlikely. /// - /// This is only `None` for payments initiated on LDK versions prior to 0.0.103. + /// This is only `None` for payments abandoned but ultimately claimed when using LDK versions + /// prior to 0.3, 0.2.3, or 0.1.10. /// /// [`Route::get_total_fees`]: crate::routing::router::Route::get_total_fees fee_paid_msat: Option, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index eaca6e03beb..538068ab594 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1075,8 +1075,15 @@ enum BackgroundEvent { /// on a channel. MonitorUpdatesComplete { counterparty_node_id: PublicKey, + funding_txo: OutPoint, channel_id: ChannelId, + highest_update_id_completed: u64, }, + /// A channel had blocked monitor updates waiting on startup. If the updates were blocked on + /// an MPP claim blocker not written to disk, we may be able to unblock them now. + /// + /// This event is never written to disk. + AttemptUnblockMonitorUpdates { counterparty_node_id: PublicKey, channel_id: ChannelId }, } /// A pointer to a channel that is unblocked when an event is surfaced @@ -3235,8 +3242,21 @@ macro_rules! emit_channel_ready_event { } } +/// Handles the completion steps for when a [`ChannelMonitorUpdate`] is applied to a live channel. +/// +/// You should not add new direct calls to this, generally, rather rely on +/// `handle_new_monitor_update` or [`ChannelManager::channel_monitor_updated`] to call it for you. +/// +/// Requires that the in-flight monitor update set for this channel is empty! macro_rules! handle_monitor_update_completion { ($self: ident, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr) => { { + #[cfg(debug_assertions)] + if let Some(funding_txo) = $chan.context.get_funding_txo() { + let in_flight_updates = + $peer_state.in_flight_monitor_updates.get(&funding_txo); + assert!(in_flight_updates.map(|updates| updates.is_empty()).unwrap_or(true)); + } + debug_assert!($chan.is_awaiting_monitor_update()); let logger = WithChannelContext::from(&$self.logger, &$chan.context, None); let update_actions = $peer_state.monitor_update_blocked_actions @@ -4101,19 +4121,7 @@ where // TODO: If we do the `in_flight_monitor_updates.is_empty()` check in // `locked_close_channel` we can skip the locks here. if let Some(funding_txo) = shutdown_res.channel_funding_txo { - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mtx) = per_peer_state.get(&shutdown_res.counterparty_node_id) { - let mut peer_state = peer_state_mtx.lock().unwrap(); - if peer_state.in_flight_monitor_updates.get(&funding_txo).map(|l| l.is_empty()).unwrap_or(true) { - let update_actions = peer_state.monitor_update_blocked_actions - .remove(&shutdown_res.channel_id).unwrap_or(Vec::new()); - - mem::drop(peer_state); - mem::drop(per_peer_state); - - self.handle_monitor_update_completion_actions(update_actions); - } - } + self.channel_monitor_updated(&funding_txo, &shutdown_res.channel_id, None, Some(&shutdown_res.counterparty_node_id)); } } let mut shutdown_results = Vec::new(); @@ -6391,7 +6399,10 @@ where /// /// Expects the caller to have a total_consistency_lock read lock. fn process_background_events(&self) -> NotifyOption { - debug_assert_ne!(self.total_consistency_lock.held_by_thread(), LockHeldState::NotHeldByThread); + debug_assert_ne!( + self.total_consistency_lock.held_by_thread(), + LockHeldState::NotHeldByThread + ); self.background_events_processed_since_startup.store(true, Ordering::Release); @@ -6408,24 +6419,42 @@ where // monitor updating completing. let _ = self.chain_monitor.update_channel(funding_txo, &update); }, - BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { - self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update); + BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id, + funding_txo, + channel_id, + update, + } => { + self.apply_post_close_monitor_update( + counterparty_node_id, + channel_id, + funding_txo, + update, + ); }, - BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, channel_id } => { - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { - 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) { - 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()); - mem::drop(peer_state_lock); - mem::drop(per_peer_state); - self.handle_monitor_update_completion_actions(update_actions); - } - } + BackgroundEvent::MonitorUpdatesComplete { + counterparty_node_id, + funding_txo, + channel_id, + highest_update_id_completed, + } => { + // Now that we can finally handle the background event, remove all in-flight + // monitor updates for this channel that we've known to complete, as they have + // already been persisted to the monitor and can be applied to our internal + // state such that the channel resumes operation if no new updates have been + // made since. + self.channel_monitor_updated( + &funding_txo, + &channel_id, + Some(highest_update_id_completed), + Some(&counterparty_node_id), + ); + }, + BackgroundEvent::AttemptUnblockMonitorUpdates { + counterparty_node_id, + channel_id, + } => { + self.handle_monitor_update_release(counterparty_node_id, channel_id, None); }, } } @@ -6707,23 +6736,20 @@ where debug_assert!(false); return false; } - if let OnionPayload::Invoice { .. } = payment.htlcs[0].onion_payload { - // Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat). - // In this case we're not going to handle any timeouts of the parts here. - // This condition determining whether the MPP is complete here must match - // exactly the condition used in `process_pending_htlc_forwards`. - if payment.htlcs[0].total_msat <= payment.htlcs.iter() - .fold(0, |total, htlc| total + htlc.sender_intended_value) - { - return true; - } else if payment.htlcs.iter_mut().any(|htlc| { - htlc.timer_ticks += 1; - return htlc.timer_ticks >= MPP_TIMEOUT_TICKS - }) { - timed_out_mpp_htlcs.extend(payment.htlcs.drain(..) - .map(|htlc: ClaimableHTLC| (htlc.prev_hop, *payment_hash))); - return false; - } + // Check if we've received all the parts we need for an MPP. + // This condition determining whether the MPP is complete here must match + // exactly the condition used in `process_pending_htlc_forwards`. + if payment.htlcs[0].total_msat <= payment.htlcs.iter() + .fold(0, |total, htlc| total + htlc.sender_intended_value) + { + return true; + } else if payment.htlcs.iter_mut().any(|htlc| { + htlc.timer_ticks += 1; + return htlc.timer_ticks >= MPP_TIMEOUT_TICKS + }) { + timed_out_mpp_htlcs.extend(payment.htlcs.drain(..) + .map(|htlc: ClaimableHTLC| (htlc.prev_hop, *payment_hash))); + return false; } true }); @@ -7321,12 +7347,12 @@ where { 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 + let entry = peer_state .actions_blocking_raa_monitor_updates - .get_mut(&channel_id) - { + .entry(channel_id); + if let btree_map::Entry::Occupied(mut entry) = entry { let mut found_blocker = false; - blockers.retain(|iter| { + entry.get_mut().retain(|iter| { // Note that we could actually be blocked, in // which case we need to only remove the one // blocker which was added duplicatively. @@ -7334,6 +7360,9 @@ where if *iter == blocker { found_blocker = true; } *iter != blocker || !first_blocker }); + if entry.get().is_empty() { + entry.remove(); + } debug_assert!(found_blocker); } } else { @@ -7785,7 +7814,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ (htlc_forwards, decode_update_add_htlcs) } - fn channel_monitor_updated(&self, funding_txo: &OutPoint, channel_id: &ChannelId, highest_applied_update_id: u64, counterparty_node_id: Option<&PublicKey>) { + fn channel_monitor_updated(&self, funding_txo: &OutPoint, channel_id: &ChannelId, highest_applied_update_id: Option, counterparty_node_id: Option<&PublicKey>) { debug_assert!(self.total_consistency_lock.try_write().is_err()); // Caller holds read lock let counterparty_node_id = match counterparty_node_id { @@ -7807,16 +7836,33 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); let peer_state = &mut *peer_state_lock; + let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(*channel_id), None); let remaining_in_flight = if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) { - pending.retain(|upd| upd.update_id > highest_applied_update_id); + if let Some(highest_applied_update_id) = highest_applied_update_id { + pending.retain(|upd| upd.update_id > highest_applied_update_id); + log_trace!( + logger, + "ChannelMonitor updated to {highest_applied_update_id}. {} pending in-flight updates.", + pending.len() + ); + } else if let Some(update) = pending.get(0) { + log_trace!( + logger, + "ChannelMonitor updated to {}. {} pending in-flight updates.", + update.update_id - 1, + pending.len() + ); + } else { + log_trace!( + logger, + "ChannelMonitor updated. {} pending in-flight updates.", + pending.len() + ); + } pending.len() } else { 0 }; - let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(*channel_id), None); - log_trace!(logger, "ChannelMonitor updated to {}. {} pending in-flight updates.", - highest_applied_update_id, remaining_in_flight); - if remaining_in_flight != 0 { return; } @@ -9651,7 +9697,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } }, MonitorEvent::Completed { funding_txo, channel_id, monitor_update_id } => { - self.channel_monitor_updated(&funding_txo, &channel_id, monitor_update_id, counterparty_node_id.as_ref()); + self.channel_monitor_updated( + &funding_txo, + &channel_id, + Some(monitor_update_id), + counterparty_node_id.as_ref(), + ); }, } } @@ -10906,10 +10957,12 @@ where let peer_state = &mut *peer_state_lck; if let Some(blocker) = completed_blocker.take() { // Only do this on the first iteration of the loop. - if let Some(blockers) = peer_state.actions_blocking_raa_monitor_updates - .get_mut(&channel_id) - { - blockers.retain(|iter| iter != &blocker); + let entry = peer_state.actions_blocking_raa_monitor_updates.entry(channel_id); + if let btree_map::Entry::Occupied(mut entry) = entry { + entry.get_mut().retain(|iter| iter != &blocker); + if entry.get().is_empty() { + entry.remove(); + } } } @@ -13900,38 +13953,58 @@ where ($counterparty_node_id: expr, $chan_in_flight_upds: expr, $funding_txo: expr, $monitor: expr, $peer_state: expr, $logger: expr, $channel_info_log: expr ) => { { + // When all in-flight updates have completed after we were last serialized, we + // need to remove them. However, we can't guarantee that the next serialization + // will have happened after processing the + // `BackgroundEvent::MonitorUpdatesComplete`, so removing them now could lead to the + // channel never being resumed as the event would not be regenerated after another + // reload. At the same time, we don't want to resume the channel now because there + // may be post-update actions to handle. Therefore, we're forced to keep tracking + // the completed in-flight updates (but only when they have all completed) until we + // are processing the `BackgroundEvent::MonitorUpdatesComplete`. let mut max_in_flight_update_id = 0; - let starting_len = $chan_in_flight_upds.len(); - $chan_in_flight_upds.retain(|upd| upd.update_id > $monitor.get_latest_update_id()); - if $chan_in_flight_upds.len() < starting_len { + let num_updates_completed = $chan_in_flight_upds + .iter() + .filter(|update| { + max_in_flight_update_id = cmp::max(max_in_flight_update_id, update.update_id); + update.update_id <= $monitor.get_latest_update_id() + }) + .count(); + if num_updates_completed > 0 { log_debug!( $logger, "{} ChannelMonitorUpdates completed after ChannelManager was last serialized", - starting_len - $chan_in_flight_upds.len() + num_updates_completed, ); } - for update in $chan_in_flight_upds.iter() { - log_debug!($logger, "Replaying ChannelMonitorUpdate {} for {}channel {}", - update.update_id, $channel_info_log, &$monitor.channel_id()); - max_in_flight_update_id = cmp::max(max_in_flight_update_id, update.update_id); - pending_background_events.push( - BackgroundEvent::MonitorUpdateRegeneratedOnStartup { - counterparty_node_id: $counterparty_node_id, - funding_txo: $funding_txo, - channel_id: $monitor.channel_id(), - update: update.clone(), - }); - } - if $chan_in_flight_upds.is_empty() { - // We had some updates to apply, but it turns out they had completed before we - // were serialized, we just weren't notified of that. Thus, we may have to run - // the completion actions for any monitor updates, but otherwise are done. + let all_updates_completed = num_updates_completed == $chan_in_flight_upds.len(); + + if all_updates_completed { + log_debug!($logger, "All monitor updates completed since the ChannelManager was last serialized"); pending_background_events.push( BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id: $counterparty_node_id, + funding_txo: $funding_txo, channel_id: $monitor.channel_id(), + highest_update_id_completed: max_in_flight_update_id, }); } else { + $chan_in_flight_upds.retain(|update| { + let replay = update.update_id > $monitor.get_latest_update_id(); + if replay { + log_debug!($logger, "Replaying ChannelMonitorUpdate {} for {}channel {}", + update.update_id, $channel_info_log, &$monitor.channel_id()); + pending_background_events.push( + BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id: $counterparty_node_id, + funding_txo: $funding_txo, + channel_id: $monitor.channel_id(), + update: update.clone(), + } + ); + } + replay + }); $peer_state.closed_channel_monitor_update_ids.entry($monitor.channel_id()) .and_modify(|v| *v = cmp::max(max_in_flight_update_id, *v)) .or_insert(max_in_flight_update_id); @@ -13954,6 +14027,7 @@ where // Channels that were persisted have to be funded, otherwise they should have been // discarded. let funding_txo = chan.context.get_funding_txo().ok_or(DecodeError::InvalidValue)?; + let chan_id = chan.context.channel_id(); let monitor = args.channel_monitors.get(&funding_txo) .expect("We already checked for monitor presence when loading channels"); let mut max_in_flight_update_id = monitor.get_latest_update_id(); @@ -13976,6 +14050,14 @@ where log_error!(logger, " Please ensure the chain::Watch API requirements are met and file a bug report at https://github.com/lightningdevkit/rust-lightning"); return Err(DecodeError::DangerousValue); } + if chan.blocked_monitor_updates_pending() > 0 { + pending_background_events.push( + BackgroundEvent::AttemptUnblockMonitorUpdates { + counterparty_node_id: *counterparty_id, + channel_id: chan_id, + }, + ); + } } else { // We shouldn't have persisted (or read) any unfunded channel types so none should have been // created in this `channel_by_id` map. @@ -15321,7 +15403,7 @@ mod tests { let chan = create_announced_chan_between_nodes(&nodes, 0, 1); - nodes[0].node.force_close_channel_with_peer(&chan.2, &nodes[1].node.get_our_node_id(), None, true).unwrap(); + nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id(), "".to_string()).unwrap(); check_added_monitors!(nodes[0], 1); check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, [nodes[1].node.get_our_node_id()], 100000); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 41a5fb7cd4d..e5f7d918117 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -10281,7 +10281,7 @@ fn test_inconsistent_mpp_params() { do_claim_payment_along_route( ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], our_payment_preimage) ); - expect_payment_sent(&nodes[0], our_payment_preimage, Some(None), true, true); + expect_payment_sent(&nodes[0], our_payment_preimage, Some(Some(2000)), true, true); } #[test] @@ -12017,3 +12017,69 @@ pub fn do_test_dust_limit_fee_accounting(can_afford: bool) { check_added_monitors(&nodes[1], 2); } } + +#[test] +fn test_dup_htlc_claim_onchain_and_offchain() { + // Tests what happens if we receive a claim first offchain, then see a counterparty broadcast + // their commitment transaction and re-claim the same HTLC on-chain. This was never broken, but + // the very specific ordering in this test did hit a debug assertion failure. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let legacy_cfg = test_default_channel_config(); + let node_chanmgrs = create_node_chanmgrs( + 3, + &node_cfgs, + &[Some(legacy_cfg.clone()), Some(legacy_cfg.clone()), Some(legacy_cfg)], + ); + let nodes = create_network(3, &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(); + + create_announced_chan_between_nodes(&nodes, 0, 1); + let chan_bc = create_announced_chan_between_nodes(&nodes, 1, 2); + + // Route payment A -> B -> C. + let (payment_preimage, payment_hash, _, _) = + route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000); + + // C claims the payment. + nodes[2].node.claim_funds(payment_preimage); + expect_payment_claimed!(nodes[2], payment_hash, 1_000_000); + check_added_monitors(&nodes[2], 1); + + // Deliver only C's update_fulfill_htlc to B (NOT the commitment_signed). B learns + // the preimage and claims from A (adding an RAA blocker on B-C via + // internal_update_fulfill_htlc, then removing it when the A-B monitor update completes + // and the EmitEventOptionAndFreeOtherChannel action runs). + let cs_updates = get_htlc_update_msgs(&nodes[2], &node_b_id); + nodes[1].node.handle_update_fulfill_htlc(node_c_id, &cs_updates.update_fulfill_htlcs[0]); + check_added_monitors(&nodes[1], 1); + + // Ignore B's attempts to claim the HTLC from A. + nodes[1].node.get_and_clear_pending_msg_events(); + + // Get C's commitment transactions. C's commitment includes the HTLC and C has + // an HTLC-success transaction (claiming with preimage). Mine both on B. + let cs_txn = get_local_commitment_txn!(nodes[2], chan_bc.2); + assert!(cs_txn.len() >= 2, "Expected commitment + HTLC-success tx, got {}", cs_txn.len()); + + // Mine C's commitment on B. B sees the counterparty commitment on-chain. + mine_transaction(&nodes[1], &cs_txn[0]); + check_closed_broadcast(&nodes[1], 1, true); + check_added_monitors(&nodes[1], 1); + let events = nodes[1].node.get_and_clear_pending_events(); + assert!( + events.iter().any(|e| matches!(e, Event::ChannelClosed { .. })), + "Expected ChannelClosed event" + ); + + // Mine C's HTLC-success transaction. B's monitor sees the preimage being used on-chain + // and generates an HTLCEvent with the preimage. + mine_transaction(&nodes[1], &cs_txn[1]); + + // Advance past ANTI_REORG_DELAY so the on-chain HTLC resolution matures. This triggers + // the monitor to generate an HTLCEvent with the preimage via process_pending_monitor_events, + // which calls claim_funds_internal a second time. + connect_blocks(&nodes[1], ANTI_REORG_DELAY); +} diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 48171d4faeb..58ebd67316b 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -109,38 +109,6 @@ fn disconnect_peers<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, peers: &[&Node<'a, 'b } } -fn announce_node_address<'a, 'b, 'c>( - node: &Node<'a, 'b, 'c>, peers: &[&Node<'a, 'b, 'c>], address: SocketAddress, -) { - let features = node.onion_messenger.provided_node_features() - | node.gossip_sync.provided_node_features(); - let rgb = [0u8; 3]; - let announcement = UnsignedNodeAnnouncement { - features, - timestamp: 1000, - node_id: NodeId::from_pubkey(&node.keys_manager.get_node_id(Recipient::Node).unwrap()), - rgb, - alias: NodeAlias([0u8; 32]), - addresses: vec![address], - excess_address_data: Vec::new(), - excess_data: Vec::new(), - }; - let signature = node.keys_manager.sign_gossip_message( - UnsignedGossipMessage::NodeAnnouncement(&announcement) - ).unwrap(); - - let msg = NodeAnnouncement { - signature, - contents: announcement - }; - - let node_pubkey = node.node.get_our_node_id(); - node.gossip_sync.handle_node_announcement(None, &msg).unwrap(); - for peer in peers { - peer.gossip_sync.handle_node_announcement(Some(node_pubkey), &msg).unwrap(); - } -} - fn resolve_introduction_node<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, path: &BlindedMessagePath) -> PublicKey { path.public_introduction_node_id(&node.network_graph.read_only()) .and_then(|node_id| node_id.as_pubkey().ok()) @@ -254,126 +222,6 @@ fn extract_invoice_error<'a, 'b, 'c>( } } -/// Checks that blinded paths without Tor-only nodes are preferred when constructing an offer. -#[test] -fn prefers_non_tor_nodes_in_blinded_paths() { - let mut accept_forward_cfg = test_default_channel_config(); - accept_forward_cfg.accept_forwards_to_priv_channels = true; - - let mut features = channelmanager::provided_init_features(&accept_forward_cfg); - features.set_onion_messages_optional(); - features.set_route_blinding_optional(); - - let chanmon_cfgs = create_chanmon_cfgs(6); - let node_cfgs = create_node_cfgs(6, &chanmon_cfgs); - - *node_cfgs[1].override_init_features.borrow_mut() = Some(features); - - let node_chanmgrs = create_node_chanmgrs( - 6, &node_cfgs, &[None, Some(accept_forward_cfg), None, None, None, None] - ); - let nodes = create_network(6, &node_cfgs, &node_chanmgrs); - - create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000); - create_unannounced_chan_between_nodes_with_value(&nodes, 2, 3, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 5, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 5, 10_000_000, 1_000_000_000); - - // Add an extra channel so that more than one of Bob's peers have MIN_PEER_CHANNELS. - create_announced_chan_between_nodes_with_value(&nodes, 4, 5, 10_000_000, 1_000_000_000); - - let (alice, bob, charlie, david) = (&nodes[0], &nodes[1], &nodes[2], &nodes[3]); - let bob_id = bob.node.get_our_node_id(); - let charlie_id = charlie.node.get_our_node_id(); - - disconnect_peers(alice, &[charlie, david, &nodes[4], &nodes[5]]); - disconnect_peers(david, &[bob, &nodes[4], &nodes[5]]); - - let tor = SocketAddress::OnionV2([255, 254, 253, 252, 251, 250, 249, 248, 247, 246, 38, 7]); - announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor.clone()); - - let offer = bob.node - .create_offer_builder(None).unwrap() - .amount_msats(10_000_000) - .build().unwrap(); - assert_ne!(offer.issuer_signing_pubkey(), Some(bob_id)); - assert!(!offer.paths().is_empty()); - for path in offer.paths() { - let introduction_node_id = resolve_introduction_node(david, &path); - assert_ne!(introduction_node_id, bob_id); - assert_ne!(introduction_node_id, charlie_id); - } - - // Use a one-hop blinded path when Bob is announced and all his peers are Tor-only. - announce_node_address(&nodes[4], &[alice, bob, charlie, david, &nodes[5]], tor.clone()); - announce_node_address(&nodes[5], &[alice, bob, charlie, david, &nodes[4]], tor.clone()); - - let offer = bob.node - .create_offer_builder(None).unwrap() - .amount_msats(10_000_000) - .build().unwrap(); - assert_ne!(offer.issuer_signing_pubkey(), Some(bob_id)); - assert!(!offer.paths().is_empty()); - for path in offer.paths() { - let introduction_node_id = resolve_introduction_node(david, &path); - assert_eq!(introduction_node_id, bob_id); - } -} - -/// Checks that blinded paths prefer an introduction node that is the most connected. -#[test] -fn prefers_more_connected_nodes_in_blinded_paths() { - let mut accept_forward_cfg = test_default_channel_config(); - accept_forward_cfg.accept_forwards_to_priv_channels = true; - - let mut features = channelmanager::provided_init_features(&accept_forward_cfg); - features.set_onion_messages_optional(); - features.set_route_blinding_optional(); - - let chanmon_cfgs = create_chanmon_cfgs(6); - let node_cfgs = create_node_cfgs(6, &chanmon_cfgs); - - *node_cfgs[1].override_init_features.borrow_mut() = Some(features); - - let node_chanmgrs = create_node_chanmgrs( - 6, &node_cfgs, &[None, Some(accept_forward_cfg), None, None, None, None] - ); - let nodes = create_network(6, &node_cfgs, &node_chanmgrs); - - create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000); - create_unannounced_chan_between_nodes_with_value(&nodes, 2, 3, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 5, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 5, 10_000_000, 1_000_000_000); - - // Add extra channels so that more than one of Bob's peers have MIN_PEER_CHANNELS and one has - // more than the others. - create_announced_chan_between_nodes_with_value(&nodes, 0, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 3, 4, 10_000_000, 1_000_000_000); - - let (alice, bob, charlie, david) = (&nodes[0], &nodes[1], &nodes[2], &nodes[3]); - let bob_id = bob.node.get_our_node_id(); - - disconnect_peers(alice, &[charlie, david, &nodes[4], &nodes[5]]); - disconnect_peers(david, &[bob, &nodes[4], &nodes[5]]); - - let offer = bob.node - .create_offer_builder(None).unwrap() - .amount_msats(10_000_000) - .build().unwrap(); - assert_ne!(offer.issuer_signing_pubkey(), Some(bob_id)); - assert!(!offer.paths().is_empty()); - for path in offer.paths() { - let introduction_node_id = resolve_introduction_node(david, &path); - assert_eq!(introduction_node_id, nodes[4].node.get_our_node_id()); - } -} - /// Checks that blinded paths are compact for short-lived offers. #[test] fn creates_short_lived_offer() { diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index b3a8bd9ffee..06ebee98575 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -133,6 +133,9 @@ pub(crate) enum PendingOutboundPayment { /// Will be `None` if the payment was serialized before 0.0.115 or if downgrading to 0.0.124 /// or later with a reason that was added after. reason: Option, + /// Preserved from `Retryable` so we can still report `fee_paid_msat` if an HTLC succeeds after + /// the payment was abandoned. Added in 0.3/0.1.10. + pending_fee_msat: Option, }, } @@ -209,6 +212,7 @@ impl PendingOutboundPayment { fn get_pending_fee_msat(&self) -> Option { match self { PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(), + PendingOutboundPayment::Abandoned { pending_fee_msat, .. } => pending_fee_msat.clone(), _ => None, } } @@ -251,6 +255,7 @@ impl PendingOutboundPayment { }, _ => new_hash_set(), }; + let pending_fee_msat = self.get_pending_fee_msat(); match self { Self::Retryable { payment_hash, .. } | Self::InvoiceReceived { payment_hash, .. } | @@ -260,6 +265,7 @@ impl PendingOutboundPayment { session_privs, payment_hash: *payment_hash, reason: Some(reason), + pending_fee_msat, }; }, _ => {} @@ -2409,6 +2415,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, (0, session_privs, required), (1, reason, upgradable_option), (2, payment_hash, required), + (5, pending_fee_msat, option), }, (5, AwaitingInvoice) => { (0, expiration, required), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 47c12c358eb..0873418c86d 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -290,7 +290,7 @@ fn mpp_retry_overpay() { expect_payment_sent!(&nodes[0], payment_preimage, Some(expected_total_fee_msat)); } -fn do_mpp_receive_timeout(send_partial_mpp: bool) { +fn do_mpp_receive_timeout(send_partial_mpp: bool, keysend: bool) { 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]); @@ -301,7 +301,12 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { let (chan_3_update, _, chan_3_id, _) = create_announced_chan_between_nodes(&nodes, 1, 3); let (chan_4_update, _, _, _) = create_announced_chan_between_nodes(&nodes, 2, 3); - let (mut route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[3], 100_000); + let (mut route, payment_hash, payment_preimage, payment_secret) = if keysend { + let payment_params = PaymentParameters::for_keysend(nodes[3].node.get_our_node_id(), TEST_FINAL_CLTV, true); + get_route_and_payment_hash!(nodes[0], nodes[3], payment_params, 100_000) + } else { + get_route_and_payment_hash!(nodes[0], nodes[3], 100_000) + }; let path = route.paths[0].clone(); route.paths.push(path); route.paths[0].hops[0].pubkey = nodes[1].node.get_our_node_id(); @@ -312,9 +317,24 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { route.paths[1].hops[1].short_channel_id = chan_4_update.contents.short_channel_id; // Initiate the MPP payment. - nodes[0].node.send_payment_with_route(route, payment_hash, - RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); - check_added_monitors!(nodes[0], 2); // one monitor per path + let onion = RecipientOnionFields::secret_only(payment_secret); + if keysend { + let route_params = route.route_params.clone().unwrap(); + nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); + nodes[0] + .node + .send_spontaneous_payment( + Some(payment_preimage), + onion, + PaymentId(payment_hash.0), + route_params, + Retry::Attempts(0), + ) + .unwrap(); + } else { + nodes[0].node.send_payment_with_route(route, payment_hash, onion, PaymentId(payment_hash.0)).unwrap(); + } + check_added_monitors(&nodes[0], 2); // one monitor per path let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 2); @@ -348,7 +368,19 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { } else { // Pass half of the payment along the second path. let node_2_msgs = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events); - pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 200_000, payment_hash, Some(payment_secret), node_2_msgs, true, None); + let path = &[&nodes[2], &nodes[3]]; + let payment_secret = Some(payment_secret); + let expected_preimage = if keysend { Some(payment_preimage) } else { None }; + pass_along_path( + &nodes[0], + path, + 200_000, + payment_hash, + payment_secret, + node_2_msgs, + true, + expected_preimage, + ); // Even after MPP_TIMEOUT_TICKS we should not timeout the MPP if we have all the parts for _ in 0..MPP_TIMEOUT_TICKS { @@ -363,8 +395,14 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { #[test] fn mpp_receive_timeout() { - do_mpp_receive_timeout(true); - do_mpp_receive_timeout(false); + do_mpp_receive_timeout(true, false); + do_mpp_receive_timeout(false, false); +} + +#[test] +fn keysend_mpp_receive_timeout() { + do_mpp_receive_timeout(true, true); + do_mpp_receive_timeout(false, true); } #[test] @@ -1713,6 +1751,36 @@ fn abandoned_send_payment_idempotent() { claim_payment(&nodes[0], &[&nodes[1]], second_payment_preimage); } +#[test] +fn abandoned_payment_fulfilled_preserves_fee_paid_msat() { + // Previously, if we abandoned a payment with HTLCs in-flight and the payment eventually + // succeeded, we would set the `Event::PaymentSent::fee_paid_msat` to None, even though we had + // docs guaranteeing that it would always be Some after 0.0.103. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let 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 amt_msat = 5_000_000; + let (route, payment_hash, payment_preimage, payment_secret) = + get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat); + let payment_id = PaymentId(payment_hash.0); + let onion = RecipientOnionFields::secret_only(payment_secret); + nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap(); + check_added_monitors(&nodes[0], 1); + + let path: &[&Node] = &[&nodes[1], &nodes[2]]; + pass_along_route(&nodes[0], &[path], amt_msat, payment_hash, payment_secret); + + nodes[0].node.abandon_payment(payment_id); + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + + claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], payment_preimage)); +} + #[derive(PartialEq)] enum InterceptTest { Forward, diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 16904d85758..d65be70e99f 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -984,6 +984,359 @@ fn test_partial_claim_before_restart() { do_test_partial_claim_before_restart(true, true); } +#[test] +fn test_mpp_claim_htlc_fulfills_unblocked_on_reload() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let persister; + let new_chain_monitor; + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes_1_deserialized; + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Open two independent channels between the same nodes. The payment below is large enough to + // force the router to split it across both channels, which is what makes the MPP claim depend + // on both ChannelMonitors durably learning the preimage. + let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); + let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); + let chan_id_a = chan_a.2; + let chan_id_b = chan_b.2; + let scid_a = chan_a.0.contents.short_channel_id; + let scid_b = chan_b.0.contents.short_channel_id; + + // Send an MPP payment to nodes[1]. `send_along_route_with_secret` leaves the payment + // claimable but unclaimed, so nodes[1] still has both inbound HTLCs live when we start + // manipulating monitor persistence below. + let amt_msat = 20_000_000; + let (route, payment_hash, payment_preimage, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], amt_msat); + assert_eq!(route.paths.len(), 2); + send_along_route_with_secret( + &nodes[0], route, &[&[&nodes[1]], &[&nodes[1]]], amt_msat, payment_hash, + payment_secret, + ); + + // Move both channels into `AWAITING_REMOTE_REVOKE` by having nodes[0] send fee updates and + // withholding nodes[1]'s responding `commitment_signed`s. When nodes[1] later claims the + // payment, the fulfill updates cannot be sent immediately and instead sit in each channel's + // holding cell. + { + let mut fee_est = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap(); + *fee_est *= 2; + } + nodes[0].node.timer_tick_occurred(); + check_added_monitors(&nodes[0], 2); + + let node_0_id = nodes[0].node.get_our_node_id(); + let node_1_id = nodes[1].node.get_our_node_id(); + + let fee_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(fee_msgs.len(), 2); + for ev in &fee_msgs { + match ev { + MessageSendEvent::UpdateHTLCs { updates, .. } => { + nodes[1].node.handle_update_fee(node_0_id, updates.update_fee.as_ref().unwrap()); + nodes[1].node.handle_commitment_signed(node_0_id, &updates.commitment_signed); + check_added_monitors(&nodes[1], 1); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + // nodes[1] responds to each fee update with a `revoke_and_ack` and a new + // `commitment_signed`. Deliver only the `revoke_and_ack`s for now. The held + // `commitment_signed`s are delivered after nodes[1] claims the payment, creating the blocked + // post-claim monitor updates whose release is exercised after reload. + let node_1_msgs = nodes[1].node.get_and_clear_pending_msg_events(); + let mut commitment_signed_msgs = Vec::new(); + for ev in &node_1_msgs { + match ev { + MessageSendEvent::SendRevokeAndACK { msg, .. } => { + nodes[0].node.handle_revoke_and_ack(node_1_id, msg); + check_added_monitors(&nodes[0], 1); + }, + MessageSendEvent::UpdateHTLCs { updates, .. } => { + commitment_signed_msgs.push(updates.commitment_signed.clone()); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + let node_0_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + for ev in &node_0_msgs { + match ev { + MessageSendEvent::SendRevokeAndACK { msg, .. } => { + nodes[1].node.handle_revoke_and_ack(node_0_id, msg); + check_added_monitors(&nodes[1], 1); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + // Snapshot channel B before the claim. The in-memory ChainMonitor applies updates even when + // the persister returns `InProgress`, so taking this snapshot after the claim would not model a + // crash between two separate monitor writes. + let mon_b_serialized = get_monitor!(nodes[1], chan_id_b).encode(); + + // Make both preimage monitor writes asynchronous. `claim_funds` attaches an in-memory MPP RAA + // blocker so neither channel can release later monitor updates until all channels have the + // preimage durably persisted. + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + nodes[1].node.claim_funds(payment_preimage); + check_added_monitors(&nodes[1], 2); + + // Complete only channel A's preimage update. Channel B will be reloaded from the stale snapshot + // above, simulating a crash where one monitor write completed and the other did not. + let (outpoint_a, update_id_a, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_a).unwrap().clone(); + nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint_a, update_id_a); + + // Now finish the fee-update commitment dance we held back. nodes[1] receives nodes[0]'s + // `revoke_and_ack`s while the MPP RAA blocker is still in place, so the resulting monitor + // updates are blocked behind state that is not serialized in the ChannelManager. + for commitment_signed in &commitment_signed_msgs { + nodes[0].node.handle_commitment_signed(node_1_id, commitment_signed); + check_added_monitors(&nodes[0], 1); + } + let node_0_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + for ev in &node_0_msgs { + match ev { + MessageSendEvent::SendRevokeAndACK { msg, .. } => { + nodes[1].node.handle_revoke_and_ack(node_0_id, msg); + check_added_monitors(&nodes[1], 0); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + // Persist the ChannelManager after the blocked post-claim monitor updates have been recorded. + // Reload with channel A's up-to-date monitor and channel B's stale monitor. The preimage update + // for B is replayed during reload, putting both channels' preimages on disk. The remaining state + // under test is the blocked post-claim `revoke_and_ack` monitor updates after the in-memory MPP + // RAA blocker that created them is gone. + let node_1_serialized = nodes[1].node.encode(); + let mon_a_serialized = get_monitor!(nodes[1], chan_id_a).encode(); + + nodes[0].node.peer_disconnected(node_1_id); + reload_node!( + nodes[1], + node_1_serialized, + &[&mon_a_serialized, &mon_b_serialized], + persister, + new_chain_monitor, + nodes_1_deserialized + ); + + // Reconnect both peers by manually exchanging `channel_reestablish`s. This avoids relying on a + // more general reconnect helper while the channels intentionally have asymmetric monitor state. + let node_1_id = nodes[1].node.get_our_node_id(); + nodes[0].node.peer_connected(node_1_id, &msgs::Init { + features: nodes[1].node.init_features(), networks: None, remote_network_address: None, + }, true).unwrap(); + nodes[1].node.peer_connected(node_0_id, &msgs::Init { + features: nodes[0].node.init_features(), networks: None, remote_network_address: None, + }, false).unwrap(); + + let reestablish_0 = nodes[0].node.get_and_clear_pending_msg_events(); + let reestablish_1 = nodes[1].node.get_and_clear_pending_msg_events(); + let mut reestablish_0_chan_ids = Vec::new(); + let mut reestablish_1_chan_ids = Vec::new(); + for ev in &reestablish_1 { + match ev { + MessageSendEvent::SendChannelReestablish { node_id, msg } => { + assert_eq!(*node_id, node_0_id); + reestablish_1_chan_ids.push(msg.channel_id); + nodes[0].node.handle_channel_reestablish(node_1_id, msg); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + for ev in &reestablish_0 { + match ev { + MessageSendEvent::SendChannelReestablish { node_id, msg } => { + assert_eq!(*node_id, node_1_id); + reestablish_0_chan_ids.push(msg.channel_id); + nodes[1].node.handle_channel_reestablish(node_0_id, msg); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + assert_eq!(reestablish_0_chan_ids.len(), 2); + assert!(reestablish_0_chan_ids.contains(&chan_id_a)); + assert!(reestablish_0_chan_ids.contains(&chan_id_b)); + assert_eq!(reestablish_1_chan_ids.len(), 2); + assert!(reestablish_1_chan_ids.contains(&chan_id_a)); + assert!(reestablish_1_chan_ids.contains(&chan_id_b)); + // Only nodes[1] was reloaded with stale monitor state. nodes[0] responds to the + // `channel_reestablish`s without touching its monitors. nodes[1] applies the replayed channel B + // preimage update, releases channel A's held RAA update, and frees channel A's held fulfill + // during startup processing. + // Note that unlike the test in 0.3, we only generate the last monitor update for node B after + // get_and_clear_pending_msg_events as we only free the holding cell then. + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 2); + + // The first message batch after reconnect contains channel updates from both nodes. nodes[1] + // also sends the channel A fulfill that startup processing released from the holding cell. + let restart_msgs_0 = nodes[0].node.get_and_clear_pending_msg_events(); + let restart_msgs_1 = nodes[1].node.get_and_clear_pending_msg_events(); + check_added_monitors(&nodes[1], 1); + let mut restart_scids_0 = Vec::new(); + let mut restart_scids_1 = Vec::new(); + let mut startup_fulfill_chan_ids = Vec::new(); + for ev in &restart_msgs_0 { + match ev { + MessageSendEvent::SendChannelUpdate { node_id, msg } => { + assert_eq!(*node_id, node_1_id); + restart_scids_0.push(msg.contents.short_channel_id); + }, + _ => panic!("Unexpected restart message from node 0: {:?}", ev), + } + } + for ev in &restart_msgs_1 { + match ev { + MessageSendEvent::SendChannelUpdate { node_id, msg } => { + assert_eq!(*node_id, node_0_id); + restart_scids_1.push(msg.contents.short_channel_id); + }, + MessageSendEvent::UpdateHTLCs { node_id, updates } => { + assert_eq!(*node_id, node_0_id); + startup_fulfill_chan_ids.push(updates.commitment_signed.channel_id); + assert_eq!(updates.update_fulfill_htlcs.len(), 1); + assert!(updates.update_add_htlcs.is_empty()); + assert!(updates.update_fail_htlcs.is_empty()); + assert!(updates.update_fail_malformed_htlcs.is_empty()); + assert!(updates.update_fee.is_none()); + for fulfill in &updates.update_fulfill_htlcs { + nodes[0].node.handle_update_fulfill_htlc(node_1_id, fulfill); + } + // Complete the standard commitment handshake for the released fulfill. The helper + // checks nodes[0]'s incoming commitment monitor update, nodes[1]'s response monitor + // updates, and nodes[0]'s held final monitor update. + do_commitment_signed_dance( + &nodes[0], &nodes[1], &updates.commitment_signed, false, false, + ); + }, + _ => panic!("Unexpected restart message from node 1: {:?}", ev), + } + } + assert_eq!(restart_scids_0.len(), 2); + assert!(restart_scids_0.contains(&scid_a)); + assert!(restart_scids_0.contains(&scid_b)); + assert_eq!(restart_scids_1.len(), 2); + assert!(restart_scids_1.contains(&scid_a)); + assert!(restart_scids_1.contains(&scid_b)); + assert_eq!(startup_fulfill_chan_ids, vec![chan_id_a]); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 0); + + // Receiving the startup-released fulfill gives nodes[0] the payment preimage. That is enough to + // emit `PaymentSent`, even though channel B's path-level success still needs its own fulfill. + let startup_payment_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(startup_payment_events.len(), 2); + let mut saw_startup_payment_sent = false; + let mut startup_success_scids = Vec::new(); + for ev in &startup_payment_events { + match ev { + Event::PaymentSent { + payment_preimage: sent_preimage, + payment_hash: sent_hash, + fee_paid_msat, + .. + } => { + assert_eq!(*sent_preimage, payment_preimage); + assert_eq!(*sent_hash, payment_hash); + assert_eq!(*fee_paid_msat, Some(0)); + saw_startup_payment_sent = true; + }, + Event::PaymentPathSuccessful { payment_hash: Some(path_hash), path, .. } => { + assert_eq!(*path_hash, payment_hash); + assert_eq!(path.hops.len(), 1); + startup_success_scids.push(path.hops[0].short_channel_id); + }, + _ => panic!("Unexpected startup payment event: {:?}", ev), + } + } + assert!(saw_startup_payment_sent); + assert_eq!(startup_success_scids, vec![scid_a]); + + // Handling the claim event runs the event-completion action that releases the remaining + // RAA-blocked monitor update. The startup unblock path already released channel A, so channel B + // is the only fulfill that should be emitted here. + let claim_events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(claim_events.len(), 1); + match &claim_events[0] { + Event::PaymentClaimed { payment_hash: claimed_hash, amount_msat, htlcs, .. } => { + assert_eq!(*claimed_hash, payment_hash); + assert_eq!(*amount_msat, amt_msat); + assert_eq!(htlcs.len(), 2); + }, + _ => panic!("Unexpected event: {:?}", claim_events[0]), + } + // The `PaymentSent` event above releases the monitor update that nodes[0] held after the final + // channel A startup revocation. + check_added_monitors(&nodes[0], 1); + // Handling `PaymentClaimed` releases channel B's held revocation update and then the fulfill + // that was waiting behind it (unlike this test in 0.3, after we free the holding cell in + // get_and_clear_pending_msg_events below). + check_added_monitors(&nodes[1], 1); + + // Channel A's fulfill was already sent during startup. The `PaymentClaimed` completion action + // now frees channel B's held fulfill, and no other HTLC update should be bundled with it. + let fulfill_msgs = nodes[1].node.get_and_clear_pending_msg_events(); + check_added_monitors(&nodes[1], 1); + assert_eq!(fulfill_msgs.len(), 1); + match &fulfill_msgs[0] { + MessageSendEvent::UpdateHTLCs { node_id, updates } => { + assert_eq!(*node_id, node_0_id); + assert_eq!(updates.commitment_signed.channel_id, chan_id_b); + assert_eq!(updates.update_fulfill_htlcs.len(), 1); + assert!(updates.update_add_htlcs.is_empty()); + assert!(updates.update_fail_htlcs.is_empty()); + assert!(updates.update_fail_malformed_htlcs.is_empty()); + assert!(updates.update_fee.is_none()); + for fulfill in &updates.update_fulfill_htlcs { + nodes[0].node.handle_update_fulfill_htlc(node_1_id, fulfill); + } + // Complete the same commitment handshake for channel B. Here nodes[0]'s final monitor + // update is persisted immediately because `PaymentSent` already ran for channel A. + do_commitment_signed_dance( + &nodes[0], &nodes[1], &updates.commitment_signed, false, false, + ); + }, + _ => panic!("Unexpected fulfill message: {:?}", fulfill_msgs[0]), + } + check_added_monitors(&nodes[1], 0); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + let final_payment_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(final_payment_events.len(), 1); + match &final_payment_events[0] { + Event::PaymentPathSuccessful { payment_hash: Some(path_hash), path, .. } => { + assert_eq!(*path_hash, payment_hash); + assert_eq!(path.hops.len(), 1); + assert_eq!(path.hops[0].short_channel_id, scid_b); + }, + _ => panic!("Unexpected final payment event: {:?}", final_payment_events[0]), + } + check_added_monitors(&nodes[0], 0); + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 0); + + // Both MPP parts should have been fulfilled back to nodes[0]. If either channel still has a + // pending outbound HTLC, its fulfill remained stuck in nodes[1]'s holding cell after reload. + let pending: Vec<_> = nodes[0].node.list_channels().iter() + .filter(|channel| channel.channel_id == chan_id_a || channel.channel_id == chan_id_b) + .filter(|channel| !channel.pending_outbound_htlcs.is_empty()) + .map(|channel| channel.channel_id) + .collect(); + assert!(pending.is_empty(), "HTLC fulfills remained stuck on channels {:?}", pending); +} + fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool, use_intercept: bool) { if !use_cs_commitment { assert!(!claim_htlc); } // If we go to forward a payment, and the ChannelMonitor persistence completes, but the @@ -1291,3 +1644,85 @@ fn test_reload_partial_funding_batch() { // Ensure the channels don't exist anymore. assert!(nodes[0].node.list_channels().is_empty()); } + +#[test] +fn test_hold_completed_inflight_monitor_updates_upon_manager_reload() { + // Test that if a `ChannelMonitorUpdate` completes after the `ChannelManager` is serialized, + // but before it is deserialized, we hold any completed in-flight updates until background event + // processing. Previously, we would remove completed monitor updates from + // `in_flight_monitor_updates` during deserialization, relying on + // [`ChannelManager::process_background_events`] to eventually be called before the + // `ChannelManager` is serialized again such that the channel is resumed and further updates can + // be made. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let (persister_a, persister_b); + let (chain_monitor_a, chain_monitor_b); + + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes_0_deserialized_a; + let nodes_0_deserialized_b; + + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2; + + send_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + + // Send a payment that will be pending due to an async monitor update. + let (route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], 1_000_000); + let payment_id = PaymentId(payment_hash.0); + let onion = RecipientOnionFields::secret_only(payment_secret); + nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap(); + check_added_monitors(&nodes[0], 1); + + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // Serialize the ChannelManager while the monitor update is still in-flight. + let node_0_serialized = nodes[0].node.encode(); + + // Now complete the monitor update by calling force_channel_monitor_updated. + // This updates the monitor's state, but the ChannelManager still thinks it's pending. + let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone(); + nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update); + let monitor_serialized_updated = get_monitor!(nodes[0], chan_id).encode(); + + // Reload the node with the updated monitor. Upon deserialization, the ChannelManager will + // detect that the monitor update completed (monitor's update_id >= the in-flight update_id) + // and queue a `BackgroundEvent::MonitorUpdatesComplete`. + nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); + nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); + reload_node!( + nodes[0], + test_default_channel_config(), + &node_0_serialized, + &[&monitor_serialized_updated[..]], + persister_a, + chain_monitor_a, + nodes_0_deserialized_a + ); + + // If we serialize again, even though we haven't processed any background events yet, we should + // still see the `BackgroundEvent::MonitorUpdatesComplete` be regenerated on startup. + let node_0_serialized = nodes[0].node.encode(); + reload_node!( + nodes[0], + test_default_channel_config(), + &node_0_serialized, + &[&monitor_serialized_updated[..]], + persister_b, + chain_monitor_b, + nodes_0_deserialized_b + ); + + // Reconnect the nodes. We should finally see the `update_add_htlc` go out, as the reconnection + // should first process `BackgroundEvent::MonitorUpdatesComplete, allowing the channel to be + // resumed. + let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); + reconnect_args.pending_htlc_adds = (0, 1); + reconnect_nodes(reconnect_args); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); +} diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs index c326cfca804..e1b7def8811 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -525,44 +525,38 @@ where // Limit the number of blinded paths that are computed. const MAX_PATHS: usize = 3; - // Ensure peers have at least three channels so that it is more difficult to infer the - // recipient's node_id. - const MIN_PEER_CHANNELS: usize = 3; - let network_graph = network_graph.deref().read_only(); let is_recipient_announced = network_graph.nodes().contains_key(&NodeId::from_pubkey(&recipient)); let has_one_peer = peers.len() == 1; - let mut peer_info = peers - // Limit to peers with announced channels unless the recipient is unannounced. - .filter_map(|peer| - network_graph - .node(&NodeId::from_pubkey(&peer.node_id)) - .filter(|info| - !is_recipient_announced || info.channels.len() >= MIN_PEER_CHANNELS - ) - .map(|info| (peer, info.is_tor_only(), info.channels.len())) - // Allow messages directly with the only peer when unannounced. - .or_else(|| (!is_recipient_announced && has_one_peer) - .then(|| (peer, false, 0)) - ) - ) - // Exclude Tor-only nodes when the recipient is announced. - .filter(|(_, is_tor_only, _)| !(*is_tor_only && is_recipient_announced)) - .collect::>(); - - // Prefer using non-Tor nodes with the most channels as the introduction node. - peer_info.sort_unstable_by(|(_, a_tor_only, a_channels), (_, b_tor_only, b_channels)| { - a_tor_only.cmp(b_tor_only).then(a_channels.cmp(b_channels).reverse()) - }); + let paths = if !is_recipient_announced { + let mut peer_info = peers + // Limit to peers with announced channels unless the recipient is unannounced. + .filter_map(|peer| + network_graph + .node(&NodeId::from_pubkey(&peer.node_id)) + .map(|info| (peer, info.is_tor_only(), info.channels.len())) + // Allow messages directly with the only peer + .or_else(|| has_one_peer.then(|| (peer, false, 0))) + ) + .collect::>(); + + // Prefer using non-Tor nodes with the most channels as the introduction node. + peer_info.sort_unstable_by(|(_, a_tor_only, a_channels), (_, b_tor_only, b_channels)| { + a_tor_only.cmp(b_tor_only).then(a_channels.cmp(b_channels).reverse()) + }); - let paths = peer_info.into_iter() - .map(|(peer, _, _)| { - BlindedMessagePath::new(&[peer], recipient, context.clone(), &**entropy_source, secp_ctx) - }) - .take(MAX_PATHS) - .collect::, _>>(); + peer_info + .into_iter() + .map(|(peer, _, _)| { + BlindedMessagePath::new(&[peer], recipient, context.clone(), &**entropy_source, secp_ctx) + }) + .take(MAX_PATHS) + .collect::, _>>() + } else { + Ok(vec![]) + }; let mut paths = match paths { Ok(paths) if !paths.is_empty() => Ok(paths),