Prepare moving script types to `primitives` by tcharding · Pull Request #3194 · rust-bitcoin/rust-bitcoin

tcharding · 2024-08-21T01:51:52Z

Move the Script and ScriptBuf types to primitives. There were still a few preparations required, things we had missed while creating the extension traits.

Note also please, in the last patch, we enable hex from the serder feature. This is not the final state we want but like we did for alloc it is a step to reduce the size of the diff.

github-actions · 2024-08-21T02:30:58Z

🚨 API BREAKING CHANGE DETECTED

github-actions · 2024-08-21T02:52:07Z

🚨 API BREAKING CHANGE DETECTED

github-actions · 2024-08-21T03:03:27Z

🚨 API BREAKING CHANGE DETECTED

github-actions · 2024-08-21T03:59:47Z

🚨 API BREAKING CHANGE DETECTED

github-actions · 2024-08-21T04:19:15Z

🚨 API BREAKING CHANGE DETECTED

github-actions · 2024-08-27T00:16:22Z

🚨 API BREAKING CHANGE DETECTED

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

🚨 API BREAKING CHANGE DETECTED

In preparation for moving the `Script` type to `primitives` stop accessing the inner field before doing slice operations, use `as_bytes` to first get at the slice.

In preparation for moving the `ScriptBuf` type to `primitives` stop accessing the inner field when encoding/decoding, use `as_script` and `from_bytes` instead.

The `Builder` is staying in `bitcoin` while the `ScriptBuf` is moving to `primitives`, so we cannot access the inner field of `ScriptBuf`. Use the new `as_byte_vec` hack to mutate the inner `ScriptBuf` field.

The optional dependencies are ordered and separated by whitspace in a manner that may not be obvious (or even have a reason). Some of this is because since use of `?` deps changed name. Put all the optional deps together in alphabetic order.

tcharding · 2024-09-11T01:00:56Z

I'm not exactly sure how you guys are supposed to review this without just trusting that I did indeed cutn'pasta the script impl blocks. I guess you could copy the impl blocks from master locally and check you git index is dirty/clean as expected.

github-actions · 2024-09-11T01:00:59Z

🚨 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.

Kixunil · 2024-09-11T08:53:20Z

without just trusting that I did indeed cutn'pasta the script impl blocks

Here's a secret trick for you and anyone else who wants to review such things:

git config --global diff.colorMoved zebra
git show ...

Speaking of tricks, this is my entire git configuration

~/.gitconfig

[merge]
	conflictstyle = diff3
[diff]
	colorMoved = zebra
[rerere]
	enabled = true
[merge "rust-merge"]
	name = Rust merge
	driver = rust-merge %O %A %B %A

~/.gitattributes

*.rs	merge=rust-merge,diff=rust

apoelstra · 2024-09-11T19:37:44Z

@Kixunil there is an additional option I learned recently. In [diff] you can add

colorMovedWs = allow-indentation-change

and this will even do the move-coloring when the indentation has changed (which would eliminate the need for the silly "dummy module" commits in these PRs).

I don't know when this option was added. I don't think it existed a few years ago.

apoelstra

ACK e234ab6f5d2f5369d1811d539ef595b1df8181de successfully ran local tests

tcharding · 2024-09-12T01:05:45Z

I can't come up with the quite the expression right now, but those config options are a total game changer. I haven't worked out exactly what all the colors mean yet but this is massive. The machine is now doing most of the review instead of us having to do it. Massive.

Kixunil

NACK putting anything deprecated into primitives (&co) at all.

@tcharding suggestion for making the process more efficient: once you see this, immediately drop the top commit and force push the branch without changing hashes, rename the PR to "Prepare moving script types to primitives. Then I'll immediately ACK 8f2f4cb without any review because all those commits are fine. You can then in parallel address the requested changes and make a new PR that only moves (non-deprecated) stuff.

Kixunil · 2024-09-12T10:09:07Z

primitives/src/script/mod.rs

Nope, these two functions should no longer exists in primitives. They are a relic from the time when Script was Sized. Everything these do can and should be achieved using Display.

Kixunil · 2024-09-12T10:17:50Z

primitives/src/script/borrowed.rs

I think/hope pub(crate) won't be needed long term...

In this case pub(super) would have been sufficient.

But even that isn't necessary because the two places that script.0 are called can be replaced with script.as_bytes.

Oh, yeah, Box casting uses unsafe anyway.

Kixunil · 2024-09-12T10:29:29Z

Also this doesn't compile with cargo test --features=serde --no-default-feature but for a different reason than I expected. Which makes me wonder if we should activate hex from serde or rather make it so that things will fail to parse human-readable encodings without hex.

apoelstra · 2024-09-12T15:09:10Z

Which makes me wonder if we should activate hex from serde or rather make it so that things will fail to parse human-readable encodings without hex.

I think we should just activate hex from serde. It's hard for me to imagine somebody being willing to take a massive macro-filled serde dep but unwilling to take a hex-conservative dep, and it makes our lives much easier to eliminate feature combinations.

~~I should investigate though why my local CI failed to catch this. It should be testing all single-feature combinations.~~ Which crate doesn't compile? I tried bitcoin, primitives and the root and they seem to be ok.

Kixunil · 2024-09-12T19:42:04Z

There's another reason to activate hex: almost-silently failing serialization because of accidentally missing feature is kinda foot-gun. It's not as terrible as some other things but it is concerning at least.

Oh, crap, I had primitives/src/script.rs lying around and didn't notice this error is unlike others.

But now I see deprecation warnings.

tcharding · 2024-09-12T20:13:27Z

Removed final patch as suggested, thanks for putting thought into how we can speed up the process @Kixunil

Kixunil

ACK 8f2f4cb

apoelstra

ACK 8f2f4cb successfully ran local tests

d649c06 Move script types to primitives (Tobin C. Harding) ec46359 Inline bytes_to_asm_fmt into Script::Display impl (Tobin C. Harding) Pull request description: First patch removes `bytes_to_asm_fmt` as requested by Kix here: #3194 (comment) Second patch does the move. The move is minimal but there is quite a bit of code moved in `script/mod.rs` - I believe it is as minimal as required as well. ACKs for top commit: apoelstra: ACK d649c06 successfully ran local tests Tree-SHA512: 329a23948ac5617402a724b734d81cde8ab1f57ddd4860f858880618e260ea8b5cc89315de1fd93ae32787d5e8508fd604a41f003b1f5772a773b5b1648d382c

tcharding mentioned this pull request Aug 21, 2024

Move Script and ScriptBuf to primitives #3143

Closed

github-actions bot added C-bitcoin PRs modifying the bitcoin crate test C-primitives labels Aug 21, 2024

tcharding force-pushed the 08-21-mv-script branch 6 times, most recently from 15d7d50 to 1044774 Compare August 21, 2024 02:29

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

tcharding force-pushed the 08-21-mv-script branch from 1044774 to bf40359 Compare August 21, 2024 02:50

tcharding marked this pull request as draft August 21, 2024 02:50

tcharding force-pushed the 08-21-mv-script branch from bf40359 to 4d5121d Compare August 21, 2024 03:01

tcharding force-pushed the 08-21-mv-script branch from 4d5121d to 0c828ce Compare August 21, 2024 03:58

tcharding mentioned this pull request Aug 21, 2024

primitives: Do not enable additional features in serde feature #3197

Closed

tcharding force-pushed the 08-21-mv-script branch from 0c828ce to f1729f2 Compare August 21, 2024 04:17

tcharding mentioned this pull request Aug 21, 2024

Run CI for primitives #3195

Closed

tcharding marked this pull request as ready for review August 21, 2024 06:49

tcharding marked this pull request as draft August 21, 2024 06:50

tcharding force-pushed the 08-21-mv-script branch 3 times, most recently from 729cea4 to 126cfcf Compare August 27, 2024 00:15

tcharding added 4 commits September 11, 2024 10:50

Use Script::as_bytes instead of inner when indexing

8b82363

In preparation for moving the `Script` type to `primitives` stop accessing the inner field before doing slice operations, use `as_bytes` to first get at the slice.

Stop accessing inner ScriptBuf field when encoding

900af45

In preparation for moving the `ScriptBuf` type to `primitives` stop accessing the inner field when encoding/decoding, use `as_script` and `from_bytes` instead.

Do not access ScriptBuf inner from builder

95f2a8d

The `Builder` is staying in `bitcoin` while the `ScriptBuf` is moving to `primitives`, so we cannot access the inner field of `ScriptBuf`. Use the new `as_byte_vec` hack to mutate the inner `ScriptBuf` field.

Re-order optional dependencies

8f2f4cb

The optional dependencies are ordered and separated by whitspace in a manner that may not be obvious (or even have a reason). Some of this is because since use of `?` deps changed name. Put all the optional deps together in alphabetic order.

tcharding force-pushed the 08-21-mv-script branch from f73dd77 to e234ab6 Compare September 11, 2024 00:58

github-actions bot removed the C-internals PRs modifying the internals crate label Sep 11, 2024

tcharding marked this pull request as ready for review September 11, 2024 01:01

tcharding changed the title ~~Move script types to primitives~~ priority: Move script types to primitives Sep 11, 2024

apoelstra previously approved these changes Sep 12, 2024

View reviewed changes

Kixunil requested changes Sep 12, 2024

View reviewed changes

tcharding dismissed apoelstra’s stale review via 8f2f4cb September 12, 2024 20:12

tcharding force-pushed the 08-21-mv-script branch from e234ab6 to 8f2f4cb Compare September 12, 2024 20:12

github-actions bot removed the test label Sep 12, 2024

tcharding changed the title ~~priority: Move script types to primitives~~ priority: Prepare moving script types to primitives Sep 12, 2024

Kixunil approved these changes Sep 13, 2024

View reviewed changes

apoelstra approved these changes Sep 13, 2024

View reviewed changes

apoelstra merged commit 60e15b8 into rust-bitcoin:master Sep 13, 2024

tcharding deleted the 08-21-mv-script branch September 18, 2024 01:57

tcharding mentioned this pull request Oct 1, 2024

Move script types to primitives #3431

Merged

tcharding changed the title ~~priority: Prepare moving script types to primitives~~ Prepare moving script types to primitives Dec 4, 2024

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 27, 2024

Uh oh!

github-actions bot commented Aug 27, 2024

Uh oh!

tcharding commented Sep 11, 2024

Uh oh!

github-actions bot commented Sep 11, 2024

Uh oh!

Kixunil commented Sep 11, 2024

Uh oh!

apoelstra commented Sep 11, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 12, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

Kixunil Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

Kixunil Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Sep 12, 2024

Uh oh!

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

Uh oh!

Uh oh!

Kixunil commented Sep 12, 2024

Uh oh!

tcharding commented Sep 12, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Aug 21, 2024 •

edited

Loading

apoelstra commented Sep 12, 2024 •

edited

Loading