Make init message serialization legacy-compatible

Make init message serialization give special treatment to the
var_onion_optin feature flag. This flag (if it is present) will only be
placed in the global_features section of the init message while other
flags are only placed in the local_features section. This makes use
compatible with legacy clients that distinguish between global and local
features since var_onion_optin was the only global feature in the spec
before the spec was changed and the two feature sets were merged.

Some other miscellaneous changes are:

Add a SetFeature method to the FeatureBit type.

Remove the lazily-initialized mutable bytes field from FeatureBit since
this made it possible for FeatureBit.ByteArray to become out of sync
with FeatureBit.BitArray. ByteArray is now computed from the internal
bitArray on demand.

Modify the BitArray.ToByteArray extension method so that it is the
inverse of the BitArray.FromBytes extension method, rather than the two
methods ordering the bits within the bytes differently.
BitArray.ToByteArray will also return the minimal-length array required
to represent the BitArray rather than (possibly) containing 0x00 bytes
at the front (which is a protocol violation if sent over the network as
a feature set). This also means changing how Payment.TaggedField.WriteTo
serializes feature bits to avoid padding being dropped when padding to a
multiple of 5 bits in order to convert to Bech32.

Change the BitArray.{FromBytes,ToByteArray} property test to take into
account that zero bytes can be dropped from the front of the array.

Add a test case which checks that converting a FeatureBit from and to a
byte array correctly trims zero bytes at the start of the array,
preserves the byte order of the array, and preserves the bit order
within the bytes.

FeatureBit.Equals now just converts both FeatureBits to byte arrays and
tests those for equality. Since converting to bytes normalises the
feature bits by removing excess zeros, this change fixes an issue where
two FeatureBits of different lengths were considered unequal when the
extra bits were all zeros.

Author: canndrew

src/DotNetLightning.Core/Payment/PaymentRequest.fs

@@ -285,13 +285,11 @@ type TaggedField =
             let routeInfoBase32 = routeInfoBase256 |> Array.concat |>  Helpers.convert8BitsTo5
             this.WriteField(writer, routeInfoBase32)
         | FeaturesTaggedField f ->
-            if f.BitArray.Length = 0 then () else
-            let pad = if (f.BitArray.Length % 40 = 0) then 0 else 40 - (f.BitArray.Length % 40)
             let dBase32 =
-                let ba =
-                    [BitArray(pad, false); f.BitArray] |> BitArray.Concat |> fun baa -> baa.ToByteArray()
-                ba |> Helpers.convert8BitsTo5
-                |> Array.skipWhile((=)0uy)
+                let mutable byteArray = f.BitArray.ToByteArray()
+                while (byteArray.Length * 8) % 5 <> 0 do
+                    byteArray <- Array.concat [ [| 0uy |]; byteArray ]
+                byteArray |> Helpers.convert8BitsTo5 |> Array.skipWhile((=)0uy)
             this.WriteField(writer, dBase32)
 
 

src/DotNetLightning.Core/Serialize/Features.fs

@@ -29,6 +29,11 @@ type Feature = private {
     with
     member this.MandatoryBitPosition = this.Mandatory
     member this.OptionalBitPosition = this.Mandatory + 1
+    member this.BitPosition(support: FeaturesSupport) =
+        match support with
+        | Mandatory -> this.MandatoryBitPosition
+        | Optional -> this.OptionalBitPosition
+
     override this.ToString() = this.RfcName
 
     static member OptionDataLossProtect = {
@@ -89,7 +94,7 @@ module internal Feature =
         |> Map.add (Feature.BasicMultiPartPayment) ([Feature.PaymentSecret])
         |> Map.add (Feature.PaymentSecret) ([Feature.VariableLengthOnion])
 
-    let private isFeatureOn(features: BitArray) (bit: int) =
+    let isFeatureOn(features: BitArray) (bit: int) =
         (features.Length > bit) && features.Reverse().[bit]
 
     let hasFeature(features: BitArray) (f: Feature) (support: FeaturesSupport option) =
@@ -170,17 +175,14 @@ module internal Feature =
         |> Set
 
 
-/// Uses regular class instead of F# type for caching byte[] representation
 [<StructuredFormatDisplay("{PrettyPrint}")>]
 type FeatureBit private (bitArray) =
-    let mutable bytes = null
     member val BitArray: BitArray = bitArray with get, set
     member this.ByteArray
         with get() =
-            if isNull bytes then
-                bytes <- this.BitArray.ToByteArray()
-            bytes
-        and set(v: byte[]) = bytes <- v
+            this.BitArray.ToByteArray()
+        and set(bytes: byte[]) =
+            this.BitArray <- BitArray.FromBytes(bytes)
     static member TryCreate(ba: BitArray) =
         result {
             do! Feature.validateFeatureGraph(ba)
@@ -196,12 +198,8 @@ type FeatureBit private (bitArray) =
         let b: bool array = [||]
         b |> BitArray |> FeatureBit
     static member TryCreate(bytes: byte[]) =
-        result {
-            let! fb = FeatureBit.TryCreate(BitArray.FromBytes(bytes))
-            fb.ByteArray <- bytes
-            return fb
-        }
-        
+        FeatureBit.TryCreate(BitArray.FromBytes(bytes))
+
     static member TryCreate(v: int64) =
         BitArray.FromInt64(v) |> FeatureBit.TryCreate
 
@@ -230,6 +228,27 @@ type FeatureBit private (bitArray) =
     override this.ToString() =
         this.BitArray.PrintBits()
 
+    member this.SetFeature(feature: Feature) (support: FeaturesSupport) (on: bool): unit =
+        let index = feature.BitPosition support
+        let length = this.BitArray.Length
+        if length <= index then
+            this.BitArray.Length <- index + 1
+
+            //this.BitArray.RightShift(index - length + 1)
+
+            // NOTE: Calling RightShift gives me:
+            // "The field, constructor or member 'RightShift' is not defined."
+            // So I just re-implement it here
+            for i in (length - 1) .. -1 .. 0 do
+                this.BitArray.[i + index - length + 1] <- this.BitArray.[i]
+
+            // NOTE: this probably wouldn't be necessary if we were using
+            // RightShift, but the dotnet docs don't actualy specify that
+            // RightShift sets the leading bits to zero.
+            for i in 0 .. (index - length) do
+                this.BitArray.[i] <- false
+        this.BitArray.[this.BitArray.Length - index - 1] <- on
+
     member this.HasFeature(f, ?featureType) =
         Feature.hasFeature this.BitArray (f) (featureType)
 
@@ -249,12 +268,8 @@ type FeatureBit private (bitArray) =
 
     // --- equality and comparison members ----
     member this.Equals(o: FeatureBit) =
-        if this.BitArray.Length <> o.BitArray.Length then false else
-        let mutable result = true
-        for i in 0..this.BitArray.Length - 1 do
-            if this.BitArray.[i] <> o.BitArray.[i] then
-                result <- false
-        result
+        this.ByteArray = o.ByteArray
+
     interface IEquatable<FeatureBit> with
         member this.Equals(o: FeatureBit) = this.Equals(o)
     override this.Equals(other: obj) =

src/DotNetLightning.Core/Serialize/Msgs/Msgs.fs

@@ -9,6 +9,7 @@ open NBitcoin
 open DotNetLightning.Utils
 open DotNetLightning.Utils.Primitives
 open DotNetLightning.Utils.OnionError
+open DotNetLightning.Core.Utils.Extensions
 
 open DotNetLightning.Serialize
 
@@ -368,7 +369,8 @@ type Init =
         interface ISetupMsg
         interface ILightningSerializable<Init> with
             member this.Deserialize(ls: LightningReaderStream) =
-                // For backwards compatibility reason, we must consider legacy `global features` section. (see bolt 1)
+                // For backwards compatibility reason, we must consider legacy
+                // `global features` section. (see bolt 1)
                 let globalFeatures = ls.ReadWithLen()
                 let localFeatures = ls.ReadWithLen()
                 let oredFeatures =
@@ -389,11 +391,54 @@ type Init =
                     oredFeatures
                 this.Features <- oredFeatures |> FeatureBit.CreateUnsafe
                 this.TLVStream <- ls.ReadTLVStream() |> Array.map(InitTLV.FromGenericTLV)
+
             member this.Serialize(ls) =
-                // For backwards compatibility reason, we must consider legacy `global features` section. (see bolt 1)
-                let g: byte[] = [||]
-                ls.WriteWithLen(g)
-                ls.WriteWithLen(this.Features.ToByteArray())
+                // For legacy compatiblity we treat the VariableLengthOnion feature specially.
+                // Older versions of the lightning protocol spec had a
+                // distinction between global and local features, of which
+                // VariableLengthOnion was the only global feature. Although
+                // this distinction has since been removed, older clients still
+                // expect VariableLengthOnion to appear only in the
+                // global_features field of an init message and other features
+                // to appear only in the local_features field. This is still
+                // compatible with newer clients since those clients simply OR
+                // the two feature fields.
+                let localFeatures =
+                    let localFeatures = this.Features
+                    localFeatures.SetFeature
+                        Feature.VariableLengthOnion
+                        FeaturesSupport.Mandatory
+                        false
+                    localFeatures.SetFeature
+                        Feature.VariableLengthOnion
+                        FeaturesSupport.Optional
+                        false
+                    localFeatures.ToByteArray()
+                let globalFeatures =
+                    let globalFeatures = FeatureBit.Zero
+                    let mandatory =
+                        this.Features.HasFeature(
+                            Feature.VariableLengthOnion,
+                            FeaturesSupport.Mandatory
+                        )
+                    let optional =
+                        this.Features.HasFeature(
+                            Feature.VariableLengthOnion,
+                            FeaturesSupport.Optional
+                        )
+                    if mandatory then
+                        globalFeatures.SetFeature
+                            Feature.VariableLengthOnion
+                            FeaturesSupport.Mandatory
+                            true
+                    if optional then
+                        globalFeatures.SetFeature
+                            Feature.VariableLengthOnion
+                            FeaturesSupport.Optional
+                            true
+                    globalFeatures.ToByteArray()
+                ls.WriteWithLen(globalFeatures)
+                ls.WriteWithLen(localFeatures)
                 ls.WriteTLVStream(this.TLVStream |> Array.map(fun tlv -> tlv.ToGenericTLV()))
 
 [<CLIMutable>]

src/DotNetLightning.Core/Utils/Extensions.fs

@@ -101,11 +101,34 @@ type BitArrayExtensions() =
 type System.Collections.BitArray with
     member this.ToByteArray() =
         if this.Length = 0 then [||] else
-        let ret: byte[] = Array.zeroCreate (((this.Length - 1) / 8) + 1)
-        let boolArray: bool[] = this.Reverse()
-        let t = BitArray(boolArray)
-        t.CopyTo(ret, 0)
-        ret |> Array.rev
+
+        let leadingZeros =
+            match (Seq.tryFindIndex (fun b -> b) (Seq.cast this)) with
+            | Some i -> i
+            | None -> this.Length
+        let trueLength = this.Length - leadingZeros
+        let desiredLength = ((trueLength + 7) / 8) * 8
+        let difference = desiredLength - this.Length
+        let bitArray =
+            if difference < 0 then
+                // Drop zeroes from the front of the array until we have a multiple of 8 bits
+                let shortenedBitArray = BitArray(desiredLength)
+                for i in 0 .. (desiredLength - 1) do
+                    shortenedBitArray.[i] <- this.[i - difference]
+                shortenedBitArray
+            else if difference > 0 then
+                // Push zeroes to the front of the array until we have a multiple of 8 bits
+                let lengthenedBitArray = BitArray(desiredLength)
+                for i in 0 .. (this.Length - 1) do
+                    lengthenedBitArray.[i + difference] <- this.[i]
+                lengthenedBitArray
+            else
+                this
+
+        // Copy the bit array to a byte array, then flip the bytes.
+        let byteArray: byte[] = Array.zeroCreate(desiredLength / 8)
+        bitArray.CopyTo(byteArray, 0)
+        byteArray |> Array.map (fun b -> b.FlipBit())
 
     static member From5BitEncoding(b: byte[]) =
         let bitArray = System.Collections.BitArray(b.Length * 5)

tests/DotNetLightning.Core.Tests/PaymentTests.fs

@@ -172,17 +172,17 @@ let tests =
         testCase "Please send $30 for coffee beans to the same peer, which supports features 9, 15 and 99, using secret 0x1111111111111111111111111111111111111111111111111111111111111111" <| fun _ ->
             let data = "lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q5sqqqqqqqqqqqqqqqpqsq67gye39hfg3zd8rgc80k32tvy9xk2xunwm5lzexnvpx6fd77en8qaq424dxgt56cag2dpt359k3ssyhetktkpqh24jqnjyw6uqd08sgptq44qu"
             let pr = PaymentRequest.Parse(data) |> Result.deref
-            Expect.equal pr.PrefixValue "lnbc" ""
-            Expect.equal pr.PaymentHash ("0001020304050607080900010203040506070809000102030405060708090102" |> uint256.Parse |> PaymentHash) ""
-            Expect.equal (pr.TimestampValue.ToUnixTimeSeconds()) 1496314658L ""
-            Expect.equal (pr.NodeIdValue) ("03e7156ae33b0a208d0744199163177e909e80176e55d97a2f221ede0f934dd9ad" |> hex.DecodeData |> PubKey |> NodeId) ""
-            Expect.equal (pr.Description) (Choice1Of2 "coffee beans") ""
-            Expect.isEmpty pr.FallbackAddresses ""
-            Expect.isSome (pr.Features) ""
-            Expect.isTrue(pr.Features.Value.HasFeature(Feature.PaymentSecret, Optional)) ""
-            Expect.isTrue(pr.Features.Value.HasFeature(Feature.VariableLengthOnion, Optional)) ""
-            Expect.equal (pr.ToString()) data ""
-            Expect.equal (pr.ToString(msgSigner)) data ""
+            Expect.equal pr.PrefixValue "lnbc" "pr.PrefixValue mismatch"
+            Expect.equal pr.PaymentHash ("0001020304050607080900010203040506070809000102030405060708090102" |> uint256.Parse |> PaymentHash) "pr.PaymentHash mismatch"
+            Expect.equal (pr.TimestampValue.ToUnixTimeSeconds()) 1496314658L "pr.TimestampValue mismatch"
+            Expect.equal (pr.NodeIdValue) ("03e7156ae33b0a208d0744199163177e909e80176e55d97a2f221ede0f934dd9ad" |> hex.DecodeData |> PubKey |> NodeId) "pr.NodeIdValue mismatch"
+            Expect.equal (pr.Description) (Choice1Of2 "coffee beans") "pr.Description mismatch"
+            Expect.isEmpty pr.FallbackAddresses "pr.FallbackAddress mismatch"
+            Expect.isSome (pr.Features) "pr.Features mismatch"
+            Expect.isTrue(pr.Features.Value.HasFeature(Feature.PaymentSecret, Optional)) "pr.Features.Value (PaymentSecret) mismatch"
+            Expect.isTrue(pr.Features.Value.HasFeature(Feature.VariableLengthOnion, Optional)) "pr.Features.Value (VariableLengthOnion) mismatch"
+            Expect.equal (pr.ToString()) data "pr.ToString() mismatch"
+            Expect.equal (pr.ToString(msgSigner)) data "pr.ToString(msgSigned) mismatch"
 
         testCase "Same, but adding invalid unknown feature 100" <| fun _ ->
             let data = "lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q4psqqqqqqqqqqqqqqqpqsqq40wa3khl49yue3zsgm26jrepqr2eghqlx86rttutve3ugd05em86nsefzh4pfurpd9ek9w2vp95zxqnfe2u7ckudyahsa52q66tgzcp6t2dyk"

tests/DotNetLightning.Core.Tests/Serialization.fs

@@ -742,8 +742,19 @@ module SerializationTest =
                 ()
 
             testProperty "BitArray serialization" <| fun (ba : NonNull<byte[]>) ->
-                Expect.sequenceEqual (BitArray.FromBytes(ba.Get).ToByteArray()) (ba.Get) ""
-                
+                let backAndForth = BitArray.FromBytes(ba.Get).ToByteArray()
+                let finalArray = Array.zeroCreate ba.Get.Length
+                Array.Copy(backAndForth, 0, finalArray, ba.Get.Length - backAndForth.Length, backAndForth.Length)
+                Expect.equal ba.Get finalArray "BitArray.ToByteArray does not invert and trim BitArray.FromBytes"
+
+            testCase "FeatureBit to/from byte array preserves byte order, bit order and trims zero bytes" <| fun _ ->
+                let features = FeatureBit.Zero
+                features.ByteArray <- [| 0b00000000uy; 0b00100000uy; 0b10000010uy |]
+                Expect.equal
+                    features.ByteArray
+                    [| 0b00100000uy; 0b10000010uy |]
+                    "unexpected ByteArray value"
+
             testCase "features dependencies" <| fun _ ->
                 let testCases =
                     Map.empty