Skip to content

Remove binary codec sv2#1952

Merged
plebhash merged 7 commits into
stratum-mining:mainfrom
Shourya742:2025-10-16-remove-binary-codec-sv2
Oct 20, 2025
Merged

Remove binary codec sv2#1952
plebhash merged 7 commits into
stratum-mining:mainfrom
Shourya742:2025-10-16-remove-binary-codec-sv2

Conversation

@Shourya742
Copy link
Copy Markdown
Member

@Shourya742 Shourya742 commented Oct 16, 2025

closes: #1462

@plebhash
Copy link
Copy Markdown
Member

#1860 is about to deprecate the ping-pong examples

I guess we already found a workaround for any CI-blocking issues here, but just to point out that if they cause problems again, we don't need to spend too much time trying to find clever solutions

Comment thread protocols/v2/binary-sv2/src/codec/encodable.rs
@plebhash
Copy link
Copy Markdown
Member

we shouldn't be slipping commits titled something something to PRs

bc96abf

Copy link
Copy Markdown
Member

@plebhash plebhash Oct 16, 2025

Choose a reason for hiding this comment

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

we'll need to bump MAJOR

$ cargo +stable semver-checks
    Building binary_sv2 v4.0.0 (current)
       Built [   2.407s] (current)
     Parsing binary_sv2 v4.0.0 (current)
      Parsed [   0.006s] (current)
    Building binary_sv2 v4.0.0 (baseline)
       Built [   2.129s] (baseline)
     Parsing binary_sv2 v4.0.0 (baseline)
      Parsed [   0.002s] (baseline)
    Checking binary_sv2 v4.0.0 -> v4.0.0 (no change)
     Checked [   0.003s] 95 checks: 94 pass, 1 fail, 0 warn, 0 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.37.0/src/lints/function_missing.ron

Failed in:
  function binary_sv2::clone_message, previously in file /Users/plebhash/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/binary_sv2-4.0.0/src/lib.rs:25

     Summary semver requires new major version: 1 major and 0 minor checks failed

@Shourya742 Shourya742 force-pushed the 2025-10-16-remove-binary-codec-sv2 branch 5 times, most recently from d8f5af7 to 8b07ddd Compare October 17, 2025 08:39
@Shourya742
Copy link
Copy Markdown
Member Author

we shouldn't be slipping commits titled something something to PRs

bc96abf

That slipped, I wanted to rebase it to other commit in the history.

@Shourya742
Copy link
Copy Markdown
Member Author

@plebhash whats your opinion on the rigidity?

Comment thread examples/ping-pong-encrypted/src/messages.rs
Comment thread examples/ping-pong/src/messages.rs
Comment thread protocols/v2/binary-sv2/derive_codec/src/lib.rs Outdated
Comment thread protocols/v2/binary-sv2/derive_codec/src/lib.rs Outdated
Comment thread protocols/v2/channels-sv2/Cargo.toml Outdated
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.

Shouldn't we bump PATCH here?

cc @plebhash

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Patch would be required as we updated its dependency, good point.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As API's are not changing, so we don't need to update major and minor versions.

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.

hmm idk

from the perspective of someone who's using channels_sv2, does it really matter where the internal implementation details are coming from?

are we changing the behavior of anything, or just moving things around?

maybe I have some blindspot, but my first instinct here would not to bump anything

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.

If we don't bump PATCH here we won't update crates published to crates.io during release.

So we would keep having the old binary_sv2 on those published crates.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will bump the patch for them, in the same PR.

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.

@GitGab19 @Shourya742 we should be careful in extrapolating this into an umbrella rule where we always bump PATCH just because some dependencies were updated

handlers_sv2 is now at v0.2.1 on this repo, while the published version is still at v0.1.0... so v0.2.0 will never be published

it doesn't make sense to bump PATCH for something that hasn't been published yet

Copy link
Copy Markdown
Member

@plebhash plebhash Oct 22, 2025

Choose a reason for hiding this comment

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

channels_sv2 is also at v2.0.1, while the last published version is at v1.0.2, so v2.0.0 will never be published

Copy link
Copy Markdown
Member

@plebhash plebhash Oct 22, 2025

Choose a reason for hiding this comment

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

codec_sv2 is also at v4.0.1 while the last published version is at v3.0.1, so v4.0.0 will never be published

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Shoot, I didn't looked at the crates.io versions for them. Thanks for pointing it out.

Comment thread protocols/v2/roles-logic-sv2/Cargo.toml Outdated
Comment thread protocols/v2/subprotocols/common-messages/Cargo.toml Outdated
Comment thread protocols/v2/subprotocols/job-declaration/Cargo.toml Outdated
Comment thread protocols/v2/subprotocols/mining/Cargo.toml Outdated
Comment thread protocols/v2/subprotocols/template-distribution/Cargo.toml Outdated
Comment thread examples/ping-pong-encrypted/Cargo.toml Outdated
Comment thread examples/ping-pong-encrypted/Cargo.toml Outdated
Comment thread examples/ping-pong-encrypted/Cargo.toml Outdated
Comment thread protocols/v2/binary-sv2/derive_codec/src/lib.rs Outdated
Comment thread protocols/v2/binary-sv2/derive_codec/src/lib.rs Outdated
@GitGab19
Copy link
Copy Markdown
Member

I would squash a891dcc, since it's reverting something you did on this PR.

Please reword commit 25f3417 which doesn't make sense (there's no stratum-common) and it's not descriptive at all.

Usually, I tend to always squash fmt and clippy commits with the related changes, to not pollute git history too much.

@Shourya742
Copy link
Copy Markdown
Member Author

I would squash a891dcc, since it's reverting something you did on this PR.

Please reword commit 25f3417 which doesn't make sense (there's no stratum-common) and it's not descriptive at all.

Usually, I tend to always squash fmt and clippy commits with the related changes, to not pollute git history too much.

I am yet to restruct the history, let me do it.

@Shourya742 Shourya742 force-pushed the 2025-10-16-remove-binary-codec-sv2 branch 6 times, most recently from cdac329 to 7d7dad2 Compare October 17, 2025 16:41
@Shourya742
Copy link
Copy Markdown
Member Author

I would squash a891dcc, since it's reverting something you did on this PR.

Please reword commit 25f3417 which doesn't make sense (there's no stratum-common) and it's not descriptive at all.

Usually, I tend to always squash fmt and clippy commits with the related changes, to not pollute git history too much.

Should be ok now.

@Shourya742 Shourya742 force-pushed the 2025-10-16-remove-binary-codec-sv2 branch 2 times, most recently from cff6d54 to 311452c Compare October 17, 2025 18:13
@Shourya742 Shourya742 marked this pull request as draft October 20, 2025 04:06
@Shourya742 Shourya742 force-pushed the 2025-10-16-remove-binary-codec-sv2 branch 2 times, most recently from a894bb5 to e17aad2 Compare October 20, 2025 11:16
@Shourya742 Shourya742 marked this pull request as ready for review October 20, 2025 11:16
… to binary-sv2, add a new test module for all end-to-end test and update the derive-codec-sv2 to work accordingly
@Shourya742 Shourya742 force-pushed the 2025-10-16-remove-binary-codec-sv2 branch from e17aad2 to b7802fd Compare October 20, 2025 12:06
@plebhash plebhash merged commit 3f587e0 into stratum-mining:main Oct 20, 2025
11 checks passed
plebhash added a commit to plebhash/stratum that referenced this pull request Oct 22, 2025
cdfc4a5 bumped some crate versions unnecessarily

see stratum-mining#1952 (comment)
SV2-bot pushed a commit to plebhash/stratum that referenced this pull request Oct 23, 2025
cdfc4a5 bumped some crate versions unnecessarily

see stratum-mining#1952 (comment)
GitGab19 added a commit to GitGab19/stratum that referenced this pull request Nov 25, 2025
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.

need to deprecate binary_codec_sv2 and restructure binary_sv2 + derive_codec_sv2

3 participants