Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocol version must be 2 in SetupConnection message #82

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

pythcoiner
Copy link
Contributor

there is a protocol version missmatch between SetupConnection here and the specs, i guess the typo is on spec side.

@Sjors
Copy link
Contributor

Sjors commented Jun 7, 2024

This was changed from 2 to 3 by @jakubtrnka in #12, but I have no idea why. Perhaps because it was a breaking change, but we've made many of those since then.

@pythcoiner
Copy link
Contributor Author

This was changed from 2 to 3 by @jakubtrnka in #12, but I have no idea why. Perhaps because it was a breaking change, but we've made many of those since then.

Does it means this version field is about stratum v2 messaging version and not about the stratum (v1/v2) version?

@Sjors
Copy link
Contributor

Sjors commented Jun 7, 2024

@pythcoiner I think so yes.

@jakubtrnka
Copy link
Collaborator

This was changed from 2 to 3 by @jakubtrnka in #12, but I have no idea why. Perhaps because it was a breaking change, but we've made many of those since then.

Does it means this version field is about stratum v2 messaging version and not about the stratum (v1/v2) version?

This is correct. It refers to the version of the protocol Stratum-V2.

Originally we wanted to follow some kind versioning even during the early development phase. There were some backwards incompatible changes and the version number seemed like a little technical detail back then. I incremented it automatically without thinking too much.

If memory serves me well, I think we then later agreed we shouldn't change the protocol versions unless absolutely necessary. It's because every change only raises questions and uncertainty about compatibility and should be thoroughly explained how implementations need to deal with it. Writing some compatibility support for every single change that has been made en the early stages of development would be totally needless maintainability nightmare.

Currently there is no need to maintain multiple implementation versions. Currently there is supposed to be only one single correct version of stratum-v2 protocol. So if there is some unexpected discrepancy somewhere, it should probably be unified.

So I suggest that we use the number 2 everywhere. In the spec, and in code. It should be only corrected in the spec, am I right? Code uses the single version 2 everywhere, right?

Also it's perhaps a bit confusing why the version number is 2. The version 1 never even existed. One might ask if we could reset the version to 1 as of the stable release and start from scratch. I think that would be a fair point.

@pythcoiner
Copy link
Contributor Author

So I suggest that we use the number 2 everywhere. In the spec, and in code. It should be only corrected in the spec, am I right? Code uses the single version 2 everywhere, right?

i only find differents values than 2 here on implem side.

Also it's perhaps a bit confusing why the version number is 2. The version 1 never even existed. One might ask if we could reset the version to 1 as of the stable release and start from scratch. I think that would be a fair point.

I think so.

@Sjors
Copy link
Contributor

Sjors commented Jun 20, 2024

The Bitcoin Core Template Provider also uses 2.

@Fi3 Fi3 merged commit c686dca into stratum-mining:main Jul 3, 2024
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.

None yet

5 participants