From 1e119a2d6a7d798a231a042c055e2d7e5e0fb73d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 18 Jul 2023 19:04:56 +0000 Subject: [PATCH 1/5] Tweak generics on `derive_channel_signer` to be bindings-compatible The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later. --- lightning/src/events/bump_transaction.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index bf56a84ca46..e56035b562b 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -26,7 +26,7 @@ use crate::ln::chan_utils::{ use crate::ln::features::ChannelTypeFeatures; use crate::ln::PaymentPreimage; use crate::prelude::*; -use crate::sign::{ChannelSigner, EcdsaChannelSigner, SignerProvider}; +use crate::sign::{ChannelSigner, EcdsaChannelSigner, SignerProvider, WriteableEcdsaChannelSigner}; use crate::sync::Mutex; use crate::util::logger::Logger; @@ -102,9 +102,9 @@ impl AnchorDescriptor { } /// Derives the channel signer required to sign the anchor input. - pub fn derive_channel_signer(&self, signer_provider: &SP) -> ::Signer + pub fn derive_channel_signer(&self, signer_provider: &SP) -> S where - SP::Target: SignerProvider + SP::Target: SignerProvider { let mut signer = signer_provider.derive_channel_signer( self.channel_derivation_parameters.value_satoshis, @@ -211,9 +211,9 @@ impl HTLCDescriptor { } /// Derives the channel signer required to sign the HTLC input. - pub fn derive_channel_signer(&self, signer_provider: &SP) -> ::Signer + pub fn derive_channel_signer(&self, signer_provider: &SP) -> S where - SP::Target: SignerProvider + SP::Target: SignerProvider { let mut signer = signer_provider.derive_channel_signer( self.channel_derivation_parameters.value_satoshis, From be08b4f6b8ff3248710852f17760647f5b63ee24 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 18 Jul 2023 19:13:19 +0000 Subject: [PATCH 2/5] Tweak transaction bumping `sign_tx` types for bindings In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users. --- lightning/src/events/bump_transaction.rs | 12 +++++++----- lightning/src/util/test_utils.rs | 16 ++++++++-------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index e56035b562b..a9133f50ed8 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -469,7 +469,7 @@ pub trait CoinSelectionSource { ) -> Result; /// Signs and provides the full witness for all inputs within the transaction known to the /// trait (i.e., any provided via [`CoinSelectionSource::select_confirmed_utxos`]). - fn sign_tx(&self, tx: &mut Transaction) -> Result<(), ()>; + fn sign_tx(&self, tx: Transaction) -> Result; } /// An alternative to [`CoinSelectionSource`] that can be implemented and used along [`Wallet`] to @@ -483,7 +483,7 @@ pub trait WalletSource { /// Signs and provides the full [`TxIn::script_sig`] and [`TxIn::witness`] for all inputs within /// the transaction known to the wallet (i.e., any provided via /// [`WalletSource::list_confirmed_utxos`]). - fn sign_tx(&self, tx: &mut Transaction) -> Result<(), ()>; + fn sign_tx(&self, tx: Transaction) -> Result; } /// A wrapper over [`WalletSource`] that implements [`CoinSelection`] by preferring UTXOs that would @@ -629,7 +629,7 @@ where .or_else(|_| do_coin_selection(true, true)) } - fn sign_tx(&self, tx: &mut Transaction) -> Result<(), ()> { + fn sign_tx(&self, tx: Transaction) -> Result { self.source.sign_tx(tx) } } @@ -748,7 +748,8 @@ where let unsigned_tx_weight = anchor_tx.weight() as u64 - (anchor_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT); log_debug!(self.logger, "Signing anchor transaction {}", anchor_txid); - self.utxo_source.sign_tx(&mut anchor_tx)?; + anchor_tx = self.utxo_source.sign_tx(anchor_tx)?; + let signer = anchor_descriptor.derive_channel_signer(&self.signer_provider); let anchor_sig = signer.sign_holder_anchor_input(&anchor_tx, 0, &self.secp)?; anchor_tx.input[0].witness = anchor_descriptor.tx_input_witness(&anchor_sig); @@ -812,7 +813,8 @@ where let unsigned_tx_weight = htlc_tx.weight() as u64 - (htlc_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT); log_debug!(self.logger, "Signing HTLC transaction {}", htlc_tx.txid()); - self.utxo_source.sign_tx(&mut htlc_tx)?; + htlc_tx = self.utxo_source.sign_tx(htlc_tx)?; + let mut signers = BTreeMap::new(); for (idx, htlc_descriptor) in htlc_descriptors.iter().enumerate() { let signer = signers.entry(htlc_descriptor.channel_derivation_parameters.keys_id) diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index bf5aa02bb41..3cc08e04628 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -1105,20 +1105,20 @@ impl TestWalletSource { } impl WalletSource for TestWalletSource { - fn list_confirmed_utxos(&self) -> Result, ()> { + fn list_confirmed_utxos(&self) -> Result, ()> { Ok(self.utxos.borrow().clone()) - } + } - fn get_change_script(&self) -> Result { + fn get_change_script(&self) -> Result { let public_key = bitcoin::PublicKey::new(self.secret_key.public_key(&self.secp)); Ok(Script::new_p2pkh(&public_key.pubkey_hash())) - } + } - fn sign_tx(&self, tx: &mut Transaction) -> Result<(), ()> { + fn sign_tx(&self, mut tx: Transaction) -> Result { let utxos = self.utxos.borrow(); for i in 0..tx.input.len() { if let Some(utxo) = utxos.iter().find(|utxo| utxo.outpoint == tx.input[i].previous_output) { - let sighash = SighashCache::new(&*tx) + let sighash = SighashCache::new(&tx) .legacy_signature_hash(i, &utxo.output.script_pubkey, EcdsaSighashType::All as u32) .map_err(|_| ())?; let sig = self.secp.sign_ecdsa(&sighash.as_hash().into(), &self.secret_key); @@ -1129,6 +1129,6 @@ impl WalletSource for TestWalletSource { .into_script(); } } - Ok(()) - } + Ok(tx) + } } From 0c629ff60a0f47757e0faab7e0508ed9aae057f5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 18 Jul 2023 19:34:00 +0000 Subject: [PATCH 3/5] Pass must-spend inputs to users by ownership We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way. --- lightning/src/events/bump_transaction.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index a9133f50ed8..751a43e0233 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -464,7 +464,7 @@ pub trait CoinSelectionSource { /// which UTXOs to double spend is left to the implementation, but it must strive to keep the /// set of other claims being double spent to a minimum. fn select_confirmed_utxos( - &self, claim_id: ClaimId, must_spend: &[Input], must_pay_to: &[TxOut], + &self, claim_id: ClaimId, must_spend: Vec, must_pay_to: &[TxOut], target_feerate_sat_per_1000_weight: u32, ) -> Result; /// Signs and provides the full witness for all inputs within the transaction known to the @@ -600,7 +600,7 @@ where L::Target: Logger { fn select_confirmed_utxos( - &self, claim_id: ClaimId, must_spend: &[Input], must_pay_to: &[TxOut], + &self, claim_id: ClaimId, must_spend: Vec, must_pay_to: &[TxOut], target_feerate_sat_per_1000_weight: u32, ) -> Result { let utxos = self.source.list_confirmed_utxos()?; @@ -726,7 +726,7 @@ where satisfaction_weight: commitment_tx.weight() as u64 + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT, }]; let coin_selection = self.utxo_source.select_confirmed_utxos( - claim_id, &must_spend, &[], anchor_target_feerate_sat_per_1000_weight, + claim_id, must_spend, &[], anchor_target_feerate_sat_per_1000_weight, )?; let mut anchor_tx = Transaction { @@ -800,13 +800,16 @@ where log_debug!(self.logger, "Peforming coin selection for HTLC transaction targeting {} sat/kW", target_feerate_sat_per_1000_weight); + #[cfg(debug_assertions)] + let must_spend_satisfaction_weight = + must_spend.iter().map(|input| input.satisfaction_weight).sum::(); let coin_selection = self.utxo_source.select_confirmed_utxos( - claim_id, &must_spend, &htlc_tx.output, target_feerate_sat_per_1000_weight, + claim_id, must_spend, &htlc_tx.output, target_feerate_sat_per_1000_weight, )?; #[cfg(debug_assertions)] let total_satisfaction_weight = coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::() + - must_spend.iter().map(|input| input.satisfaction_weight).sum::(); + must_spend_satisfaction_weight; self.process_coin_selection(&mut htlc_tx, coin_selection); #[cfg(debug_assertions)] From d2c20ecc2d2eec46b5f1d29173bc9e29cd5179a0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 18 Jul 2023 19:41:07 +0000 Subject: [PATCH 4/5] Pass `InFlightHltcs` to the scorer by ownership rather than ref Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route. --- fuzz/src/chanmon_consistency.rs | 2 +- fuzz/src/full_stack.rs | 2 +- lightning/src/ln/outbound_payment.rs | 4 ++-- lightning/src/ln/payment_tests.rs | 2 +- lightning/src/routing/router.rs | 10 +++++----- lightning/src/util/test_utils.rs | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index cd9c484da2a..e923ef882f2 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -89,7 +89,7 @@ struct FuzzRouter {} impl Router for FuzzRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, - _inflight_htlcs: &InFlightHtlcs + _inflight_htlcs: InFlightHtlcs ) -> Result { Err(msgs::LightningError { err: String::from("Not implemented"), diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 9caf9104034..1fbd7dbec88 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -131,7 +131,7 @@ struct FuzzRouter {} impl Router for FuzzRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, - _inflight_htlcs: &InFlightHtlcs + _inflight_htlcs: InFlightHtlcs ) -> Result { Err(msgs::LightningError { err: String::from("Not implemented"), diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 546dc6c5bcd..30e718dccd6 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -669,7 +669,7 @@ impl OutboundPayments { let route = router.find_route_with_id( &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, - Some(&first_hops.iter().collect::>()), &inflight_htlcs(), + Some(&first_hops.iter().collect::>()), inflight_htlcs(), payment_hash, payment_id, ).map_err(|_| RetryableSendFailure::RouteNotFound)?; @@ -712,7 +712,7 @@ impl OutboundPayments { let route = match router.find_route_with_id( &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, - Some(&first_hops.iter().collect::>()), &inflight_htlcs(), + Some(&first_hops.iter().collect::>()), inflight_htlcs(), payment_hash, payment_id, ) { Ok(route) => route, diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index b23b6364182..2ae606106c9 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -3218,7 +3218,7 @@ fn do_claim_from_closed_chan(fail_payment: bool) { final_value_msat: 10_000_000, }; let mut route = nodes[0].router.find_route(&nodes[0].node.get_our_node_id(), &route_params, - None, &nodes[0].node.compute_inflight_htlcs()).unwrap(); + None, nodes[0].node.compute_inflight_htlcs()).unwrap(); // Make sure the route is ordered as the B->D path before C->D route.paths.sort_by(|a, _| if a.hops[0].pubkey == nodes[1].node.get_our_node_id() { std::cmp::Ordering::Less } else { std::cmp::Ordering::Greater }); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 9242e98b1ab..f0822ed5b38 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -64,7 +64,7 @@ impl< G: Deref>, L: Deref, S: Deref, SP: Sized, Sc: Sco payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>, - inflight_htlcs: &InFlightHtlcs + inflight_htlcs: InFlightHtlcs ) -> Result { let random_seed_bytes = { let mut locked_random_seed_bytes = self.random_seed_bytes.lock().unwrap(); @@ -73,7 +73,7 @@ impl< G: Deref>, L: Deref, S: Deref, SP: Sized, Sc: Sco }; find_route( payer, params, &self.network_graph, first_hops, &*self.logger, - &ScorerAccountingForInFlightHtlcs::new(self.scorer.lock().deref_mut(), inflight_htlcs), + &ScorerAccountingForInFlightHtlcs::new(self.scorer.lock().deref_mut(), &inflight_htlcs), &self.score_params, &random_seed_bytes ) @@ -85,13 +85,13 @@ pub trait Router { /// Finds a [`Route`] between `payer` and `payee` for a payment with the given values. fn find_route( &self, payer: &PublicKey, route_params: &RouteParameters, - first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: &InFlightHtlcs + first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs ) -> Result; /// Finds a [`Route`] between `payer` and `payee` for a payment with the given values. Includes /// `PaymentHash` and `PaymentId` to be able to correlate the request with a specific payment. fn find_route_with_id( &self, payer: &PublicKey, route_params: &RouteParameters, - first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: &InFlightHtlcs, + first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs, _payment_hash: PaymentHash, _payment_id: PaymentId ) -> Result { self.find_route(payer, route_params, first_hops, inflight_htlcs) @@ -112,7 +112,7 @@ pub struct ScorerAccountingForInFlightHtlcs<'a, S: Score, SP: impl<'a, S: Score, SP: Sized> ScorerAccountingForInFlightHtlcs<'a, S, SP> { /// Initialize a new `ScorerAccountingForInFlightHtlcs`. - pub fn new(scorer: &'a mut S, inflight_htlcs: &'a InFlightHtlcs) -> Self { + pub fn new(scorer: &'a mut S, inflight_htlcs: &'a InFlightHtlcs) -> Self { ScorerAccountingForInFlightHtlcs { scorer, inflight_htlcs diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 3cc08e04628..65c0483a59c 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -112,13 +112,13 @@ impl<'a> TestRouter<'a> { impl<'a> Router for TestRouter<'a> { fn find_route( &self, payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&channelmanager::ChannelDetails]>, - inflight_htlcs: &InFlightHtlcs + inflight_htlcs: InFlightHtlcs ) -> Result { if let Some((find_route_query, find_route_res)) = self.next_routes.lock().unwrap().pop_front() { assert_eq!(find_route_query, *params); if let Ok(ref route) = find_route_res { let mut binding = self.scorer.lock().unwrap(); - let scorer = ScorerAccountingForInFlightHtlcs::new(binding.deref_mut(), inflight_htlcs); + let scorer = ScorerAccountingForInFlightHtlcs::new(binding.deref_mut(), &inflight_htlcs); for path in &route.paths { let mut aggregate_msat = 0u64; for (idx, hop) in path.hops.iter().rev().enumerate() { From 35dda4e61ce3357f66e484ba33c408191707c824 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 18 Jul 2023 19:52:26 +0000 Subject: [PATCH 5/5] Drop `c_bindings` implementation of scoring on `Mutex`/`RefCell` This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them. --- lightning/src/routing/scoring.rs | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index d83adaf97f6..615cc1a19eb 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -175,7 +175,7 @@ pub trait WriteableScore<'a>: LockableScore<'a> + Writeable {} #[cfg(not(c_bindings))] impl<'a, T> WriteableScore<'a> for T where T: LockableScore<'a> + Writeable {} -/// This is not exported to bindings users +#[cfg(not(c_bindings))] impl<'a, T: 'a + Score> LockableScore<'a> for Mutex { type Score = T; type Locked = MutexGuard<'a, T>; @@ -185,6 +185,7 @@ impl<'a, T: 'a + Score> LockableScore<'a> for Mutex { } } +#[cfg(not(c_bindings))] impl<'a, T: 'a + Score> LockableScore<'a> for RefCell { type Score = T; type Locked = RefMut<'a, T>; @@ -255,21 +256,7 @@ impl<'a, T: 'a + Score> Deref for MultiThreadedScoreLock<'a, T> { } } -#[cfg(c_bindings)] -/// This is not exported to bindings users -impl<'a, T: Writeable> Writeable for RefMut<'a, T> { - fn write(&self, writer: &mut W) -> Result<(), io::Error> { - T::write(&**self, writer) - } -} -#[cfg(c_bindings)] -/// This is not exported to bindings users -impl<'a, S: Writeable> Writeable for MutexGuard<'a, S> { - fn write(&self, writer: &mut W) -> Result<(), io::Error> { - S::write(&**self, writer) - } -} /// Proposed use of a channel passed as a parameter to [`Score::channel_penalty_msat`]. #[derive(Clone, Copy, Debug, PartialEq)]