Bugfix log range filter #445
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent changes enhance the blockchain API's log retrieval functionality by improving type handling and ensuring more robust error management. Key modifications include transitioning from 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 (
|
| PostState []byte `json:"root"` | ||
| Status uint64 `json:"status"` | ||
| CumulativeGasUsed uint64 `json:"cumulativeGasUsed"` | ||
| // todo we could skip bloom to optimize storage and dynamically recalculate it |
There was a problem hiding this comment.
this todo is not really true since we store blooms in storage
| for _, bloomHeight := range bloomsHeight { | ||
| for _, bloom := range bloomHeight.Blooms { | ||
| if !bloomMatch(*bloom, r.criteria) { |
There was a problem hiding this comment.
this was broken, probably got broken after changes, because it did nothing, so it didn't optimize next step.
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- api/api.go (2 hunks)
- api/pull.go (2 hunks)
- models/receipt.go (2 hunks)
- services/logs/filter.go (4 hunks)
- services/logs/filter_test.go (4 hunks)
- storage/index.go (2 hunks)
- storage/index_testsuite.go (3 hunks)
- storage/mocks/BlockIndexer.go (4 hunks)
- storage/mocks/ReceiptIndexer.go (6 hunks)
- storage/mocks/mocks.go (2 hunks)
- storage/pebble/accounts.go (1 hunks)
- storage/pebble/receipts.go (9 hunks)
- storage/pebble/storage_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- storage/mocks/BlockIndexer.go
Additional context used
golangci-lint
storage/index_testsuite.go
342-342: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
343-343: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
Additional comments not posted (26)
storage/mocks/mocks.go (1)
65-65: Consider the impact of randomness on test determinism.The addition of the
Bloomfield with random bytes may lead to non-deterministic test results. Ensure that tests using this mock are robust against such variability or consider using a fixed seed for randomness.storage/pebble/accounts.go (1)
59-59: LGTM! Verify the correctness of the new logic.The change to use
txHeightfor encoding the nonce aligns with the transaction context and seems appropriate. Ensure that this change does not introduce any unintended side effects in the nonce handling logic.Verification successful
Verified: Consistent usage of
txHeightin nonce encoding logic.The use of
txHeightis consistent within thestorage/pebble/accounts.gofile, and its role in encoding the nonce appears to be correctly implemented. There are no apparent side effects or inconsistencies in its usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `txHeight` in nonce encoding logic. # Test: Search for the usage of `txHeight` in the codebase. Expect: Consistent usage in nonce encoding logic. rg --type go 'txHeight'Length of output: 209
storage/mocks/ReceiptIndexer.go (1)
Line range hint
20-119: LGTM! Verify the impact of type changes.The transition from
*big.Inttouint64for block heights simplifies the code and should enhance performance. Ensure that all related code is updated to handleuint64correctly and that there are no lingering dependencies on*big.Int.Verification successful
No lingering dependencies on
*big.Intfor block heights found.The transition from
*big.Inttouint64for block heights appears to be successfully implemented, as there are no remaining dependencies on*big.Intfor this specific purpose in the codebase. Other uses of*big.Intpertain to different functionalities and do not impact the block height changes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing block height types to `uint64`. # Test: Search for any lingering dependencies on `*big.Int` for block heights. Expect: No dependencies remain. rg --type go 'big\.Int' --context 5Length of output: 18936
models/receipt.go (1)
136-136: TransitionHeighttouint64inBloomsHeight.The change from
*big.Inttouint64for theHeightfield simplifies the handling of block heights and aligns with the PR objectives. Ensure that all usages ofBloomsHeightare updated to handleuint64correctly.Verification successful
Transition to
uint64forHeightinBloomsHeightis correctly implemented.The change from
*big.Inttouint64for theHeightfield inBloomsHeightis handled correctly across all usages. The values are derived fromuint64sources, ensuring proper handling and alignment with the PR objectives.
services/logs/filter_test.go:Heightis set usingr.BlockNumber.Uint64().storage/pebble/receipts.go:Heightis set usingbinary.BigEndian.Uint64(height).These usages confirm that the transition to
uint64is properly managed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `BloomsHeight` to ensure they handle `uint64` correctly. # Test: Search for the `BloomsHeight` struct usage. Expect: Correct handling of `uint64`. rg --type go 'BloomsHeight'Length of output: 1218
Script:
#!/bin/bash # Extract code snippets around the usage of BloomsHeight to verify correct handling of uint64. # Check usage in storage/index.go rg --type go -A 5 'BloomsHeight' storage/index.go # Check usage in services/logs/filter_test.go rg --type go -A 5 'BloomsHeight' services/logs/filter_test.go # Check usage in storage/pebble/receipts.go rg --type go -A 5 'BloomsHeight' storage/pebble/receipts.go # Check usage in storage/mocks/ReceiptIndexer.go rg --type go -A 5 'BloomsHeight' storage/mocks/ReceiptIndexer.goLength of output: 2110
storage/index.go (3)
48-48: Parameter renaming inSetLatestCadenceHeight.The parameter
heightis renamed tocadenceHeightfor clarity. This change improves the readability of the method signature.
54-61: Standardize parameter naming and type in interface methods.The parameter
heightnow consistently usesuint64across methods, improving clarity and aligning with the PR objectives. Ensure that all implementations of these interfaces are updated accordingly.Verification successful
Parameter Type Consistency Verified for Interface Methods
The
uint64type is consistently used for theheightparameter in all implementations and mocks of theBlockIndexerandReceiptIndexerinterfaces. This aligns with the review comment's objective to standardize parameter naming and type. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `BlockIndexer` and `ReceiptIndexer` to ensure they handle `uint64` correctly. # Test: Search for the implementations of `BlockIndexer` and `ReceiptIndexer`. Expect: Correct handling of `uint64`. rg --type go 'BlockIndexer' -A 10 rg --type go 'ReceiptIndexer' -A 10Length of output: 51989
Line range hint
69-86:
UpdateReceiptIndexermethods to useuint64.The methods
Store,GetByBlockHeight, andBloomsForBlockRangenow useuint64for block heights, simplifying the code and improving performance. Verify that all method implementations handleuint64correctly.Verification successful
Verification Successful:
ReceiptIndexerMethods Handleuint64CorrectlyThe methods
Store,GetByBlockHeight, andBloomsForBlockRangein theReceiptIndexerinterface have been verified to correctly handleuint64for block heights in both their implementations and associated test cases.
- Implementations in
storage/pebble/receipts.gouseuint64for block heights.- Test cases in
storage/index_testsuite.goalso useuint64, ensuring proper testing of these methods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `ReceiptIndexer` methods to ensure they handle `uint64` correctly. # Test: Search for the implementations of `ReceiptIndexer` methods. Expect: Correct handling of `uint64`. rg --type go 'func (r \*Receipts) Store' -A 10 rg --type go 'func (r \*Receipts) GetByBlockHeight' -A 10 rg --type go 'func (r \*Receipts) BloomsForBlockRange' -A 10Length of output: 171
Script:
#!/bin/bash # Description: Search for the `ReceiptIndexer` interface and its method implementations to verify `uint64` usage. # Search for the `ReceiptIndexer` interface definition. rg --type go 'type ReceiptIndexer interface' -A 20 # Search for any implementation of `Store` method that might be related to `ReceiptIndexer`. rg --type go 'Store\(' -A 10 # Search for any implementation of `GetByBlockHeight` method that might be related to `ReceiptIndexer`. rg --type go 'GetByBlockHeight\(' -A 10 # Search for any implementation of `BloomsForBlockRange` method that might be related to `ReceiptIndexer`. rg --type go 'BloomsForBlockRange\(' -A 10Length of output: 39942
storage/pebble/receipts.go (3)
47-51: Simplify block height handling inStore.The block height is now directly retrieved as a
uint64, simplifying the code. Ensure that all related logic correctly handlesuint64.
Line range hint
152-217:
OptimizeBloomsForBlockRangewithuint64.The method now uses
uint64for range queries, simplifying the logic and improving performance. Verify that all related logic correctly handlesuint64.
113-117: UpdateGetByBlockHeightto useuint64.The method now accepts
uint64, improving performance by reducing type conversion overhead. Verify that all related logic correctly handlesuint64.Verification successful
All usages of
GetByBlockHeightcorrectly handleuint64.The method
GetByBlockHeightis used in various parts of the codebase, and all instances correctly handleuint64parameters. Specifically:
- In
services/logs/filter.go,heightvalues are derived fromuint64sources.- The
blk.Heightis effectivelyuint64, as it corresponds to theNumberfield in theapi/models.goBlockstruct, which useshexutil.Uint64.No further changes are necessary.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `GetByBlockHeight` to ensure they handle `uint64` correctly. # Test: Search for the method usage. Expect: Correct handling of `uint64`. rg --type go 'GetByBlockHeight'Length of output: 1169
Script:
#!/bin/bash # Verify the types of `height` and `blk.Height` in `services/logs/filter.go` to ensure they are `uint64`. rg --type go 'height' services/logs/filter.go -A 5 rg --type go 'blk' services/logs/filter.go -A 5Length of output: 1290
Script:
#!/bin/bash # Verify the type of `blk.Height` by searching for the definition or assignment of `blk`. rg --type go 'blk' services/logs/filter.go -B 5Length of output: 268
Script:
#!/bin/bash # Search for the definition or implementation of the `GetByID` method to determine the type of `blk`. rg --type go 'GetByID' -A 5Length of output: 5132
Script:
#!/bin/bash # Search for the definition of the `models.Block` struct to verify the type of the `Height` field. rg --type go 'type Block struct' -A 10Length of output: 1197
services/logs/filter.go (4)
51-51: LGTM! Simplified block height handling.Changing
startandendfrom*big.Inttouint64simplifies the code and improves performance.
57-66: LGTM! Improved clarity and efficiency.The use of
uint64forstartandendenhances clarity and performance by allowing direct comparisons.
87-106: LGTM! Optimized filtering process.The use of
uint64forbloomHeightMatchesand direct fetching of receipts improves efficiency. Consider addressing the TODO for concurrent processing.
158-158: LGTM! Simplified receipt fetching.Using
blk.Heightdirectly aligns with the new type handling and simplifies the logic.storage/pebble/storage_test.go (1)
40-40: LGTM! Enhanced test coverage for larger block heights.Changing the block height from
30to300broadens the test scenario to ensure robustness.services/logs/filter_test.go (3)
121-125: LGTM! Simplified receipt storage logic.Switching to
uint64for block height comparisons enhances performance and simplifies the logic.
138-148: LGTM! Simplified blooms range logic.Using
uint64for start and end parameters aligns with the new type handling and simplifies range checks.
288-341: LGTM! Updated test cases for new type handling.Using
uint64forstartandendin test cases ensures alignment with updated function signatures.api/pull.go (1)
480-504: LGTM! But verify the function usage in the codebase.The transition from
big.Inttouint64for block heights simplifies the logic and improves performance. The error handling for range filter creation is clear and concise.However, ensure that all function calls to
getLogscorrectly handle the newuint64type.Verification successful
Verify the Usage of
getLogswithuint64ParametersThe
LatestEVMHeightfunction, which is used to retrieve block heights, returns auint64type. This ensures compatibility with thegetLogsfunction, which now expectsuint64parameters. Therefore, the use ofcurrentas an argument ingetLogsis correctly handled as it is derived fromLatestEVMHeight.No changes are needed for the
getLogsfunction call.
- Function:
getLogs- Parameters:
uint64(fromLatestEVMHeight)The transition to using
uint64is correctly implemented in the codebase.Action:: No action required. The code is already correctly using
uint64for block heights.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getLogs` correctly handle the new `uint64` type. # Test: Search for the function usage. Expect: Correct handling of `uint64` type. rg --type go -A 5 $'getLogs'Length of output: 979
Script:
#!/bin/bash # Description: Verify the type of `current` used in `getLogs` calls to ensure it is `uint64`. # Test: Search for the definition and type of `current` in `api/pull.go`. rg --type go 'current' api/pull.goLength of output: 643
Script:
#!/bin/bash # Description: Verify the return type of `LatestEVMHeight()` to ensure it is `uint64`. # Test: Search for the definition of `LatestEVMHeight` to check its return type. rg --type go 'func LatestEVMHeight' --context 5Length of output: 49
Script:
#!/bin/bash # Description: Search for the definition of `LatestEVMHeight` across all Go files to determine its return type. # Test: Search for `LatestEVMHeight` function definition in all Go files. rg --type go 'func LatestEVMHeight' --context 5Length of output: 49
Script:
#!/bin/bash # Description: Search for `LatestEVMHeight` as a method on any struct or interface to determine its return type. # Test: Search for `LatestEVMHeight` method definitions in all Go files. rg --type go 'LatestEVMHeight' --context 5Length of output: 13618
storage/index_testsuite.go (6)
193-193: LGTM!The use of
uint64for block heights in the test case improves type safety and readability.
230-236: LGTM!The use of
uint64for block heights in the test case improves type safety and readability.
245-362: LGTM!The use of
uint64for block heights in the test case improves type safety and readability.Tools
golangci-lint
342-342: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
343-343: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
283-329: LGTM!The use of
uint64for block heights in the test case improves type safety and readability.
353-362: LGTM!The use of
uint64for block heights in the test case improves type safety and readability.
332-349: Verify the necessity ofbig.Intusage.The use of
big.Intfor block heights in this test case seems inconsistent with other changes that useuint64. Verify if this is required for specific test coverage.Verification successful
Verified: Use of
big.Intis consistent and appropriate.The use of
big.Intfor block heights in thesingle height rangetest case is consistent with its usage throughout the codebase. This suggests it is a deliberate design choice, likely to handle large numbers or ensure consistency across tests and implementations. No changes are necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the use of `big.Int` for block heights in test cases is necessary. # Test: Search for `big.Int` usage in test cases. Expect: Consistent usage across test cases. rg --type go -A 5 $'big.NewInt'Length of output: 24150
Tools
golangci-lint
342-342: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
343-343: SA4010: this result of append is never used, except maybe in other appends
(staticcheck)
api/api.go (1)
Line range hint
652-666: LGTM!The use of
Uint64()for block heights and ensuring consistent return types in theGetLogsmethod improves type safety and robustness.
Closes: #439
Description
The storage implementation for bloom filters incorrectly defined the range which resulted in additional logs being returned.
Because we've used
big.Inttype for heights, more specificallybig.Int.Bytes()which returned byte array that only contained non-empty values in big-endian encoding, it allowed for multiple other values to match the pebble itteration upper/lower bound filter.Simply said, if we wanted to get height 1 when converted using
Bytes()we got a slice[]byte{0x1}and that was used as boundary in iterations, but that also matched other values starting with same byte (like[]byte{0x01, 0x02}or[]byte{0x01, 0xf, 0xa}). We changed usage ofbig.Inttouint64since it doesn't really make sense in storage layer to usebig.Int, and we changed to encoding of uint64 to fixed length of 8 bytes. So the above example of height 1 would be defined as[]byte{0x1, 0x0, 0x0, 0x0}thus no other value would match it.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
*big.Inttouint64).Bug Fixes
nilreference errors.Documentation
Tests