Skip to content

fix: mixed-case validation in hrp::parse_display#277

Merged
apoelstra merged 2 commits into
rust-bitcoin:masterfrom
alexgrad42:fix/hrp_parse_display_mixed_case
Jun 24, 2026
Merged

fix: mixed-case validation in hrp::parse_display#277
apoelstra merged 2 commits into
rust-bitcoin:masterfrom
alexgrad42:fix/hrp_parse_display_mixed_case

Conversation

@alexgrad42

Copy link
Copy Markdown
Contributor

Move has_lower and has_upper flags from local variables inside write_str to fields on the ByteFormatter struct. This ensures that case validation persists across multiple write_str calls (e.g., when processing split Display arguments) instead of resetting on each boundary.

Fixes: #276

@tcharding

tcharding commented Jun 17, 2026

Copy link
Copy Markdown
Member

Needs unit test. Done as a separate patch last so review can verify the bug this fixes.

@alexgrad42 alexgrad42 force-pushed the fix/hrp_parse_display_mixed_case branch from 81cbcd5 to e2d6c15 Compare June 17, 2026 11:32
@alexgrad42

Copy link
Copy Markdown
Contributor Author

I've extracted the test into separate commit, please check

@tcharding

Copy link
Copy Markdown
Member

Thanks, still a couple things to fix please:

  • The commit brief log (the title) should not be the same for both commits
  • The commit that adds the tests has to be second so that test pass on all commits. The during review someone will re-order the commits and check that the test fails without the fix.

Thanks

@tcharding

Copy link
Copy Markdown
Member

I'm reasonably sure this was mentioned to you on another PR. I'm also still uncertain how much of you work is human and how much LLM. If you are using an LLM you will need to configure it to remember stuff we mention in review otherwise it waists review time. Of course simply forgetting things if you are actually human is ok.

@tcharding

Copy link
Copy Markdown
Member

On further thought the tests are probably too verbose. Why not just use the one in the issue (witthout all the code comments).

@alexgrad42 alexgrad42 force-pushed the fix/hrp_parse_display_mixed_case branch from e2d6c15 to d001d1e Compare June 18, 2026 06:26
Move `has_lower` and `has_upper` flags from local variables inside
`write_str` to fields on the `ByteFormatter` struct to ensure
that case validation persists across multiple `write_str` calls

Fixes: rust-bitcoin#276
Move `has_lower` and `has_upper` flags from local variables inside
`write_str` to fields on the `ByteFormatter` struct to ensure
that case validation persists across multiple `write_str` calls

This commit only adds unit test to reproduce the issue, no real fix

Fixes: rust-bitcoin#276
@alexgrad42 alexgrad42 force-pushed the fix/hrp_parse_display_mixed_case branch from d001d1e to a5312e7 Compare June 18, 2026 07:37
@alexgrad42

Copy link
Copy Markdown
Contributor Author

I have rearranged the commit and cleaned the test. Note that example from Loupe in fact doesn't work, I've added a case to show this.

@apoelstra apoelstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a5312e7; successfully ran local tests

@apoelstra apoelstra merged commit b4221c3 into rust-bitcoin:master Jun 24, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hrp::parse_display accepts mixed-case input across write_str boundaries

3 participants