Simplify usage of FixedHash field on Block type
#520
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Block as Block
participant Hash as Hash
participant Test as Test
Block->>Hash: Set FixedHash to gethCommon.Hash
Hash-->>Block: Return hash value
Test->>Block: Validate block hash
Test-->>Test: Assert hash matches expected value
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
models/block.go (2)
49-50: Clarification onFixedHashfield documentation.The comment above the
FixedHashfield explains the purpose of this field well, ensuring that the hash calculated with the previous format is preserved. However, it might be beneficial to clarify further why this specific implementation choice (usinggethCommon.Hashinstead of a pointer) improves safety and simplicity, as mentioned in the PR summary.
121-121: Ensure consistency in hash assignment during legacy block decoding.The assignment of
FixedHashdirectly from theblock.Hashin thedecodeLegacyBlockEventfunction is a good use of the newFixedHashtype. This ensures that the hash is preserved accurately during the decoding of legacy events. It would be beneficial to add a comment here explaining that this change is part of the transition to the new hash handling method.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- models/block.go (9 hunks)
- models/block_test.go (4 hunks)
- tests/web3js/eth_non_interactive_test.js (1 hunks)
Additional comments not posted (5)
models/block.go (2)
59-60: Approval of the updated hash check logic.The updated logic in the
Hash()method to check ifFixedHashis not the empty hash (gethCommon.HexToHash("0x0")) is a clear improvement over the previous need to check for nil and an empty string. This change simplifies the code and reduces potential errors related to nil dereferencing.
175-175: Validate the newFixedHashassignment indecodeBlockBreakingChanges.The assignment of
FixedHashtoblockHashin thedecodeBlockBreakingChangesfunction is a crucial part of ensuring that blocks decoded from older formats retain their original hash. This change aligns with the PR's goal of simplifying hash handling. Ensure that this change is thoroughly tested, especially since it involves historical data and could impact data integrity during re-indexing.Verification successful
The
FixedHashassignment is properly tested indecodeBlockBreakingChanges. TheTest_FixedHashBlockfunction inmodels/block_test.goensures that theFixedHashis retained correctly during encoding and decoding, addressing the review comment's concern about data integrity.
models/block_test.go:Test_FixedHashBlockverifies theFixedHashfunctionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `FixedHash` assignment in `decodeBlockBreakingChanges` retains the original hash. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'decodeBlockBreakingChanges'Length of output: 763
Script:
#!/bin/bash # Search for test files or functions that involve `decodeBlockBreakingChanges` or `FixedHash`. rg --type go 'decodeBlockBreakingChanges|FixedHash' --glob '*_test.go'Length of output: 175
Script:
#!/bin/bash # Extract and review the `Test_FixedHashBlock` function from `models/block_test.go`. rg --type go -A 20 'func Test_FixedHashBlock' models/block_test.goLength of output: 513
models/block_test.go (2)
35-42: Improved hash comparison in tests.The update to use
gethCommon.HexToHashfor direct hash comparison instead of converting to strings enhances type safety and clarity in the test assertions.
77-77: Enhanced handling and testing ofFixedHash.The changes to directly use
gethCommon.HashforFixedHashand update the assertions accordingly improve the clarity and efficiency of the tests. This aligns well with the PR's objectives to simplify and enhance safety in hash handling.Also applies to: 82-82, 91-91, 102-102
tests/web3js/eth_non_interactive_test.js (1)
28-28: Verify the updated block size expectation.The change in the expected block size from
3995nto4028nneeds verification to ensure it aligns with the updated blockchain logic or data changes. It's crucial to confirm that this new expected size is accurate based on the modifications made to the blockchain's behavior.Run the following script to verify the block size calculation:
m-Peter
force-pushed
the
simplify-fixed-hash-field
branch
from
d4282c7
to
65ec134
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- models/block.go (9 hunks)
- models/block_test.go (4 hunks)
- tests/web3js/eth_non_interactive_test.js (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- models/block.go
- models/block_test.go
- tests/web3js/eth_non_interactive_test.js
|
Yeah this is nicer, I did have this approach but with a pointer, I should remove the pointer. I think this is good but I wouldn't do it unless we plan a reindex anyway. So we can keep this in draft until then and add a comment in the description |
Description
It is safer/simpler to use the
gethCommon.Hashtype forFixedHash, instead of a*string, all we have to do is check thatFixedHashis not the empty hash.The problem is that this change will require a re-index, so I am not sure if it is worth it.
DO NOT MERGE UNLESS REINDEX
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes