-
Notifications
You must be signed in to change notification settings - Fork 806
chore: update blockdb to use HeightIndex interface and errors in database pkg #4318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR aligns the blockdb Database with the database.HeightIndex
interface by renaming public methods and updating file naming conventions to be more generic rather than block-specific.
- Renamed public methods
WriteBlock
,ReadBlock
, andHasBlock
toPut
,Get
, andHas
respectively - Updated file naming from block-specific to generic (e.g.,
blockdb.idx
todb.idx
) - Renamed
BlockEntryVersion
constant toDataEntryVersion
to reflect generic data storage
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
x/blockdb/database.go | Core database implementation with method renames and constant updates |
x/blockdb/writeblock_test.go | Test file updated to use new Put method instead of WriteBlock |
x/blockdb/readblock_test.go | Test file updated to use new Get method instead of ReadBlock |
x/blockdb/recovery_test.go | Recovery tests updated with new method names and DataEntryVersion constant |
x/blockdb/datasplit_test.go | Data splitting tests updated to use new Put and Get methods |
x/blockdb/database_test.go | Database tests updated with new method names |
Comments suppressed due to low confidence (2)
x/blockdb/readblock_test.go:1
- The error message still references the old method name 'ReadBlock' but should be updated to 'Get' to match the renamed method.
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.
x/blockdb/recovery_test.go:1
- [nitpick] The error message refers to ErrBlockNotFound but this constant should likely be renamed to ErrDataNotFound or similar to align with the generic nature of the database as mentioned in the PR description.
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
e98ed7a
to
6326739
Compare
Once #4212 is merged in. I'll also add the common interface tests to blockdb tests to ensure consistent behaviour. |
af3d8b1
to
e3652bd
Compare
ebef129
to
3663e7f
Compare
2ef8932
to
11cf2a0
Compare
11cf2a0
to
4461ccf
Compare
x/blockdb/writeblock_test.go
Outdated
// Verify all written blocks are readable and data is correct | ||
for h, expectedBlock := range blocksWritten { | ||
readBlock, err := store.ReadBlock(h) | ||
require.NoError(t, err, "ReadBlock failed at height %d", h) | ||
readBlock, err := store.Get(h) | ||
require.NoError(t, err, "Get failed at height %d", h) | ||
require.Equal(t, expectedBlock, readBlock) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We test this behavior in TestWriteBlock_Basic that a block inserted via Put is returned through Get. It feels like we're duplicating coverage here, since it seems that this test is intended to verify heights.
Some ideas might be to either:
- Refactor the tests to share the same test table, since it seems that testing the correctness of Put requires us to verify behavior on Get and the height apis
- Remove this assertion entirely from this test, so we'd have a single test for Put/Get and one for Put/heights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 2 makes sense here. The basic Put/Get tests should be covered and we can focus this tests on just the height verification. I'll remove this dup assertion to keep this tests more focused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. We might also want to rename the tests here to reflect that the former is testing the contract between Put/Get and this one is testing the contract between Put/Height.
Why this should be merged
Follow up to #4133, update
x/blockdb
Database to align with thedatabase.HeightIndex
interface and use the errors fromdatabase/errors.go
for database closed and not found.How this works
ErrDatabaseClosed
andErrBlockNotFound
with the common errors indatabase/errors.go
How this was tested
Unit tests
Need to be documented in RELEASES.md?
No