Remove CheckedHrpString::validate_segwit method - #288
Merged
apoelstra merged 3 commits intoJul 30, 2026
Conversation
tcharding
approved these changes
Jul 27, 2026
Member
|
diffstat |
apoelstra
force-pushed
the
2026-07/segwit-validate
branch
from
July 27, 2026 16:33
db43371 to
d661b2c
Compare
Member
Author
|
Fixed formatting (sorry) |
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 rust-bitcoin#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.
This was only used by the broken segwit validation functions, which were removed in the last commit.
apoelstra
force-pushed
the
2026-07/segwit-validate
branch
from
July 27, 2026 18:53
d661b2c to
9e69b81
Compare
Member
Author
|
Added regression test for #291 which this apparently fixes too. |
apoelstra
force-pushed
the
2026-07/segwit-validate
branch
from
July 27, 2026 19:05
9e69b81 to
cfded00
Compare
tcharding
approved these changes
Jul 27, 2026
Member
Author
|
On cfded00 successfully ran local tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This appears to be a helper function for
SegwitHrpString::newwhich shouldn't have been in the public API. In fact, it's not even very useful in the private API -- once you remove all the redundant checks then it's only 3-4 lines of code which are better off inlined intoSegwitHrpString::new.Fixes #274
Fixes #291