Merge pull request #2676 from TheBlueMatt/2023-10-various-followups

Various Followups to 2039 and 2674

Author: TheBlueMatt

ci/ci-tests.sh

@@ -100,14 +100,16 @@ popd
 
 echo -e "\n\nTesting no-std flags in various combinations"
 for DIR in lightning lightning-invoice lightning-rapid-gossip-sync; do
-	pushd $DIR
-	cargo test --verbose --color always --no-default-features --features no-std
+	cargo test -p $DIR --verbose --color always --no-default-features --features no-std
 	# check if there is a conflict between no-std and the default std feature
-	cargo test --verbose --color always --features no-std
+	cargo test -p $DIR --verbose --color always --features no-std
+done
+for DIR in lightning lightning-invoice lightning-rapid-gossip-sync; do
 	# check if there is a conflict between no-std and the c_bindings cfg
-	RUSTFLAGS="--cfg=c_bindings" cargo test --verbose --color always --no-default-features --features=no-std
-	popd
+	RUSTFLAGS="--cfg=c_bindings" cargo test -p $DIR --verbose --color always --no-default-features --features=no-std
 done
+RUSTFLAGS="--cfg=c_bindings" cargo test --verbose --color always
+
 # Note that outbound_commitment_test only runs in this mode because of hardcoded signature values
 pushd lightning
 cargo test --verbose --color always --no-default-features --features=std,_test_vectors

lightning-background-processor/src/lib.rs

@@ -864,7 +864,7 @@ mod tests {
 	use lightning::ln::peer_handler::{PeerManager, MessageHandler, SocketDescriptor, IgnoringMessageHandler};
 	use lightning::routing::gossip::{NetworkGraph, NodeId, P2PGossipSync};
 	use lightning::routing::router::{DefaultRouter, Path, RouteHop};
-	use lightning::routing::scoring::{ChannelUsage, ScoreUpdate, ScoreLookUp};
+	use lightning::routing::scoring::{ChannelUsage, ScoreUpdate, ScoreLookUp, LockableScore};
 	use lightning::util::config::UserConfig;
 	use lightning::util::ser::Writeable;
 	use lightning::util::test_utils;
@@ -894,6 +894,11 @@ mod tests {
 		fn disconnect_socket(&mut self) {}
 	}
 
+	#[cfg(c_bindings)]
+	type LockingWrapper<T> = lightning::routing::scoring::MultiThreadedLockableScore<T>;
+	#[cfg(not(c_bindings))]
+	type LockingWrapper<T> = Mutex<T>;
+
 	type ChannelManager =
 		channelmanager::ChannelManager<
 			Arc<ChainMonitor>,
@@ -905,7 +910,7 @@ mod tests {
 			Arc<DefaultRouter<
 				Arc<NetworkGraph<Arc<test_utils::TestLogger>>>,
 				Arc<test_utils::TestLogger>,
-				Arc<Mutex<TestScorer>>,
+				Arc<LockingWrapper<TestScorer>>,
 				(),
 				TestScorer>
 			>,
@@ -927,7 +932,7 @@ mod tests {
 		network_graph: Arc<NetworkGraph<Arc<test_utils::TestLogger>>>,
 		logger: Arc<test_utils::TestLogger>,
 		best_block: BestBlock,
-		scorer: Arc<Mutex<TestScorer>>,
+		scorer: Arc<LockingWrapper<TestScorer>>,
 	}
 
 	impl Node {
@@ -1148,6 +1153,9 @@ mod tests {
 		}
 	}
 
+	#[cfg(c_bindings)]
+	impl lightning::routing::scoring::Score for TestScorer {}
+
 	impl Drop for TestScorer {
 		fn drop(&mut self) {
 			if std::thread::panicking() {
@@ -1179,7 +1187,7 @@ mod tests {
 			let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", i)));
 			let genesis_block = genesis_block(network);
 			let network_graph = Arc::new(NetworkGraph::new(network, logger.clone()));
-			let scorer = Arc::new(Mutex::new(TestScorer::new()));
+			let scorer = Arc::new(LockingWrapper::new(TestScorer::new()));
 			let seed = [i as u8; 32];
 			let router = Arc::new(DefaultRouter::new(network_graph.clone(), logger.clone(), seed, scorer.clone(), Default::default()));
 			let chain_source = Arc::new(test_utils::TestChainSource::new(Network::Bitcoin));
@@ -1689,7 +1697,7 @@ mod tests {
 				maybe_announced_channel: true,
 			}], blinded_tail: None };
 
-			$nodes[0].scorer.lock().unwrap().expect(TestResult::PaymentFailure { path: path.clone(), short_channel_id: scored_scid });
+			$nodes[0].scorer.write_lock().expect(TestResult::PaymentFailure { path: path.clone(), short_channel_id: scored_scid });
 			$nodes[0].node.push_pending_event(Event::PaymentPathFailed {
 				payment_id: None,
 				payment_hash: PaymentHash([42; 32]),
@@ -1706,7 +1714,7 @@ mod tests {
 
 			// Ensure we'll score payments that were explicitly failed back by the destination as
 			// ProbeSuccess.
-			$nodes[0].scorer.lock().unwrap().expect(TestResult::ProbeSuccess { path: path.clone() });
+			$nodes[0].scorer.write_lock().expect(TestResult::ProbeSuccess { path: path.clone() });
 			$nodes[0].node.push_pending_event(Event::PaymentPathFailed {
 				payment_id: None,
 				payment_hash: PaymentHash([42; 32]),
@@ -1721,7 +1729,7 @@ mod tests {
 				_ => panic!("Unexpected event"),
 			}
 
-			$nodes[0].scorer.lock().unwrap().expect(TestResult::PaymentSuccess { path: path.clone() });
+			$nodes[0].scorer.write_lock().expect(TestResult::PaymentSuccess { path: path.clone() });
 			$nodes[0].node.push_pending_event(Event::PaymentPathSuccessful {
 				payment_id: PaymentId([42; 32]),
 				payment_hash: None,
@@ -1733,7 +1741,7 @@ mod tests {
 				_ => panic!("Unexpected event"),
 			}
 
-			$nodes[0].scorer.lock().unwrap().expect(TestResult::ProbeSuccess { path: path.clone() });
+			$nodes[0].scorer.write_lock().expect(TestResult::ProbeSuccess { path: path.clone() });
 			$nodes[0].node.push_pending_event(Event::ProbeSuccessful {
 				payment_id: PaymentId([42; 32]),
 				payment_hash: PaymentHash([42; 32]),
@@ -1745,7 +1753,7 @@ mod tests {
 				_ => panic!("Unexpected event"),
 			}
 
-			$nodes[0].scorer.lock().unwrap().expect(TestResult::ProbeFailure { path: path.clone() });
+			$nodes[0].scorer.write_lock().expect(TestResult::ProbeFailure { path: path.clone() });
 			$nodes[0].node.push_pending_event(Event::ProbeFailed {
 				payment_id: PaymentId([42; 32]),
 				payment_hash: PaymentHash([42; 32]),

lightning/src/chain/chaininterface.rs

@@ -71,14 +71,12 @@ pub enum ConfirmationTarget {
 	/// [`ChannelConfig::max_dust_htlc_exposure`]: crate::util::config::ChannelConfig::max_dust_htlc_exposure
 	MaxAllowedNonAnchorChannelRemoteFee,
 	/// This is the lowest feerate we will allow our channel counterparty to have in an anchor
-	/// channel in order to close the channel if a channel party goes away. Because our counterparty
-	/// must ensure they can always broadcast the latest state, this value being too high will cause
-	/// immediate force-closures.
+	/// channel in order to close the channel if a channel party goes away.
 	///
 	/// This needs to be sufficient to get into the mempool when the channel needs to
-	/// be force-closed. Setting too low may result in force-closures. Because this is for anchor
-	/// channels, we can always bump the feerate later, the feerate here only needs to suffice to
-	/// enter the mempool.
+	/// be force-closed. Setting too high may result in force-closures if our counterparty attempts
+	/// to use a lower feerate. Because this is for anchor channels, we can always bump the feerate
+	/// later; the feerate here only needs to be sufficient to enter the mempool.
 	///
 	/// A good estimate is the expected mempool minimum at the time of force-closure. Obviously this
 	/// is not an estimate which is very easy to calculate because we do not know the future. Using
@@ -87,13 +85,10 @@ pub enum ConfirmationTarget {
 	/// (package relay) may obviate the need for this entirely.
 	MinAllowedAnchorChannelRemoteFee,
 	/// The lowest feerate we will allow our channel counterparty to have in a non-anchor channel.
-	/// This needs to be sufficient to get confirmed when the channel needs to be force-closed.
-	/// Setting too low may result in force-closures.
 	///
 	/// This is the feerate on the transaction which we (or our counterparty) will broadcast in
-	/// order to close the channel if a channel party goes away. Because our counterparty must
-	/// ensure they can always broadcast the latest state, this value being too high will cause
-	/// immediate force-closures.
+	/// order to close the channel if a channel party goes away. Setting this value too high will
+	/// cause immediate force-closures in order to avoid having an unbroadcastable state.
 	///
 	/// This feerate represents the fee we pick now, which must be sufficient to enter a block at an
 	/// arbitrary time in the future. Obviously this is not an estimate which is very easy to

lightning/src/chain/package.rs

@@ -24,7 +24,7 @@ use crate::ln::PaymentPreimage;
 use crate::ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment};
 use crate::ln::chan_utils;
 use crate::ln::msgs::DecodeError;
-use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, fee_for_weight, FEERATE_FLOOR_SATS_PER_KW};
+use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW};
 use crate::sign::WriteableEcdsaChannelSigner;
 use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler};
 use crate::util::logger::Logger;

lightning/src/ln/channelmanager.rs

@@ -827,7 +827,8 @@ struct PendingInboundPayment {
 /// or, respectively, [`Router`] for its router, but this type alias chooses the concrete types
 /// of [`KeysManager`] and [`DefaultRouter`].
 ///
-/// This is not exported to bindings users as Arcs don't make sense in bindings
+/// This is not exported to bindings users as type aliases aren't supported in most languages.
+#[cfg(not(c_bindings))]
 pub type SimpleArcChannelManager<M, T, F, L> = ChannelManager<
 	Arc<M>,
 	Arc<T>,
@@ -855,7 +856,8 @@ pub type SimpleArcChannelManager<M, T, F, L> = ChannelManager<
 /// or, respectively, [`Router`]  for its router, but this type alias chooses the concrete types
 /// of [`KeysManager`] and [`DefaultRouter`].
 ///
-/// This is not exported to bindings users as Arcs don't make sense in bindings
+/// This is not exported to bindings users as type aliases aren't supported in most languages.
+#[cfg(not(c_bindings))]
 pub type SimpleRefChannelManager<'a, 'b, 'c, 'd, 'e, 'f, 'g, 'h, M, T, F, L> =
 	ChannelManager<
 		&'a M,
@@ -7329,6 +7331,8 @@ where
 	/// Requires a direct connection to the introduction node in the responding [`InvoiceRequest`]'s
 	/// reply path.
 	///
+	/// This is not exported to bindings users as builder patterns don't map outside of move semantics.
+	///
 	/// [`Offer`]: crate::offers::offer::Offer
 	/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
 	pub fn create_offer_builder(
@@ -7362,6 +7366,9 @@ where
 	/// invoice. If abandoned, or an invoice isn't received before expiration, the payment will fail
 	/// with an [`Event::InvoiceRequestFailed`].
 	///
+	/// If `max_total_routing_fee_msat` is not specified, The default from
+	/// [`RouteParameters::from_payment_params_and_value`] is applied.
+	///
 	/// # Privacy
 	///
 	/// Uses a one-hop [`BlindedPath`] for the refund with [`ChannelManager::get_our_node_id`] as
@@ -7379,6 +7386,8 @@ where
 	/// Errors if a duplicate `payment_id` is provided given the caveats in the aforementioned link
 	/// or if `amount_msats` is invalid.
 	///
+	/// This is not exported to bindings users as builder patterns don't map outside of move semantics.
+	///
 	/// [`Refund`]: crate::offers::refund::Refund
 	/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice
 	/// [`Bolt12Invoice::payment_paths`]: crate::offers::invoice::Bolt12Invoice::payment_paths
@@ -7421,6 +7430,9 @@ where
 	/// - `amount_msats` if overpaying what is required for the given `quantity` is desired, and
 	/// - `payer_note` for [`InvoiceRequest::payer_note`].
 	///
+	/// If `max_total_routing_fee_msat` is not specified, The default from
+	/// [`RouteParameters::from_payment_params_and_value`] is applied.
+	///
 	/// # Payment
 	///
 	/// The provided `payment_id` is used to ensure that only one invoice is paid for the request
@@ -8961,10 +8973,10 @@ where
 								match invoice.sign(|invoice| self.node_signer.sign_bolt12_invoice(invoice)) {
 									Ok(invoice) => Ok(OffersMessage::Invoice(invoice)),
 									Err(SignError::Signing(())) => Err(OffersMessage::InvoiceError(
-											InvoiceError::from_str("Failed signing invoice")
+											InvoiceError::from_string("Failed signing invoice".to_string())
 									)),
 									Err(SignError::Verification(_)) => Err(OffersMessage::InvoiceError(
-											InvoiceError::from_str("Failed invoice signature verification")
+											InvoiceError::from_string("Failed invoice signature verification".to_string())
 									)),
 								});
 						match response {
@@ -8980,15 +8992,15 @@ where
 			OffersMessage::Invoice(invoice) => {
 				match invoice.verify(expanded_key, secp_ctx) {
 					Err(()) => {
-						Some(OffersMessage::InvoiceError(InvoiceError::from_str("Unrecognized invoice")))
+						Some(OffersMessage::InvoiceError(InvoiceError::from_string("Unrecognized invoice".to_owned())))
 					},
 					Ok(_) if invoice.invoice_features().requires_unknown_bits_from(&self.bolt12_invoice_features()) => {
 						Some(OffersMessage::InvoiceError(Bolt12SemanticError::UnknownRequiredFeatures.into()))
 					},
 					Ok(payment_id) => {
 						if let Err(e) = self.send_payment_for_bolt12_invoice(&invoice, payment_id) {
 							log_trace!(self.logger, "Failed paying invoice: {:?}", e);
-							Some(OffersMessage::InvoiceError(InvoiceError::from_str(&format!("{:?}", e))))
+							Some(OffersMessage::InvoiceError(InvoiceError::from_string(format!("{:?}", e))))
 						} else {
 							None
 						}

lightning/src/ln/outbound_payment.rs

@@ -762,7 +762,6 @@ impl OutboundPayments {
 		}
 	}
 
-	#[allow(unused)]
 	pub(super) fn send_payment_for_bolt12_invoice<R: Deref, ES: Deref, NS: Deref, IH, SP, L: Deref>(
 		&self, invoice: &Bolt12Invoice, payment_id: PaymentId, router: &R,
 		first_hops: Vec<ChannelDetails>, inflight_htlcs: IH, entropy_source: &ES, node_signer: &NS,
@@ -779,7 +778,7 @@ impl OutboundPayments {
 		SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
 	{
 		let payment_hash = invoice.payment_hash();
-		let mut max_total_routing_fee_msat = None;
+		let max_total_routing_fee_msat;
 		match self.pending_outbound_payments.lock().unwrap().entry(payment_id) {
 			hash_map::Entry::Occupied(entry) => match entry.get() {
 				PendingOutboundPayment::AwaitingInvoice { retry_strategy, max_total_routing_fee_msat: max_total_fee, .. } => {
@@ -795,11 +794,12 @@ impl OutboundPayments {
 			hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice),
 		};
 
-		let route_params = RouteParameters {
-			payment_params: PaymentParameters::from_bolt12_invoice(&invoice),
-			final_value_msat: invoice.amount_msats(),
-			max_total_routing_fee_msat,
-		};
+		let pay_params = PaymentParameters::from_bolt12_invoice(&invoice);
+		let amount_msat = invoice.amount_msats();
+		let mut route_params = RouteParameters::from_payment_params_and_value(pay_params, amount_msat);
+		if let Some(max_fee_msat) = max_total_routing_fee_msat {
+			route_params.max_total_routing_fee_msat = Some(max_fee_msat);
+		}
 
 		self.find_route_and_send_payment(
 			payment_hash, payment_id, route_params, router, first_hops, &inflight_htlcs,

lightning/src/ln/payment_tests.rs

@@ -1871,7 +1871,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 	let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
 	let route = get_route(
 		&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph.read_only(), None,
-		nodes[0].logger, &scorer, &(), &random_seed_bytes
+		nodes[0].logger, &scorer, &Default::default(), &random_seed_bytes
 	).unwrap();
 
 	let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap();

lightning/src/ln/peer_handler.rs

@@ -24,12 +24,15 @@ use crate::ln::ChannelId;
 use crate::ln::features::{InitFeatures, NodeFeatures};
 use crate::ln::msgs;
 use crate::ln::msgs::{ChannelMessageHandler, LightningError, SocketAddress, OnionMessageHandler, RoutingMessageHandler};
+#[cfg(not(c_bindings))]
 use crate::ln::channelmanager::{SimpleArcChannelManager, SimpleRefChannelManager};
 use crate::util::ser::{VecWriter, Writeable, Writer};
 use crate::ln::peer_channel_encryptor::{PeerChannelEncryptor,NextNoiseStep};
 use crate::ln::wire;
 use crate::ln::wire::{Encode, Type};
-use crate::onion_message::{CustomOnionMessageHandler, OffersMessage, OffersMessageHandler, OnionMessageContents, PendingOnionMessage, SimpleArcOnionMessenger, SimpleRefOnionMessenger};
+#[cfg(not(c_bindings))]
+use crate::onion_message::{SimpleArcOnionMessenger, SimpleRefOnionMessenger};
+use crate::onion_message::{CustomOnionMessageHandler, OffersMessage, OffersMessageHandler, OnionMessageContents, PendingOnionMessage};
 use crate::routing::gossip::{NetworkGraph, P2PGossipSync, NodeId, NodeAlias};
 use crate::util::atomic_counter::AtomicCounter;
 use crate::util::logger::Logger;
@@ -608,7 +611,8 @@ impl Peer {
 /// SimpleRefPeerManager is the more appropriate type. Defining these type aliases prevents
 /// issues such as overly long function definitions.
 ///
-/// This is not exported to bindings users as `Arc`s don't make sense in bindings.
+/// This is not exported to bindings users as type aliases aren't supported in most languages.
+#[cfg(not(c_bindings))]
 pub type SimpleArcPeerManager<SD, M, T, F, C, L> = PeerManager<
 	SD,
 	Arc<SimpleArcChannelManager<M, T, F, L>>,
@@ -626,7 +630,8 @@ pub type SimpleArcPeerManager<SD, M, T, F, C, L> = PeerManager<
 /// But if this is not necessary, using a reference is more efficient. Defining these type aliases
 /// helps with issues such as long function definitions.
 ///
-/// This is not exported to bindings users as general type aliases don't make sense in bindings.
+/// This is not exported to bindings users as type aliases aren't supported in most languages.
+#[cfg(not(c_bindings))]
 pub type SimpleRefPeerManager<
 	'a, 'b, 'c, 'd, 'e, 'f, 'logger, 'h, 'i, 'j, 'graph, 'k, SD, M, T, F, C, L
 > = PeerManager<

lightning/src/offers/invoice_error.rs

@@ -50,10 +50,10 @@ pub struct ErroneousField {
 
 impl InvoiceError {
 	/// Creates an [`InvoiceError`] with the given message.
-	pub fn from_str(s: &str) -> Self {
+	pub fn from_string(s: String) -> Self {
 		Self {
 			erroneous_field: None,
-			message: UntrustedString(s.to_string()),
+			message: UntrustedString(s),
 		}
 	}
 }

lightning/src/onion_message/messenger.rs

@@ -19,6 +19,7 @@ use crate::blinded_path::BlindedPath;
 use crate::blinded_path::message::{advance_path_by_one, ForwardTlvs, ReceiveTlvs};
 use crate::blinded_path::utils;
 use crate::sign::{EntropySource, KeysManager, NodeSigner, Recipient};
+#[cfg(not(c_bindings))]
 use crate::ln::channelmanager::{SimpleArcChannelManager, SimpleRefChannelManager};
 use crate::ln::features::{InitFeatures, NodeFeatures};
 use crate::ln::msgs::{self, OnionMessage, OnionMessageHandler};
@@ -712,10 +713,11 @@ where
 /// Useful for simplifying the parameters of [`SimpleArcChannelManager`] and
 /// [`SimpleArcPeerManager`]. See their docs for more details.
 ///
-/// This is not exported to bindings users as `Arc`s don't make sense in bindings.
+/// This is not exported to bindings users as type aliases aren't supported in most languages.
 ///
 /// [`SimpleArcChannelManager`]: crate::ln::channelmanager::SimpleArcChannelManager
 /// [`SimpleArcPeerManager`]: crate::ln::peer_handler::SimpleArcPeerManager
+#[cfg(not(c_bindings))]
 pub type SimpleArcOnionMessenger<M, T, F, L> = OnionMessenger<
 	Arc<KeysManager>,
 	Arc<KeysManager>,
@@ -728,10 +730,11 @@ pub type SimpleArcOnionMessenger<M, T, F, L> = OnionMessenger<
 /// Useful for simplifying the parameters of [`SimpleRefChannelManager`] and
 /// [`SimpleRefPeerManager`]. See their docs for more details.
 ///
-/// This is not exported to bindings users as general type aliases don't make sense in bindings.
+/// This is not exported to bindings users as type aliases aren't supported in most languages.
 ///
 /// [`SimpleRefChannelManager`]: crate::ln::channelmanager::SimpleRefChannelManager
 /// [`SimpleRefPeerManager`]: crate::ln::peer_handler::SimpleRefPeerManager
+#[cfg(not(c_bindings))]
 pub type SimpleRefOnionMessenger<
 	'a, 'b, 'c, 'd, 'e, 'f, 'g, 'h, 'i, 'j, M, T, F, L
 > = OnionMessenger<