From d71d7b5e9b1f7811adc5bfa3f768307043e68f68 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 24 Jul 2026 21:04:41 +0000 Subject: [PATCH 1/3] CheckedHrpString: remove validate_segwit method This method is weirdly confused. It does some, but not all, of the checks required for a CheckedHrpString to be a valid Segwit string. Some of the checks it *does* do (in particular, the empty data and maximum length checks) are redundant with checks done in SegwitHrpString::new. It does not check that the witness version is a valid witness version. But worse, it *cannot* check that the witness version matches the checksum, because CheckedHrpString does not keep track of the checksum that was used when constructing it, and Segwit uses a different checksum depending on its version number. It appears that this method is a helper method for SegwitHrpString::new which got into the public API (by accident?). But it doesn't make sense for the public to call it, and it doesn't even really make sense for SegwitHrpString::new to call it, since it does overlapping checks so that inlining it turns out to be only 3-4 lines of code. It was also called by SegwitHrpString::new_bech32, a very weird method which is covered in "do not use this" warnings but which is apparently needed for old PSBT users or something. This method lacks several of the checks of SegwitHrpString::new, including the length checks. To fix this, we refactor SegwitHrpString::new into a new `new_internal` method which has a boolean flag to override the bech32/bech32m switch, and then both SegwitHrpString::new and SegwitHrpString::new_bech32 can just pass through to this. So the result is a net reduction in code, despite an increase in the number of checks that we're doing. The next commit will delete even more code, since a consequence of this refactor is that the CheckedHrpString::hrpstring_length field is no longer used. This mess originates in https://github.com/rust-bitcoin/rust-bech32/pull/117 by Tobin -- but in his defense, he was implementing a huge feature and there was a ton of review iteration, so it's unsurprising that some weirdness slipped in. --- src/primitives/decode.rs | 63 ++++++++++++---------------------------- 1 file changed, 19 insertions(+), 44 deletions(-) diff --git a/src/primitives/decode.rs b/src/primitives/decode.rs index e8a67326..8cd8b4ae 100644 --- a/src/primitives/decode.rs +++ b/src/primitives/decode.rs @@ -444,27 +444,6 @@ impl<'s> CheckedHrpstring<'s> { ByteIter { iter: AsciiToFe32Iter { iter: self.ascii.iter().copied() }.fes_to_bytes() } } - /// Converts this type to a [`SegwitHrpstring`] after validating the witness and HRP. - #[inline] - pub fn validate_segwit(mut self) -> Result, SegwitHrpstringError> { - if self.ascii.is_empty() { - return Err(SegwitHrpstringError::NoData); - } - - if self.hrpstring_length > segwit::MAX_STRING_LENGTH { - return Err(SegwitHrpstringError::TooLong(self.hrpstring_length)); - } - - // Unwrap ok since check_characters checked the bech32-ness of this char. - let witness_version = Fe32::from_char(self.ascii[0].into()).unwrap(); - self.ascii = &self.ascii[1..]; // Remove the witness version byte. - - self.validate_segwit_padding()?; - self.validate_witness_program_length(witness_version)?; - - Ok(SegwitHrpstring { hrp: self.hrp(), witness_version, ascii: self.ascii }) - } - /// Validates the segwit padding rules. /// /// Must be called after the witness version byte is removed from the data part. @@ -544,15 +523,7 @@ pub struct SegwitHrpstring<'s> { } impl<'s> SegwitHrpstring<'s> { - /// Parses an HRP string, treating the first data character as a witness version. - /// - /// The version byte does not appear in the extracted binary data, but is covered by the - /// checksum. It can be accessed with [`Self::witness_version`]. - /// - /// NOTE: We do not enforce any restrictions on the HRP, use [`SegwitHrpstring::has_valid_hrp`] - /// to get strict BIP conformance (also [`Hrp::is_valid_on_mainnet`] and friends). - #[inline] - pub fn new(s: &'s str) -> Result { + fn new_internal(s: &'s str, force_bech32: bool) -> Result { let len = s.len(); if len > segwit::MAX_STRING_LENGTH { return Err(SegwitHrpstringError::TooLong(len)); @@ -572,14 +543,28 @@ impl<'s> SegwitHrpstring<'s> { return Err(SegwitHrpstringError::InvalidWitnessVersion(witness_version)); } - let checked: CheckedHrpstring<'s> = match witness_version { - VERSION_0 => unchecked.validate_and_remove_checksum::()?, + let mut checked: CheckedHrpstring<'s> = match (force_bech32, witness_version) { + (true, _) | (false, VERSION_0) => unchecked.validate_and_remove_checksum::()?, _ => unchecked.validate_and_remove_checksum::()?, }; + checked.ascii = &checked.ascii[1..]; // Remove the witness version byte. - checked.validate_segwit() + // Do additional segwit-specific checks. + checked.validate_segwit_padding()?; + checked.validate_witness_program_length(witness_version)?; + Ok(SegwitHrpstring { hrp: checked.hrp(), witness_version, ascii: checked.ascii }) } + /// Parses an HRP string, treating the first data character as a witness version. + /// + /// The version byte does not appear in the extracted binary data, but is covered by the + /// checksum. It can be accessed with [`Self::witness_version`]. + /// + /// NOTE: We do not enforce any restrictions on the HRP, use [`SegwitHrpstring::has_valid_hrp`] + /// to get strict BIP conformance (also [`Hrp::is_valid_on_mainnet`] and friends). + #[inline] + pub fn new(s: &'s str) -> Result { Self::new_internal(s, false) } + /// Parses an HRP string, treating the first data character as a witness version. /// /// ## WARNING @@ -594,17 +579,7 @@ impl<'s> SegwitHrpstring<'s> { /// [BIP-350]: https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki #[inline] pub fn new_bech32(s: &'s str) -> Result { - let unchecked = UncheckedHrpstring::new(s)?; - let data_part = unchecked.data_part_ascii(); - - // Unwrap ok since check_characters (in `Self::new`) checked the bech32-ness of this char. - let witness_version = Fe32::from_char(data_part[0].into()).unwrap(); - if witness_version.to_u8() > 16 { - return Err(SegwitHrpstringError::InvalidWitnessVersion(witness_version)); - } - - let checked = unchecked.validate_and_remove_checksum::()?; - checked.validate_segwit() + Self::new_internal(s, true) } /// Returns `true` if the HRP is "bc" or "tb". From d1f8a868958d34a91a2bb1da09ae005c3363e881 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 24 Jul 2026 21:23:17 +0000 Subject: [PATCH 2/3] primitives: remove unused CheckedHrpString::hrpstring_length field This was only used by the broken segwit validation functions, which were removed in the last commit. --- src/primitives/decode.rs | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/src/primitives/decode.rs b/src/primitives/decode.rs index 8cd8b4ae..82b7521a 100644 --- a/src/primitives/decode.rs +++ b/src/primitives/decode.rs @@ -293,11 +293,7 @@ impl<'s> UncheckedHrpstring<'s> { pub fn remove_checksum(self) -> CheckedHrpstring<'s> { let end = self.data_part_ascii.len() - Ck::CHECKSUM_LENGTH; - CheckedHrpstring { - hrp: self.hrp(), - ascii: &self.data_part_ascii[..end], - hrpstring_length: self.hrpstring_length, - } + CheckedHrpstring { hrp: self.hrp(), ascii: &self.data_part_ascii[..end] } } } @@ -332,8 +328,6 @@ pub struct CheckedHrpstring<'s> { /// /// The characters after the '1' separator and before the checksum. ascii: &'s [u8], - /// The length of the parsed hrpstring. - hrpstring_length: usize, // Guaranteed to be <= CK::CODE_LENGTH } impl<'s> CheckedHrpstring<'s> { @@ -1258,16 +1252,13 @@ mod tests { }; assert_eq!(unchecked_too_large.witness_version(), None); - let checked_empty = - CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"", hrpstring_length: 3 }; + let checked_empty = CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"" }; assert_eq!(checked_empty.witness_version(), None); - let checked_valid = - CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"s", hrpstring_length: 4 }; + let checked_valid = CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"s" }; assert_eq!(checked_valid.witness_version(), Some(Fe32(16))); - let checked_too_large = - CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"3", hrpstring_length: 4 }; + let checked_too_large = CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"3" }; assert_eq!(checked_too_large.witness_version(), None); } @@ -1288,11 +1279,8 @@ mod tests { #[test] fn validate_segwit_padding() { - let checked = |ascii: &'static [u8]| CheckedHrpstring { - hrp: Hrp::parse_unchecked("bc"), - ascii, - hrpstring_length: ascii.len() + 3, - }; + let checked = + |ascii: &'static [u8]| CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii }; // padding_len = 1 assert_eq!(checked(b"qqqqq").validate_segwit_padding(), Ok(())); @@ -1320,11 +1308,7 @@ mod tests { assert_eq!(fe_iter.next(), Some(Fe32::Q)); assert_eq!(fe_iter.size_hint(), (2, Some(2))); - let checked = CheckedHrpstring { - hrp: Hrp::parse_unchecked("bc"), - ascii: b"qqqqqqqq", - hrpstring_length: 11, - }; + let checked = CheckedHrpstring { hrp: Hrp::parse_unchecked("bc"), ascii: b"qqqqqqqq" }; let mut byte_iter = checked.byte_iter(); assert_eq!(byte_iter.size_hint(), (5, Some(5))); assert_eq!(byte_iter.next(), Some(0)); From cfded00edd0cce79db020b09b65d671d29aba5af Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 27 Jul 2026 18:52:37 +0000 Subject: [PATCH 3/3] add regression test for #291 --- src/segwit.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/segwit.rs b/src/segwit.rs index 60397b0d..bb1bde27 100644 --- a/src/segwit.rs +++ b/src/segwit.rs @@ -651,4 +651,7 @@ mod tests { assert_eq!(upper_writer, upper.as_bytes()); } } + + #[test] + fn segwit_bech32_no_data() { let _ = SegwitHrpstring::new_bech32("bc1"); } }