Remove `VarInt` and use `ReadExt` and `WriteExt` trait methods instead by tcharding · Pull Request #2931 · rust-bitcoin/rust-bitcoin

tcharding · 2024-06-27T05:27:48Z

This the meat and potatoes out of Steven's work in #2133 and also closes #1016

tcharding · 2024-06-27T05:28:03Z

CC @stevenroose, I picked up your work man, hope you don't mind.

tcharding · 2024-06-27T05:31:11Z

Might close #1016

tcharding · 2024-06-27T05:47:17Z

Name might be wrong, converting to draft to work it out. ref : #1016 (comment)

Kixunil · 2024-06-27T11:27:48Z

bitcoin/src/blockdata/witness.rs

The 9fe420d2210bfcfab4d0913c9dd94f633496f95a commit message makes it sound like getting the length is impossible without returning it. That's a complete BS.

Instead the message should be something like "Returning usize is more convenient for consumers that need to add up the length of multiple encoded items."

Kixunil · 2024-06-27T11:31:01Z

bitcoin/src/consensus/encode.rs

This should be ToU64 which was added in fc03492d8d45a54da0567db8c46e6f84cb5d0148

Ugh, sloppy work by me.

Kixunil · 2024-06-27T11:32:45Z

bitcoin/src/consensus/encode.rs

Not const, it should say varint_size_u64.

Kixunil · 2024-06-27T11:38:02Z

bitcoin/src/consensus/encode.rs

Alternative design: add varint_size method to ToU64, make this accept u64 and make it const, delete varint_size_const. Then users will be able to call .varint_size on integers. (In that case, the trait must be public and stable, not private/internals as I suggested elsewhere.) There's potential silliness that some other crate will define consensus-encoding-related method. We could have NumExt: ToU64 trait to move it away.

You can't make trait methods const.

I mean make this fn const by being non-generic and use methods for convenience instead.

Flagging that I did not action this.

github-actions · 2024-08-06T01:54:53Z

🚨 API BREAKING CHANGE DETECTED

github-actions · 2024-08-06T02:45:17Z

🚨 API BREAKING CHANGE DETECTED

tcharding · 2024-08-27T02:16:10Z

Rebased, added a new final patch, and re-wrote PR description.

github-actions · 2024-08-27T02:16:25Z

🚨 API BREAKING CHANGE DETECTED

tcharding · 2024-08-27T02:26:08Z

Perhaps we should instead use get_size_of_comact_size to mirror GetSizeOfCompactSize from Core

ref: https://github.com/bitcoin/bitcoin/blob/37cdb5f2483cd57077497925caf69a82b0d39471/src/serialize.h#L295C31-L295C35

github-actions · 2024-09-17T23:05:20Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2024-09-17T23:06:07Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra · 2024-09-18T18:27:13Z

Needs rebase

tcharding · 2024-09-18T21:29:02Z

Rebased, then got lost fixing my emacs, not sure if I touched anything other than rebase but I don't think so.

github-actions · 2024-09-18T21:31:22Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra · 2024-09-20T15:02:20Z

Will ACK. FWIW I'm happy with this API and would be willing to pull it out of internals (though I guess it'd most sensibly live with ReadExt/WriteExt which also don't have homes yet).

apoelstra

ACK 1599db253574a97191ee63146ce57040845800e1 successfully ran local tests; nice, though sad that we need a separate non-generic compact_size_const method

tcharding · 2024-09-20T17:39:50Z

though sad that we need a separate non-generic compact_size_const method

Agreed.

tcharding · 2024-09-22T21:01:33Z

Re-based, during which arbitrary import moved locations, I think that is why the range-diff shows changed lines.

github-actions · 2024-09-22T21:02:14Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2024-09-22T23:33:45Z

Needs #3395

apoelstra

ACK b58dd27c42d56df8a3e05ff719022e240ea0ff81 successfully ran local tests; nice, though sad that we need a separate non-generic compact_size_const method

tcharding · 2024-09-23T23:47:32Z

lol, amusingly, your re-post of the 'sad that we ...' confused me, I was sure I'd read that before :)

apoelstra · 2024-09-24T00:13:06Z

Yeah :) I'm not sure what the best way for me to "refresh an ACK" with a nontrivial message is. I want to repeat the message so it shows up in the commit message, but it's weird to read for people on the PR.

tcharding · 2024-09-24T04:20:02Z

Oh, that is ok, now I know it won't surprise me.

We would like to add a `emit_varint` function, however doing so requires that we can get access to the length of a slice when we are encoding it so we can use `emit_slice` to implement `emit_varint`. It would be easier to do so if `emit_slice` returned the length of the slice. In preparation for adding `emit_varint` (and removing the `VarInt` type) return the encoded length of a slice from `WriteExt::emit_slice`. (Patch originally written by Steven, cherry-pick and patch description written by Tobin.) Co-developed-by: Tobin C. Harding <me@tobin.cc>

At some stage we named the compact encoding `VarInt` (which makes sense because the compact size encoding is a variable length integer encoding). However it turns out the term "varint" is used in Core for a different encoding so this may lead to confusion. While we fix this naming thing observe also that the `VarInt` type is unnecessarily complicated, all we need to be able to do is encode and decode integers in compact form as specified by Core. We can do this simply by extending our `WriteExt` and `ReadExt` traits. Add `emit_compact_size` and `read_compact_size` to emit and read compact endcodings respectively. Includes addition of `internals::compact_size::encoded_size_const`. Patch originally written by Steven, Tobin cherry-picked and did a bunch of impovements after the varint vs compact_size thing (rust-bitcoin#1016). ref: https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer Co-developed-by: Tobin C. Harding <me@tobin.cc>

github-actions · 2024-09-24T21:11:40Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra

ACK 18d8b0e successfully ran local tests

tcharding · 2024-09-29T22:42:05Z

I rekon we can merge this, all @Kixunil's concerns from last month have been either addressed or commented on.

tcharding mentioned this pull request Jun 27, 2024

encode: Replace VarInt type with ReadExt and WriteExt functions #2133

Closed

tcharding marked this pull request as draft June 27, 2024 05:47

github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-internals PRs modifying the internals crate C-io PRs modifying the io crate C-base58 PRs modifying the base58 crate labels Jun 27, 2024

Kixunil reviewed Jun 27, 2024

View reviewed changes

tcharding mentioned this pull request Jun 27, 2024

Introduce ToU64 trait #2929

Merged

tcharding force-pushed the 06-27-var-int branch from 355a8fb to 4b142f8 Compare August 6, 2024 01:53

github-actions bot removed C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-io PRs modifying the io crate C-base58 PRs modifying the base58 crate labels Aug 6, 2024

github-actions bot added the API break This PR requires a version bump for the next release label Aug 6, 2024

tcharding force-pushed the 06-27-var-int branch from 4b142f8 to acc547f Compare August 6, 2024 02:44

apoelstra mentioned this pull request Aug 14, 2024

upgrade to bitcoin 0.32 ElementsProject/rust-elements#209

Merged

tcharding mentioned this pull request Aug 22, 2024

VarInt in this project is CompactSize in Bitcoin Core #1016

Closed

tcharding force-pushed the 06-27-var-int branch from acc547f to 5d5ea04 Compare August 27, 2024 02:15

github-actions bot removed the C-internals PRs modifying the internals crate label Aug 27, 2024

tcharding marked this pull request as ready for review August 27, 2024 02:16

tcharding force-pushed the 06-27-var-int branch 2 times, most recently from 8b387ae to ae96162 Compare September 17, 2024 23:03

tcharding marked this pull request as ready for review September 17, 2024 23:04

tcharding force-pushed the 06-27-var-int branch from ae96162 to 1599db2 Compare September 18, 2024 21:28

apoelstra previously approved these changes Sep 20, 2024

View reviewed changes

tcharding dismissed apoelstra’s stale review via b58dd27 September 22, 2024 21:00

tcharding force-pushed the 06-27-var-int branch from 1599db2 to b58dd27 Compare September 22, 2024 21:00

apoelstra approved these changes Sep 23, 2024

View reviewed changes

stevenroose added 2 commits September 25, 2024 06:56

tcharding force-pushed the 06-27-var-int branch from b58dd27 to 18d8b0e Compare September 24, 2024 20:56

apoelstra approved these changes Sep 25, 2024

View reviewed changes

apoelstra merged commit dfa8692 into rust-bitcoin:master Sep 30, 2024

tcharding deleted the 06-27-var-int branch October 14, 2024 22:18

tcharding mentioned this pull request Aug 6, 2025

Remove usage of term VarInt #4784

Closed

Conversation

tcharding commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 27, 2024

Uh oh!

tcharding commented Jun 27, 2024

Uh oh!

tcharding commented Jun 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 6, 2024

Uh oh!

github-actions bot commented Aug 6, 2024

Uh oh!

tcharding commented Aug 27, 2024

Uh oh!

github-actions bot commented Aug 27, 2024

Uh oh!

tcharding commented Aug 27, 2024

Uh oh!

github-actions bot commented Sep 17, 2024

Uh oh!

github-actions bot commented Sep 17, 2024

Uh oh!

apoelstra commented Sep 18, 2024

Uh oh!

tcharding commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 18, 2024

Uh oh!

apoelstra commented Sep 20, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 20, 2024

Uh oh!

tcharding commented Sep 22, 2024

Uh oh!

github-actions bot commented Sep 22, 2024

Uh oh!

tcharding commented Sep 22, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 23, 2024

Uh oh!

apoelstra commented Sep 24, 2024

Uh oh!

tcharding commented Sep 24, 2024

Uh oh!

github-actions bot commented Sep 24, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 29, 2024

Uh oh!

Reviewers

Assignees

tcharding commented Jun 27, 2024 •

edited

Loading

tcharding commented Sep 18, 2024 •

edited

Loading