Skip to content

Commit 3872564

Browse files
optout21optout21
committed
Rework Bolt11Features deserialization, cleaner, better testable (with mutants)
1 parent 5fd4f77 commit 3872564

File tree

3 files changed

+98
-42
lines changed

3 files changed

+98
-42
lines changed

‎lightning-invoice/src/de.rs‎

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -71,26 +71,43 @@ impl FromBase32 for PaymentSecret {
7171
impl FromBase32 for Bolt11InvoiceFeatures {
7272
type Err = CheckedHrpstringError;
7373

74+
/// Convert to byte values, by packing the 5-bit groups,
75+
/// putting the 5-bit values from left to-right (reverse order),
76+
/// starting from the rightmost bit,
77+
/// and taking the resulting 8-bit values (right to left),
78+
/// with the leading 0's skipped.
7479
fn from_base32(field_data: &[Fe32]) -> Result<Self, Self::Err> {
75-
// Explanation for the "7": the normal way to round up when dividing is to add the divisor
76-
// minus one before dividing
77-
let length_bytes = (field_data.len() * 5 + 7) / 8 as usize;
78-
let mut res_bytes: Vec<u8> = vec![0; length_bytes];
79-
for (u5_idx, chunk) in field_data.iter().enumerate() {
80-
let bit_pos_from_right_0_indexed = (field_data.len() - u5_idx - 1) * 5;
81-
let new_byte_idx = (bit_pos_from_right_0_indexed / 8) as usize;
82-
let new_bit_pos = bit_pos_from_right_0_indexed % 8;
83-
let chunk_u16 = chunk.to_u8() as u16;
84-
res_bytes[new_byte_idx] |= ((chunk_u16 << new_bit_pos) & 0xff) as u8;
85-
if new_byte_idx != length_bytes - 1 {
86-
res_bytes[new_byte_idx + 1] |= ((chunk_u16 >> (8-new_bit_pos)) & 0xff) as u8;
80+
// Fe32 conversion cannot be used, because this unpacks from right, right-to-left
81+
// Carry bits, 0, 1, 2, 3, or 4 bits
82+
let mut carry_bits = 0;
83+
let mut carry = 0u8;
84+
let mut output = Vec::<u8>::new();
85+
86+
// Iterate over input in reverse
87+
for (_, curr_in) in field_data.into_iter().rev().enumerate() {
88+
let curr_in_as_u8 = curr_in.to_u8();
89+
if carry_bits >= 3 {
90+
// we have a new full byte -- 3, 4 or 5 carry bits, plus 5 new ones
91+
// For combining with carry '|', '^', or '+' can be used (disjoint bit positions)
92+
let next = carry + (curr_in_as_u8 << carry_bits);
93+
output.push(next);
94+
carry = curr_in_as_u8 >> (8 - carry_bits);
95+
carry_bits -= 3; // added 5, removed 8
96+
} else {
97+
// only 0, 1, or 2 carry bits, plus 5 new ones
98+
carry = carry + (curr_in_as_u8 << carry_bits);
99+
carry_bits += 5;
87100
}
88101
}
89-
// Trim the highest feature bits.
90-
while !res_bytes.is_empty() && res_bytes[res_bytes.len() - 1] == 0 {
91-
res_bytes.pop();
102+
// No more inputs, output remaining (if any)
103+
if carry_bits > 0 {
104+
output.push(carry);
92105
}
93-
Ok(Bolt11InvoiceFeatures::from_le_bytes(res_bytes))
106+
// Trim the highest feature bits
107+
while !output.is_empty() && output[output.len() - 1] == 0 {
108+
output.pop();
109+
}
110+
Ok(Bolt11InvoiceFeatures::from_le_bytes(output))
94111
}
95112
}
96113

‎lightning-invoice/src/ser.rs‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -79,42 +79,40 @@ impl Base32Len for PaymentSecret {
7979
}
8080

8181
impl Base32Iterable for Bolt11InvoiceFeatures {
82-
/// Convert to 32-byte values, by packing into 5 bits,
83-
/// putting the bytes from right-to-left (reverse order),
82+
/// Convert to 5-bit values, by unpacking the 5 bit groups,
83+
/// putting the bytes from right-to-left,
8484
/// starting from the rightmost bit,
8585
/// and taking the resulting 5-bit values in reverse (left-to-right),
8686
/// with the leading 0's skipped.
8787
fn fe_iter<'s>(&'s self) -> Box<dyn Iterator<Item = Fe32> + 's> {
8888
// Fe32 conversion cannot be used, because this packs from right, right-to-left
89-
let input_u8s = &self.le_flags();
90-
let mut remain_bits = 0;
91-
let mut remain_u8 = 0u8;
92-
let mut curr_idx = 0;
89+
let mut input_iter = self.le_flags().into_iter();
90+
// Carry bits, 0..7 bits
91+
let mut carry = 0u8;
92+
let mut carry_bits = 0;
9393
let mut output = Vec::<Fe32>::new();
9494

9595
loop {
96-
let next_out8 = if remain_bits >= 5 {
97-
// We have enough remained bits for an output, no need to read the input
98-
let next_out8 = remain_u8;
99-
remain_u8 = remain_u8 >> 5;
100-
remain_bits -= 5;
96+
let next_out8 = if carry_bits >= 5 {
97+
// We have enough carry bits for an output, no need to read the input
98+
let next_out8 = carry;
99+
carry = carry >> 5;
100+
carry_bits -= 5;
101101
next_out8
102102
} else {
103-
if curr_idx < input_u8s.len() {
104-
// take next byte
105-
let curr_in = input_u8s[curr_idx];
106-
curr_idx += 1;
103+
// take next byte
104+
if let Some(curr_in) = input_iter.next() {
107105
// we have at least one Fe32 to output (maybe two)
108-
let next_out8 = remain_u8 | (curr_in << remain_bits);
109-
let to_remain_shift = 5 - remain_bits;
110-
remain_u8 = curr_in >> to_remain_shift;
111-
remain_bits += 3; // added 8, removed 5
106+
// For combining with carry '|', '^', or '+' can be used (disjoint bit positions)
107+
let next_out8 = carry + (curr_in << carry_bits);
108+
carry = curr_in >> (5 - carry_bits);
109+
carry_bits += 3; // added 8, removed 5
112110
next_out8
113111
} else {
114112
// No more inputs, output remaining (if any)
115-
if remain_bits > 0 {
116-
remain_bits = 0;
117-
remain_u8
113+
if carry_bits > 0 {
114+
carry_bits = 0;
115+
carry
118116
} else {
119117
break;
120118
}
@@ -123,7 +121,7 @@ impl Base32Iterable for Bolt11InvoiceFeatures {
123121
// Isolate the 5 right bits
124122
output.push(Fe32::try_from(next_out8 & 31u8).expect("<32"))
125123
}
126-
// Take result in reverse order, skip leading 0s
124+
// Take result in reverse order, and skip leading 0s
127125
return Box::new(output.into_iter().rev().skip_while(|e| *e == Fe32::Q));
128126
}
129127
}

‎lightning-invoice/src/test_ser_de.rs‎

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ where
1717

1818
// deserialize back
1919
let o2 = T::from_base32(&serialized_32).unwrap();
20-
assert_eq!(o, o2);
20+
assert_eq!(o, o2, "Mismatch for {}", serialized_str);
2121
}
2222

2323
/// Test base32 encode and decode, and also length hint
@@ -76,7 +76,10 @@ fn bolt11_invoice_features() {
7676
use crate::Bolt11InvoiceFeatures;
7777

7878
// Test few values, lengths, and paddings
79-
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![1, 2, 3, 4, 5, 42, 100, 101]), "x2ep2q5zqxqsp");
79+
ser_de_test_len(
80+
Bolt11InvoiceFeatures::from_le_bytes(vec![1, 2, 3, 4, 5, 42, 100, 101]),
81+
"x2ep2q5zqxqsp",
82+
);
8083
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![]), "");
8184
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![0]), "");
8285
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![1]), "p");
@@ -88,11 +91,49 @@ fn bolt11_invoice_features() {
8891
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![1, 2, 3]), "xqsp");
8992
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![1, 2, 3, 4]), "zqxqsp");
9093
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![1, 2, 3, 4, 5]), "5zqxqsp");
91-
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![255, 254, 253, 252, 251]), "l070mlhl");
94+
ser_de_test_len(
95+
Bolt11InvoiceFeatures::from_le_bytes(vec![255, 254, 253, 252, 251]),
96+
"l070mlhl",
97+
);
9298
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![100, 0]), "ry");
9399
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![100, 0, 0, 0]), "ry");
94100
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![0, 100]), "eqq");
95101
ser_de_test_len(Bolt11InvoiceFeatures::from_le_bytes(vec![0, 0, 0, 100]), "pjqqqqq");
102+
ser_de_test_len(
103+
Bolt11InvoiceFeatures::from_le_bytes(vec![255, 255, 255, 255, 255, 255, 255, 255, 255]),
104+
"rllllllllllllll",
105+
);
106+
ser_de_test_len(
107+
Bolt11InvoiceFeatures::from_le_bytes(vec![255, 255, 255, 255, 255, 255, 255, 255, 254]),
108+
"rl0llllllllllll",
109+
);
110+
ser_de_test_len(
111+
Bolt11InvoiceFeatures::from_le_bytes(vec![255, 255, 255, 255, 255, 255, 255, 255, 127]),
112+
"pllllllllllllll",
113+
);
114+
ser_de_test_len(
115+
Bolt11InvoiceFeatures::from_le_bytes(vec![255, 255, 255, 255, 255, 255, 255, 255, 63]),
116+
"llllllllllllll",
117+
);
118+
119+
// To test skipping 0's in deserialization, we have to start with deserialization
120+
assert_eq!(
121+
Bolt11InvoiceFeatures::from_base32(
122+
&"qqqqqry".to_string().chars().map(|c| Fe32::from_char(c).unwrap()).collect::<Vec<_>>()
123+
[..]
124+
)
125+
.unwrap()
126+
.le_flags(),
127+
vec![100]
128+
);
129+
assert_eq!(
130+
Bolt11InvoiceFeatures::from_base32(
131+
&"ry".to_string().chars().map(|c| Fe32::from_char(c).unwrap()).collect::<Vec<_>>()[..]
132+
)
133+
.unwrap()
134+
.le_flags(),
135+
vec![100]
136+
);
96137
}
97138

98139
#[test]

0 commit comments

Comments
 (0)