fix(store): make node.copy() actually copy - #569
Open
0xrlawrence wants to merge 1 commit into
Open
Conversation
node.copy() was 'return &(*x)', which is just x: dereferencing and
immediately re-addressing a pointer yields the same pointer. Every caller
that asked for a copy got an alias to the original node, so mutating the
'copy' mutated the original. staticcheck reports the expression as SA4001.
This is already known to be load-bearing. store.go carries a comment saying
the SMT node cache 'MUST NOT be persisted across blocks' precisely because
'node.copy() is a no-op alias, so the parallel commit mutates cached *node
objects in place', and works around it with a fresh per-block cache. Fixing
the copy removes the reason that workaround exists, though this change
leaves the workaround in place.
The node is rebuilt field by field rather than with '*x', because lib.Node
embeds a protobuf MessageState and a plain struct copy would be a lock copy
that 'go vet' rejects. Byte slices and the *key are still shared, which is
what the existing 'shallow copy' doc comment describes.
Verification: full suite passes ('go test ./... -p=1'), and the state root
over a deterministic 500-write / 167-delete workload is byte-identical
before and after the change:
main ROOT=3944f19bc128472bd163bd081b5bc648fd97f535ca823b88f6b5da8548e391a6
fix/smt-node-copy-no-op ROOT=3944f19bc128472bd163bd081b5bc648fd97f535ca823b88f6b5da8548e391a6
Given this is consensus-critical storage code, reviewers may still want to
run it against a real chain replay before merging.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
node.copy()was:&(*x)is justx— dereferencing and immediately re-addressing a pointer yields the same pointer. Every caller that asked for a copy got an alias to the original node, so mutating the "copy" mutated the original. staticcheck reports the expression as SA4001.This is already known to be load-bearing.
store/store.gocarries a comment saying the SMT node cache "MUST NOT be persisted across blocks", precisely because:Fixing the copy removes the reason that workaround exists. This PR leaves the workaround in place — it only makes the primitive honest.
Changes Made
copy()now rebuilds the node field by field and returns a distinct pointer.Rebuilt field by field rather than with
*xbecauselib.Nodeembeds a protobufMessageState; a plain struct copy would be a lock copy thatgo vetrejects. Byte slices and the*keyare still shared, which is what the existing "shallow copy" doc comment describes.Testing
go test ./... -p=1Note for reviewers
This is consensus-critical storage code. The root comparison above and a green suite are good signals, but not proof — you may well want to run it against a real chain replay before merging.
🤖 Generated with Claude Code