Change hash representation from slice to 32-byte array #505
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
This change is non-functional, just a clean-up, reducing tech debt. This codebase manipulates many 32-byte hashes, such as block hashes, transaction IDs, and merkle roots. These should always have been represented as fixed-size 32-byte arrays rather than variable-length slices. This prevents bugs (a slice that's supposed to be of length 32 bytes could be a different length) and makes assigning, comparing, function argument passing, and function return value passing simpler and less fragile. The new hash type, hash32.T, which is defined as [32]byte (32-byte array of bytes), can be treated like any simple variable, such as an integer. A slice, in contrast, is like a string; it's really a structure that includes a length, capacity, and pointer to separately-allocated memory to hold the elements of the slice. The only point of friction is that protobuf doesn't support fixed-sized arrays, only (in effect) slices. So conversion must be done in each direction.
LarryRuane
force-pushed
the
2025-01-hash32
branch
from
9cf08c4 to
7cfe086
Compare
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 change is non-functional, just a clean-up, reducing tech debt. I've been meaning to do this almost from when I first started working on lightwalletd in 2019.
This codebase manipulates many 32-byte hashes, such as block hashes, transaction IDs, and merkle roots. These should always have been represented as fixed-size 32-byte arrays rather than variable-length slices. This prevents bugs (a slice that's supposed to be of length 32 bytes could be a different length) and makes assigning, comparing, function argument passing, and function return value passing simpler and less fragile.
The new hash type,
hash32.T, which is defined as[32]byte(32-byte array of bytes), can be treated like any simple variable, such as an integer. A slice, in contrast, is like a string; it's really a structure that includes a length, capacity, and pointer to separately-allocated memory to hold the elements of the slice.The only point of friction is that protobuf doesn't support fixed-sized arrays, only (in effect) slices. So conversion must be done in each direction.
This PR also changes the representations of a few other (non-hash) variables that have known, fixed sizes, such as the block header fields
NBitsBytes,Nonce, andSolution.I tested this (besides running the unit test suite) manually by using both the current master branch and this PR's branch to sync the mainnet blockchain, which means deserializing full blocks, converting them to compact blocks, and writing them to disk. Then, I compared these files to verify that they were identical. If this PR introduced a bug in serializing or deserializing compact blocks, this test would have caught it.