Skip to content

Remove length prefix from SliceEncoder#5108

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:push-turwlxrwwxmp
Oct 10, 2025
Merged

Remove length prefix from SliceEncoder#5108
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:push-turwlxrwwxmp

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Oct 8, 2025

Currently we hide the length prefix inside the SliceEncoder. While technically correct the usage of the SliceEncoder can be made more clear by forcing users to combine a compact size encoder and a slice encoder using an Encoder2 instead of the current abstraction.

Original idea from jrakibi in #5104. I gave you a co-developed-by tag mate. Pushing this up because I'm itching to get primitives 1.0.0-rc.0 out.

@tcharding tcharding changed the title Remove length prefix from SliceEncoder Remove length prefix from SliceEncoder Oct 8, 2025
@github-actions github-actions bot added C-consensus_encoding PRs modifying the consensus-encoding crate test C-primitives labels Oct 8, 2025
Copy link
Copy Markdown
Contributor

@jrakibi jrakibi left a comment

Choose a reason for hiding this comment

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

ACK ed02a0b

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 ed02a0b; successfully ran local tests; but needs rebase

@apoelstra
Copy link
Copy Markdown
Member

Needs rebase.

Currently we hide the length prefix inside the `SliceEncoder`. While
technically correct the usage of the `SliceEncoder` can be made more
clear by forcing users to combine a compact size encoder and a slice
encoder using an `Encoder2` instead of the current abstraction.

Co-developed-by: jrakabi <j.errakibi@gmail.com>
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 63bbe12; successfully ran local tests

@apoelstra
Copy link
Copy Markdown
Member

Gonna merge this -- the diff is not substantially different from @jrakibi's previous ACK.

@apoelstra apoelstra merged commit 7d910cd 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-primitives test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants