Test: zstd + combined layout #3333
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces zstd compression for the Pebble database and implements a migration to combine transactions and receipts into a single database layout. The migration consolidates the previously separate TransactionsByBlockNumberAndIndex and ReceiptsByBlockNumberAndIndex buckets into a single BlockTransactions bucket.
Key changes:
- Adds zstd compression configuration to database initialization
- Implements a new migration to consolidate transaction and receipt storage
- Refactors accessor functions to use the new combined layout
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| node/node.go | Adds zstd compression configuration to database initialization |
| cmd/juno/dbcmd.go | Adds zstd compression configuration for db command |
| db/buckets.go | Defines new BlockTransactions bucket constant |
| core/typed_buckets.go | Defines typed bucket for BlockTransactions |
| core/block_transaction.go | Adds new BlockTransactions struct for combined storage |
| core/accessors.go | Refactors accessor functions to use new combined layout |
| blockchain/blockchain.go | Updates blockchain calls to use new accessor APIs |
| migration/migration.go | Returns context error to properly handle cancellation |
| migration/blocktransactions/*.go | Implements new migration with concurrent batch processing |
| db/pebblev2/batch.go | Adds Close method to batch type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
|
|
||
| counter.log(batch, batchSize) |
There was a problem hiding this comment.
The counter is being updated concurrently from multiple goroutines without synchronization. The counter.log method at line 246 is called from within a callback that runs in a goroutine pool (batchConcurrency=4), but the counter struct has no mutex protection. This can lead to race conditions when updating c.size and c.count fields.
| return 0, false, nil | ||
| } | ||
|
|
||
| minBlock := min(transactions, receipts) |
There was a problem hiding this comment.
The min function is called when one or both of the values might be zero (when hasTransactions or hasReceipts is false). This could result in using an uninitialized value. The logic should handle the case where only one bucket has data by checking hasTransactions and hasReceipts flags individually before computing the minimum.
| minBlock := min(transactions, receipts) | |
| var minBlock uint64 | |
| if hasTransactions && hasReceipts { | |
| minBlock = min(transactions, receipts) | |
| } else if hasTransactions { | |
| minBlock = transactions | |
| } else { // hasReceipts must be true here | |
| minBlock = receipts | |
| } |
| return | ||
| } | ||
|
|
||
| counter.log(batch, batchSize) |
There was a problem hiding this comment.
The counter is being incremented by batchSize regardless of how many blocks were actually processed. If some blocks are skipped in the batch (as indicated by the shouldMigrate check in createBatch), this will result in inaccurate block count reporting. The actual number of blocks written should be passed instead.
| counter.log(batch, batchSize) | |
| counter.log(batch, batch.Len()) |
| if len(transactions) == 0 || len(receipts) == 0 { | ||
| has, err := core.BlockTransactionsBucket.Has(database, blockNumber) | ||
| if err != nil { | ||
| return blockTransactions{}, false, err | ||
| } | ||
| if has { | ||
| return blockTransactions{}, false, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The condition uses OR logic (||) but should likely use AND logic (&&). If either transactions or receipts is empty (but not both), the function will check if the block already exists in the new format and skip it. However, this means that blocks with only one of transactions or receipts in the old format would be partially migrated. The check should ensure both are empty before determining the block has already been migrated.
| cleanup.AddError(err) | ||
| return | ||
| } | ||
|
|
||
| debugLog(logger, "writing block", startBlock, endBlock) | ||
| if err := batch.Write(); err != nil { | ||
| cleanup.AddError(err) |
There was a problem hiding this comment.
When an error occurs during batch creation (line 235) or during batch write (line 241-243), the batch is not closed before returning. This will leak the batch resource. The batch should be closed in both error paths.
| cleanup.AddError(err) | |
| return | |
| } | |
| debugLog(logger, "writing block", startBlock, endBlock) | |
| if err := batch.Write(); err != nil { | |
| cleanup.AddError(err) | |
| cleanup.AddError(err) | |
| cleanup.CloseBatch(batch) | |
| return | |
| } | |
| debugLog(logger, "writing block", startBlock, endBlock) | |
| if err := batch.Write(); err != nil { | |
| cleanup.AddError(err) | |
| cleanup.CloseBatch(batch) |
| return blockTransactions.Receipts[index], nil | ||
| } | ||
|
|
||
| func DeleteTxsAndReceipts(batch db.IndexedBatch, blockNum, numTxs uint64) error { |
There was a problem hiding this comment.
The numTxs parameter is no longer used in the refactored DeleteTxsAndReceipts function since the transaction count is now determined from the blockTransactions struct. Consider removing this unused parameter to simplify the API.
| func DeleteTxsAndReceipts(batch db.IndexedBatch, blockNum, numTxs uint64) error { | |
| func DeleteTxsAndReceipts(batch db.IndexedBatch, blockNum uint64) error { |
|
|
||
| now := time.Now() | ||
| elapsed := now.Sub(c.start).Seconds() | ||
| if elapsed > float64(timeLogRate.Seconds()) { |
There was a problem hiding this comment.
The variable timeLogRate is referenced without the receiver qualifier c.. This should be c.timeLogRate.Seconds() instead of timeLogRate.Seconds().
| if elapsed > float64(timeLogRate.Seconds()) { | |
| if elapsed > float64(c.timeLogRate.Seconds()) { |
| for blockNumber := startBlock; blockNumber <= endBlock; blockNumber++ { | ||
| blockTransactions, shouldMigrate, err := readBlock(database, blockNumber) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
When an error occurs after creating a batch (line 126), the batch is returned as nil but never closed, potentially leaking resources. The batch should be closed before returning the error.
| type BlockTransactions struct { | ||
| Transactions []Transaction `cbor:"1,keyasint,omitempty"` | ||
| Receipts []*TransactionReceipt `cbor:"2,keyasint,omitempty"` | ||
| } |
There was a problem hiding this comment.
The BlockTransactions type is missing a documentation comment. Public types should have a comment that describes their purpose, especially for a new database schema structure.
| } | ||
|
|
||
| func (c *cleanup) CloseBatch(batch db.Batch) { | ||
| c.closers.Go(batch.(io.Closer).Close) |
There was a problem hiding this comment.
The type assertion batch.(io.Closer) will panic if the batch does not implement io.Closer. While the Close method was added to the batch type in this PR, a safe type assertion with error checking should be used to avoid potential panics.
| c.closers.Go(batch.(io.Closer).Close) | |
| closer, ok := batch.(io.Closer) | |
| if !ok { | |
| c.AddError(errors.New("batch does not implement io.Closer")) | |
| return | |
| } | |
| c.closers.Go(closer.Close) |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (28.45%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3333 +/- ##
==========================================
- Coverage 76.05% 75.71% -0.34%
==========================================
Files 347 350 +3
Lines 32829 33006 +177
==========================================
+ Hits 24967 24991 +24
- Misses 6074 6220 +146
- Partials 1788 1795 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.