Correct several stratum/pooled mining issues. #220
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jholdstock
reviewed
Oct 11, 2023
| w := work.NewWork(workData, bigTarget, givenTs, uint32(time.Now().Unix()), true) | ||
|
|
||
| const isGetWork = true | ||
| timestamp := binary.LittleEndian.Uint32(workData[128+4*work.TimestampWord:]) |
There was a problem hiding this comment.
I'm struggling to understand why the upper bound of the slice has been removed here. Given that timestamp is not the last item in the bytes (per the serialization format provided here), isn't removing the upper bound going to select the timestamp and everything after it?
There was a problem hiding this comment.
All of the little/big endian byte conversions are necessarily automatically sized by the type in question. A Uint32 is always 4 bytes. As a result, 4 bytes, and only 4 bytes, from the slice are always used.
This updates the code to properly support pooled mining via stratum. In particular: - The extra nonce provided by the pool is now correctly set and used - The length for the second extra nonce provided by the pool is now validated - The second extra nonce length now respects the provided length - The mining code now uses the correct offset within the serialized work data for the second extra nonce to ensure the pool properly reconstructs the header - The provided timestamp is now updated locally as the mining process is underway and the final timestamp is submitted along with the share as expected - The correct network parameters for the active network are now passed into the stratum code so the right difficulties are used - The stratum fields that represent numbers are now consistently in little endian per the stratum "spec" (such that it is) - The "second generation tx" field is now ignored because it does not apply to Decred Finally, various loggging messages have been cleaned up and the job id is no longer incorrectly expected to be numeric when logging it.
davecgh
force-pushed
the
correct_stratum_support
branch
from
7ac9d37
to
2e7c33a
Compare
jholdstock
approved these changes
Oct 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the code to properly support pooled mining via stratum.
In particular:
Finally, various loggging messages have been cleaned up and the job id is no longer incorrectly expected to be numeric when logging it.