Use &mut self in invoice updaters, not take-self-return-Self

The take-self-return-Self idiom in Rust is substantially less
usable than it is in Java, where its more common. Because we have
to take self by move, it prevents using the update methods to
actually update features, something we occasionally want to do.

See, eg, the change in lightning-invoice where we previously had
to copy and re-create an entire vec of fields just to update the
features field, which is nuts.

There are a few places where this makes things a little less clean,
but the tradeoff to enable more effecient and broader uses of the
update methods seems worth it.

Author: TheBlueMatt

lightning-invoice/src/lib.rs

@@ -610,9 +610,9 @@ impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, T,
 impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::False> {
 	/// Sets the payment secret and relevant features.
 	pub fn payment_secret(mut self, payment_secret: PaymentSecret) -> InvoiceBuilder<D, H, T, C, tb::True> {
-		let features = InvoiceFeatures::empty()
-			.set_variable_length_onion_required()
-			.set_payment_secret_required();
+		let mut features = InvoiceFeatures::empty();
+		features.set_variable_length_onion_required();
+		features.set_payment_secret_required();
 		self.tagged_fields.push(TaggedField::PaymentSecret(payment_secret));
 		self.tagged_fields.push(TaggedField::Features(features));
 		self.set_flags()
@@ -622,13 +622,11 @@ impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T,
 impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::True> {
 	/// Sets the `basic_mpp` feature as optional.
 	pub fn basic_mpp(mut self) -> Self {
-		self.tagged_fields = self.tagged_fields
-			.drain(..)
-			.map(|field| match field {
-				TaggedField::Features(f) => TaggedField::Features(f.set_basic_mpp_optional()),
-				_ => field,
-			})
-			.collect();
+		for field in self.tagged_fields.iter_mut() {
+			if let TaggedField::Features(f) = field {
+				f.set_basic_mpp_optional();
+			}
+		}
 		self
 	}
 }

lightning/src/ln/features.rs

@@ -318,15 +318,13 @@ mod sealed {
 
 			impl <T: $feature> Features<T> {
 				/// Set this feature as optional.
-				pub fn $optional_setter(mut self) -> Self {
+				pub fn $optional_setter(&mut self) {
 					<T as $feature>::set_optional_bit(&mut self.flags);
-					self
 				}
 
 				/// Set this feature as required.
-				pub fn $required_setter(mut self) -> Self {
+				pub fn $required_setter(&mut self) {
 					<T as $feature>::set_required_bit(&mut self.flags);
-					self
 				}
 
 				/// Checks if this feature is supported.
@@ -506,7 +504,9 @@ impl InvoiceFeatures {
 	/// [`PaymentParameters::for_keysend`]: crate::routing::router::PaymentParameters::for_keysend
 	/// [`find_route`]: crate::routing::router::find_route
 	pub(crate) fn for_keysend() -> InvoiceFeatures {
-		InvoiceFeatures::empty().set_variable_length_onion_optional()
+		let mut res = InvoiceFeatures::empty();
+		res.set_variable_length_onion_optional();
+		res
 	}
 }
 
@@ -838,11 +838,13 @@ mod tests {
 		assert!(!features.requires_unknown_bits());
 		assert!(!features.supports_unknown_bits());
 
-		let features = ChannelFeatures::empty().set_unknown_feature_required();
+		let mut features = ChannelFeatures::empty();
+		features.set_unknown_feature_required();
 		assert!(features.requires_unknown_bits());
 		assert!(features.supports_unknown_bits());
 
-		let features = ChannelFeatures::empty().set_unknown_feature_optional();
+		let mut features = ChannelFeatures::empty();
+		features.set_unknown_feature_optional();
 		assert!(!features.requires_unknown_bits());
 		assert!(features.supports_unknown_bits());
 	}
@@ -886,17 +888,18 @@ mod tests {
 	fn convert_to_context_with_unknown_flags() {
 		// Ensure the `from` context has fewer known feature bytes than the `to` context.
 		assert!(InvoiceFeatures::known().flags.len() < NodeFeatures::known().flags.len());
-		let invoice_features = InvoiceFeatures::known().set_unknown_feature_optional();
+		let mut invoice_features = InvoiceFeatures::known();
+		invoice_features.set_unknown_feature_optional();
 		assert!(invoice_features.supports_unknown_bits());
 		let node_features: NodeFeatures = invoice_features.to_context();
 		assert!(!node_features.supports_unknown_bits());
 	}
 
 	#[test]
 	fn set_feature_bits() {
-		let features = InvoiceFeatures::empty()
-			.set_basic_mpp_optional()
-			.set_payment_secret_required();
+		let mut features = InvoiceFeatures::empty();
+		features.set_basic_mpp_optional();
+		features.set_payment_secret_required();
 		assert!(features.supports_basic_mpp());
 		assert!(!features.requires_basic_mpp());
 		assert!(features.requires_payment_secret());
@@ -938,7 +941,8 @@ mod tests {
 	fn test_channel_type_mapping() {
 		// If we map an InvoiceFeatures with StaticRemoteKey optional, it should map into a
 		// required-StaticRemoteKey ChannelTypeFeatures.
-		let init_features = InitFeatures::empty().set_static_remote_key_optional();
+		let mut init_features = InitFeatures::empty();
+		init_features.set_static_remote_key_optional();
 		let converted_features = ChannelTypeFeatures::from_counterparty_init(&init_features);
 		assert_eq!(converted_features, ChannelTypeFeatures::only_static_remote_key());
 		assert!(!converted_features.supports_any_optional_bits());

lightning/src/routing/router.rs

@@ -2369,7 +2369,8 @@ mod tests {
 		let scorer = test_utils::TestScorer::with_penalty(0);
 
 		// Disable nodes 1, 2, and 8 by requiring unknown feature bits
-		let unknown_features = NodeFeatures::known().set_unknown_feature_required();
+		let mut unknown_features = NodeFeatures::known();
+		unknown_features.set_unknown_feature_required();
 		add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[0], unknown_features.clone(), 1);
 		add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[1], unknown_features.clone(), 1);
 		add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[7], unknown_features.clone(), 1);