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.

diff --git a/lightning-invoice/src/lib.rs b/lightning-invoice/src/lib.rs
index d676f6c28d6faf7926fa970030ffb1d258fa1af7..dddc0d734f405330a0c84ab6b35fe5892596c727 100644 (file)
--- a/lightning-invoice/src/lib.rs
+++ b/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
        }
 }

diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
index 0ccce88e9f5d71743530a441504ac7c18b1da6a2..7d7553dfa0222acc76c66e239bb64613c658004d 100644 (file)
--- a/lightning/src/ln/features.rs
+++ b/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,7 +888,8 @@ 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());
@@ -894,9 +897,9 @@ mod tests {
 
        #[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());

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 65119f0f843a5266f248ed53f1a9cdd05653a99e..740fd691e75c85e41f1f3ae06c5a1e18f41c82fa 100644 (file)
--- a/lightning/src/routing/router.rs
+++ b/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);