Skip to content

Rename min_bytes_needed to read_limit#5107

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
nyonson:rename-min-bytes
Oct 10, 2025
Merged

Rename min_bytes_needed to read_limit#5107
apoelstra merged 2 commits intorust-bitcoin:masterfrom
nyonson:rename-min-bytes

Conversation

@nyonson
Copy link
Copy Markdown
Contributor

@nyonson nyonson commented Oct 8, 2025

The min_bytes_needed name is a bit of a misnomer because it implies a lower bound instead of an upper. I believe the original intention was to capture "minimum bytes needed to advance the state of the decoder", but that is a little wordy. Updated to read_limit since this is an upper bound to protect against over consuming bytes the decoder doesn't know what to do with.

@github-actions github-actions bot added C-units PRs modifying the units crate C-consensus_encoding PRs modifying the consensus-encoding crate test labels Oct 8, 2025
tcharding
tcharding previously approved these changes Oct 8, 2025
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 65d6b78

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Oct 8, 2025

nit: The second patch should not have consensus_encoding: in the commit brief message because it does not touch consensus_encoding

@nyonson
Copy link
Copy Markdown
Contributor Author

nyonson commented Oct 8, 2025

Ah dang, I have totally been messing that up.

@tcharding
Copy link
Copy Markdown
Member

Grab you box and out the building by 3, your fired.

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 7251d0c; successfully ran local tests; great name

apoelstra
apoelstra previously approved these changes Oct 9, 2025
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 8a94ec7; successfully ran local tests; great name

@apoelstra
Copy link
Copy Markdown
Member

Gr, that second ack was supposed to be a merge.

@apoelstra
Copy link
Copy Markdown
Member

Needs rebase now, sorry. (I think this one is gonna be a bit of a PITA to get in while we're doing any other decoder work.)

The `min_bytes_needed` name is a bit of a misnomer because it implies a
lower bound instead of an upper. I believe the original intention was
to capture "minimum bytes needed to advance the state of the decoder",
but that is a little wordy. Updated to `read_limit` since this is an
upper bound to protect against over consuming bytes the decoder doesn't
know what to do with.
@nyonson nyonson dismissed stale reviews from apoelstra and tcharding via 38b47c7 October 9, 2025 17:03
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 38b47c7; successfully ran local tests

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 38b47c7

@apoelstra apoelstra merged commit 3a94e06 into rust-bitcoin:master Oct 10, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-consensus_encoding PRs modifying the consensus-encoding crate C-units PRs modifying the units crate test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants