chore: check if a block is empty by taking its reference #4063
Conversation
rach-id
added
chore
rach-id
requested review from
cmwaters and
evan-forbes
and removed request for
a team
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExtendBlock
participant Data
Client->>ExtendBlock: Call IsEmptyBlock
ExtendBlock->>Data: Check if transactions are empty
Data-->>ExtendBlock: Return empty status
ExtendBlock-->>Client: Return empty status
Client->>ExtendBlock: Call IsEmptyBlockRef
ExtendBlock->>Data: Check if transactions are empty (by reference)
Data-->>ExtendBlock: Return empty status
ExtendBlock-->>Client: Return empty status
Warning Rate limit exceeded@rach-id has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
Documentation and Community
|
![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 and nitpick comments (4)
app/extend_block.go (2)
28-34: Consider enhancing the deprecation notice and reviewing the version parameter.The deprecation notice is good, but could be more specific about when this will be removed. Also, the unused version parameter (
uint64) might indicate a design consideration that should be documented or removed if not needed.Consider updating the deprecation notice:
-// Deprecated: Use IsEmptyBlockRef for better performance with large data structures. +// Deprecated: Use IsEmptyBlockRef for better performance with large data structures. +// This function will be removed in the next major version (v4.0.0).If the version parameter is truly unused across all implementations, consider removing it:
-func IsEmptyBlock(data coretypes.Data, _ uint64) bool { +func IsEmptyBlock(data coretypes.Data) bool {
28-41: Consider providing migration guidance and benchmarks.Since this is a core package functionality, it would be valuable to:
- Document the expected performance improvement with some benchmark numbers
- Provide migration examples for existing users
- Consider adding these functions to a more specific package (e.g.,
blockutils) in a future refactoringWould you like me to help create a benchmark test to quantify the performance improvement?
app/test/integration_test.go (2)
278-287: Consider deprecating this test in favor of TestIsEmptyBlockRefThe staticcheck suppression indicates this test is using a deprecated method. Since we now have
TestIsEmptyBlockReftesting the new implementation, we should:
- Add a TODO comment indicating when this test will be removed
- Document why we're keeping both tests temporarily
func (s *IntegrationTestSuite) TestIsEmptyBlock() { + // TODO: Remove this test once IsEmptyBlock is removed in the next major version + // Keeping this temporarily to ensure backward compatibility during transition t := s.T() emptyHeights := []int64{1, 2, 3}
289-297: Consider adding negative test casesThe test effectively validates empty blocks, but could be more comprehensive. Consider adding:
- Test cases with non-empty blocks to verify false negatives
- Edge cases like blocks with empty transactions array vs nil
func (s *IntegrationTestSuite) TestIsEmptyBlockRef() { t := s.T() + testCases := []struct { + name string + height int64 + expected bool + }{ + {"empty block", 1, true}, + {"block with transactions", 4, false}, // Ensure this height has transactions + } - emptyHeights := []int64{1, 2, 3} - for _, h := range emptyHeights { + for _, tc := range testCases { + s.T().Run(tc.name, func(t *testing.T) { - blockRes, err := s.cctx.Client.Block(s.cctx.GoContext(), &h) + blockRes, err := s.cctx.Client.Block(s.cctx.GoContext(), &tc.height) require.NoError(t, err) - require.True(t, app.IsEmptyBlockRef(&blockRes.Block.Data, blockRes.Block.Header.Version.App)) + result := app.IsEmptyBlockRef(&blockRes.Block.Data, blockRes.Block.Header.Version.App) + require.Equal(t, tc.expected, result) ExtendBlockTest(t, blockRes.Block) + }) } }
| // IsEmptyBlockRef returns true if the application considers the given block data | ||
| // empty at a given version. | ||
| // This method passes the block data by reference for improved performance. | ||
| func IsEmptyBlockRef(data *coretypes.Data, _ uint64) bool { | ||
| return len(data.Txs) == 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil pointer check and enhance documentation.
While the implementation is correct, there are a few improvements that could make it more robust and informative:
- Add nil pointer check for safety
- Document the performance benefit more specifically
- Consider the unused version parameter
Consider this enhanced implementation:
// IsEmptyBlockRef returns true if the application considers the given block data
// empty at a given version.
-// This method passes the block data by reference for improved performance.
+// This method passes the block data by reference to avoid copying large data structures,
+// particularly beneficial when dealing with blocks containing many transactions.
func IsEmptyBlockRef(data *coretypes.Data, _ uint64) bool {
+ if data == nil {
+ return true
+ }
return len(data.Txs) == 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // IsEmptyBlockRef returns true if the application considers the given block data | |
| // empty at a given version. | |
| // This method passes the block data by reference for improved performance. | |
| func IsEmptyBlockRef(data *coretypes.Data, _ uint64) bool { | |
| return len(data.Txs) == 0 | |
| } | |
| // IsEmptyBlockRef returns true if the application considers the given block data | |
| // empty at a given version. | |
| // This method passes the block data by reference to avoid copying large data structures, | |
| // particularly beneficial when dealing with blocks containing many transactions. | |
| func IsEmptyBlockRef(data *coretypes.Data, _ uint64) bool { | |
| if data == nil { | |
| return true | |
| } | |
| return len(data.Txs) == 0 | |
| } |
This will be helpful in Celestia-node to avoid copying the block when checking if it's empty (cherry picked from commit 2ca0feb)
| // IsEmptyBlockRef returns true if the application considers the given block data | ||
| // empty at a given version. | ||
| // This method passes the block data by reference for improved performance. | ||
| func IsEmptyBlockRef(data *coretypes.Data, _ uint64) bool { |
There was a problem hiding this comment.
what's the second argument for? Do we still need to keep it around? Also if we're not planning on backporting this can we just break the go API?
There was a problem hiding this comment.
what's the second argument for?
in case at some point we want to have specific method for each version. For now, it's not used and not sure why it was added in first place
Also if we're not planning on backporting this can we just break the go API?
I'm backporting this to v3 to be released and used in node
There was a problem hiding this comment.
- It's for app version
- We are backporting it
) (#4064) This will be helpful in Celestia-node to avoid copying the block when checking if it's empty<hr>This is an automatic backport of pull request #4063 done by [Mergify](https://mergify.com). Co-authored-by: CHAMI Rachid <[email protected]>
| // This method passes the block data by reference for improved performance. | ||
| func IsEmptyBlockRef(data *coretypes.Data, _ uint64) bool { |
There was a problem hiding this comment.
nit: there are no references in Golang, only pointers
see unsafe.Pointer or reflect.PointerTo
This will be helpful in Celestia-node to avoid copying the block when checking if it's empty