From fe6366895a67da62e7be4b295948a8fba43ce5e0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 8 Aug 2023 04:15:20 +0000 Subject: [PATCH 1/2] Scope payment preimage in do_test_keysend_payments b0d4ab8cf8c93740674a00546be38a1a5f0a83c3 fixed a nasty bug where we'd failed to include the payment preimage in keysend onions at all. Ultimately, this was a test failure - the existing test suite should which did keysend payments were not structured in a way that would fail in this case, instead using the same preimage variable both for sending and receiving. Here we improve the main keysend test tweaked by b0d4ab8cf8c9374067 to make absolutely sure it cannot work if the preimage doesn't come from the onion. We make the payment preimage on the sending side a variable inside a scope which only exists for the send call. Once that scope completes the payment preimage only exists in the sending `ChannelManager`, which must have put it in the onion in order for the receiving node to have it. --- lightning/src/ln/payment_tests.rs | 41 +++++++++++++++++++------------ 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 7d639611df2..6326796c783 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -15,7 +15,7 @@ use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch}; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS}; use crate::sign::EntropySource; use crate::chain::transaction::OutPoint; -use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason}; +use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose}; use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS; use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, RecentPaymentDetails, RecipientOnionFields, HTLCForwardInfo, PendingHTLCRouting, PendingAddHTLCInfo}; use crate::ln::features::Bolt11InvoiceFeatures; @@ -274,22 +274,31 @@ fn do_test_keysend_payments(public_node: bool, with_retry: bool) { nodes[0].logger, &scorer, &(), &random_seed_bytes ).unwrap(); - let test_preimage = PaymentPreimage([42; 32]); - let payment_hash = if with_retry { - nodes[0].node.send_spontaneous_payment_with_retry(Some(test_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0), - route_params, Retry::Attempts(1)).unwrap() - } else { - nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0)).unwrap() - }; + { + let test_preimage = PaymentPreimage([42; 32]); + if with_retry { + nodes[0].node.send_spontaneous_payment_with_retry(Some(test_preimage), + RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0), + route_params, Retry::Attempts(1)).unwrap() + } else { + nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage), + RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0)).unwrap() + }; + } check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - let event = events.pop().unwrap(); - let path = vec![&nodes[1]]; - pass_along_path(&nodes[0], &path, 10000, payment_hash, None, event, true, Some(test_preimage)); - claim_payment(&nodes[0], &path, test_preimage); + let send_event = SendEvent::from_node(&nodes[0]); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]); + do_commitment_signed_dance(&nodes[1], &nodes[0], &send_event.commitment_msg, false, false); + expect_pending_htlcs_forwardable!(nodes[1]); + // Previously, a refactor caused us to stop including the payment preimage in the onion which + // is sent as a part of keysend payments. Thus, to be extra careful here, we scope the preimage + // above to demonstrate that we have no way to get the preimage at this point except by + // extracting it from the onion nodes[1] received. + let event = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(event.len(), 1); + if let Event::PaymentClaimable { purpose: PaymentPurpose::SpontaneousPayment(preimage), .. } = event[0] { + claim_payment(&nodes[0], &[&nodes[1]], preimage); + } else { panic!(); } } #[test] From 67e5399eff1fcdcdc889ff70603ff2598f386677 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 8 Aug 2023 04:19:51 +0000 Subject: [PATCH 2/2] Make CI build -> test flows test -> build. Doing `cargo test` causes us to build both the crate(s) themselves and the test binaries, which depend on the main builds. However, it can start building the test code while the actual program code for the main crate(s) themselves are being built, making a build -> test flow slightly slower than test -> build. Its not really a huge deal, but I'm using `ci/ci-tests.sh` more locally and it meaningfully changes the time-to-test-run. --- ci/ci-tests.sh | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ci/ci-tests.sh b/ci/ci-tests.sh index 5b46b2da079..ef9ecf7b86d 100755 --- a/ci/ci-tests.sh +++ b/ci/ci-tests.sh @@ -37,30 +37,30 @@ PIN_RELEASE_DEPS # pin the release dependencies in our main workspace export RUST_BACKTRACE=1 echo -e "\n\nBuilding and testing all workspace crates..." -cargo build --verbose --color always cargo test --verbose --color always +cargo build --verbose --color always echo -e "\n\nBuilding and testing Block Sync Clients with features" pushd lightning-block-sync -cargo build --verbose --color always --features rest-client cargo test --verbose --color always --features rest-client -cargo build --verbose --color always --features rpc-client +cargo build --verbose --color always --features rest-client cargo test --verbose --color always --features rpc-client -cargo build --verbose --color always --features rpc-client,rest-client +cargo build --verbose --color always --features rpc-client cargo test --verbose --color always --features rpc-client,rest-client -cargo build --verbose --color always --features rpc-client,rest-client,tokio +cargo build --verbose --color always --features rpc-client,rest-client cargo test --verbose --color always --features rpc-client,rest-client,tokio +cargo build --verbose --color always --features rpc-client,rest-client,tokio popd if [[ $RUSTC_MINOR_VERSION -gt 67 && "$HOST_PLATFORM" != *windows* ]]; then echo -e "\n\nBuilding and testing Transaction Sync Clients with features" pushd lightning-transaction-sync - cargo build --verbose --color always --features esplora-blocking cargo test --verbose --color always --features esplora-blocking - cargo build --verbose --color always --features esplora-async + cargo build --verbose --color always --features esplora-blocking cargo test --verbose --color always --features esplora-async - cargo build --verbose --color always --features esplora-async-https + cargo build --verbose --color always --features esplora-async cargo test --verbose --color always --features esplora-async-https + cargo build --verbose --color always --features esplora-async-https popd fi