Empty miner field on eth_getBlockByNumber
#463
Conversation
WalkthroughThe changes involve updates to the blockchain API to include a Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add 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: 0
Outside diff range, codebase verification and nitpick comments (1)
api/api.go (1)
969-969: Issue Found:CoinbaseAddressis not defined in the codebase.The
CoinbaseAddressused inapi/api.godoes not have a corresponding definition within the codebase. This could lead to runtime errors. Please ensure thatCoinbaseAddressis correctly defined or imported from the appropriate package.
- File:
api/api.go- Line: 969
Analysis chain
LGTM! Verify the correctness of
CoinbaseAddress.The addition of the
Minerfield to the block response is a positive change that aligns with the PR objectives. Ensure thatevmTypes.CoinbaseAddresscorrectly represents the miner's address in all contexts.Run the following script to verify the definition and usage of
CoinbaseAddress:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `CoinbaseAddress`. # Test: Search for the definition and usage of `CoinbaseAddress`. Expect: Correct definition and usage. rg --type go 'CoinbaseAddress'Length of output: 98
Script:
#!/bin/bash # Description: Search for the definition of `CoinbaseAddress` in the codebase. # Test: Locate the definition of `CoinbaseAddress`. Expect: Definition found. rg --type go 'CoinbaseAddress'Length of output: 98
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
Files selected for processing (7)
- api/api.go (2 hunks)
- go.mod (5 hunks)
- models/block.go (1 hunks)
- models/block_test.go (4 hunks)
- models/events_test.go (1 hunks)
- tests/go.mod (4 hunks)
- tests/web3js/eth_non_interactive_test.js (1 hunks)
Additional comments not posted (17)
models/block.go (1)
63-63: Addition ofPrevRandaofield inBlockstructure.The addition of the
PrevRandaofield enhances the block data structure by providing additional information. Ensure that this change is consistently handled across the codebase.models/block_test.go (4)
23-23: Inclusion ofPrevRandaoinTest_DecodeBlockExecutedEvent.The addition of
PrevRandaoin this test case ensures that the new field is correctly decoded and validated.
48-48: Update of hash constant inTest_Hash.The updated hash value reflects changes in the block data or hashing algorithm. Ensure that this change is intentional and documented.
57-57: Inclusion ofPrevRandaoinTest_Hash.The addition of
PrevRandaoin this test case ensures that the hash calculation accounts for the new field.
75-75: Inclusion ofPrevRandaoinTest_EncodingDecoding.The addition of
PrevRandaoin this test case ensures that encoding and decoding processes handle the new field correctly.models/events_test.go (1)
178-183: Inclusion ofPrevRandaoinnewBlockfunction.The addition of the
PrevRandaoparameter in thenewBlockfunction ensures that block creation includes the new field, maintaining consistency with the updated block structure.tests/web3js/eth_non_interactive_test.js (2)
28-28: Update to block size assertion.The expected block size has been updated to
3994n. Ensure this matches the actual block size returned by the API.
29-29: Addition of miner assertion.The test now verifies the
block.minerfield. Ensure that the miner address'0x0000000000000000000000030000000000000000'is correct and consistent with the expected data.go.mod (5)
10-12: Dependency updates foratree,cadence, andflow-go.These updates reflect a shift to newer versions. Ensure compatibility with the rest of the codebase and check for any breaking changes introduced by these updates.
24-24: Update togolang.org/x/sync.The version has been updated to
v0.8.0. Verify that this update does not introduce any breaking changes.
42-42: Update tobtcsuite/btcd/btcec/v2.The version has been updated to
v2.3.4. Ensure this update is compatible with the existing code.
137-137: Update toonflow/crypto.The version has been updated to
v0.25.2. Verify compatibility with the codebase.
196-200: Updates togolang.org/xpackages.The versions for
crypto,net,sys, andtexthave been updated. Ensure these updates do not introduce breaking changes.tests/go.mod (4)
7-11: Dependency updates forcadence,crypto, andflow-go.These updates reflect a shift to newer versions. Ensure compatibility with the test code and check for any breaking changes introduced by these updates.
34-34: Update tobtcsuite/btcd/btcec/v2.The version has been updated to
v2.3.4. Ensure this update is compatible with the test code.
148-148: Update toonflow/atree.The version has been updated to
v0.8.0-rc.6. Verify compatibility with the test code.
215-222: Updates togolang.org/xpackages.The versions for
crypto,net,sync,sys,term, andtexthave been updated. Ensure these updates do not introduce breaking changes in the test code.
|
We should add logic for backward compatibility. Best, if you make it part of this PR |
m-Peter
force-pushed
the
update-block-miner-field
branch
from
a5219e4
to
a3be967
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
Files selected for processing (2)
- go.mod (5 hunks)
- tests/go.mod (4 hunks)
Files skipped from review due to trivial changes (1)
- tests/go.mod
Files skipped from review as they are similar to previous changes (1)
- go.mod
I have added such a logic in this commit: 5b33fe1. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- models/block.go (3 hunks)
- models/block_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- models/block_test.go
Additional comments not posted (9)
models/block.go (9)
12-12: Imports look good.The import statements are appropriate for the operations performed in the file.
32-41: Backward compatibility logic is well-implemented.The function
NewBlockFromBytescorrectly handles backward compatibility by attempting to decode previous block versions.
49-49: Addition ofhashfield is appropriate.The
hashfield in theBlockstruct is a necessary addition for storing the block's hash.
53-58: Efficient hash retrieval method.The
Hashmethod provides a clear and efficient way to retrieve the block's hash.
81-81: Decoding block event is correctly implemented.The
decodeBlockEventfunction accurately decodes a block event into theBlocktype.
86-94: Backward compatibility structblockV0is appropriate.The
blockV0struct is necessary for handling older block versions.
96-106: Inclusion ofPrevRandaofield inblockV1is correct.The
blockV1struct is necessary for handling newer block versions with thePrevRandaofield.
108-111: Hash computation for past block is correctly implemented.The
pastBlockHashfunction correctly encodes the block and computes its hash.
113-148: Backward compatibility decoding is well-implemented.The
decodeBlockBreakingChangesfunction correctly handles decoding of previous block versions.
| func (b *Block) Hash() (gethCommon.Hash, error) { | ||
| if b.hash != nil { | ||
| return *b.hash, nil | ||
| } | ||
| return b.Block.Hash() | ||
| } |
There was a problem hiding this comment.
Is this Block object serialized? if yes are we worried about caching the hash?
There was a problem hiding this comment.
This Block object is indeed serialized, but the hash field is only set for past blocks. Where past block means a block that fails to decode with the current format.
| // decodeBlockBreakingChanges will try to decode the bytes into all | ||
| // previous versions of block type, if it succeeds it will return the | ||
| // migrated block, otherwise it will return nil. | ||
| func decodeBlockBreakingChanges(encoded []byte) *Block { |
There was a problem hiding this comment.
Isn't this a cop of the code from flow go?
should we just export those methods?
There was a problem hiding this comment.
Yeah, this is a copy from that code in flow-go. Though I am not sure if it still exists in the flow-go repo 🤔 .
We used to have these file changes for PreviewNet: 0e520ac. But removed them for testnet launch.
models/block.go
Outdated
| b1 := &blockV1{} | ||
| if err := rlp.DecodeBytes(encoded, b1); err == nil { | ||
| return &Block{ | ||
| Block: &types.Block{ | ||
| ParentBlockHash: b1.ParentBlockHash, | ||
| Height: b1.Height, | ||
| Timestamp: b1.Timestamp, | ||
| TotalSupply: b1.TotalSupply, | ||
| ReceiptRoot: b1.ReceiptRoot, | ||
| TransactionHashRoot: b1.TransactionHashRoot, | ||
| TotalGasUsed: b1.TotalGasUsed, | ||
| PrevRandao: b1.PrevRandao, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need this, given the Block itself ? is similar to BlockV1
There was a problem hiding this comment.
BlockV1 has the addition of the PrevRandao field, which BlockV0 does not have. Unless you mean something else which I'm missing 😅
There was a problem hiding this comment.
Ah right, now I see what you mean 😇
There was a problem hiding this comment.
Removed in 2f97be1
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.mod (5 hunks)
Additional comments not posted (10)
go.mod (10)
10-10: Dependency update approved forgithub.com/onflow/atree.The update from
v0.8.0-rc.5tov0.8.0-rc.6is likely to include minor improvements or bug fixes.
11-11: Dependency update approved forgithub.com/onflow/cadence.The update from
v1.0.0-preview.48tov1.0.0-preview.49is likely to include minor improvements or bug fixes.
12-12: Dependency update approved forgithub.com/onflow/flow-go.The update from
v0.37.1tov0.37.6might include bug fixes or improvements. However, ensure backward compatibility due to the mentioned breaking change in block hash calculation.
24-24: Dependency update approved forgolang.org/x/sync.The update from
v0.7.0tov0.8.0is likely to include minor improvements or bug fixes.
42-42: Dependency update approved forgithub.com/btcsuite/btcd/btcec/v2.The update from
v2.2.1tov2.3.4is likely to include minor improvements or bug fixes.
137-137: Dependency update approved forgithub.com/onflow/crypto.The update from
v0.25.1tov0.25.2is likely to include minor improvements or bug fixes.
196-196: Dependency update approved forgolang.org/x/crypto.The update from
v0.22.0tov0.26.0is likely to include minor improvements or bug fixes.
197-197: Dependency update approved forgolang.org/x/net.The update from
v0.24.0tov0.25.0is likely to include minor improvements or bug fixes.
199-199: Dependency update approved forgolang.org/x/sys.The update from
v0.20.0tov0.23.0is likely to include minor improvements or bug fixes.
200-200: Dependency update approved forgolang.org/x/text.The update from
v0.14.0tov0.17.0is likely to include minor improvements or bug fixes.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- models/block.go (3 hunks)
Additional comments not posted (6)
models/block.go (6)
12-12: Import addition is appropriate.The
gethCryptoimport is necessary for hash calculations.
49-58: Enhancement of Block data model is appropriate.The addition of the
hashfield and theHashmethod improves the block's data model.
81-81: Addition ofPrevRandaofield is appropriate.The
PrevRandaofield aligns with the changes mentioned in the PR summary and objectives.
86-94: Introduction ofblockV0type is necessary for backward compatibility.The
blockV0type is essential for handling older block versions.
96-121: Functions for handling past block data are well-implemented.The
pastBlockHashanddecodeBlockBreakingChangesfunctions are correctly implemented for handling past block data.Run the following script to verify the usage of
decodeBlockBreakingChangesin the codebase:Verification successful
Usage of
decodeBlockBreakingChangesis appropriate and limited tomodels/block.go.The function
decodeBlockBreakingChangesis used correctly within themodels/block.gofile to decode block data and handle errors. There are no other occurrences of this function in the codebase, indicating its specialized use.
- File:
models/block.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `decodeBlockBreakingChanges` in the codebase. # Test: Search for the function usage. Expect: Occurrences of the function being called. rg --type go -A 5 $'decodeBlockBreakingChanges'Length of output: 841
32-41: Backward compatibility logic is well-implemented.The
NewBlockFromBytesfunction effectively handles decoding of previous block versions and assigns the hash correctly.Run the following script to verify the usage of
NewBlockFromBytesin the codebase:Verification successful
Backward compatibility logic is well-implemented and verified.
The
NewBlockFromBytesfunction is used in both test and non-test files, confirming its integration and backward compatibility handling within the codebase.
- Usage Locations:
models/block_test.go: Tests theNewBlockFromBytesfunction.storage/pebble/blocks.go: Utilizes the function for processing data.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NewBlockFromBytes` in the codebase. # Test: Search for the function usage. Expect: Occurrences of the function being called. rg --type go -A 5 $'NewBlockFromBytes'Length of output: 631
Closes: #454
Description
Note: The version bump of the
flow-godependency, comes with a backwards incompatible change on the block hash calculation.If we want to deploy this change on
testnet, we'll have to add some custom logic to accommodate that breaking change.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Minerfield to block response, providing crucial miner address information.PrevRandaofield in block events for enhanced event tracking.Blockstructure with a newhashfield and a method to return the block's hash.Minerfield and updated block sizes.Bug Fixes
Chores