feat(store): support pruning store #2208
Conversation
WalkthroughThis update introduces a configurable pruning mechanism to the node's data storage system. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Node (CLI)
participant Config
participant AsyncPruningStore
participant Store
User->>Node (CLI): Start node with pruning flags
Node (CLI)->>Config: Parse flags, build NodeConfig with PruningConfig
Node (CLI)->>AsyncPruningStore: Initialize with Store and PruningConfig
AsyncPruningStore->>Store: Delegates most storage operations
loop On new block
Node (CLI)->>AsyncPruningStore: SaveBlockData(header, data, sig)
AsyncPruningStore->>AsyncPruningStore: Update latest block height
end
loop Periodic pruning interval
AsyncPruningStore->>AsyncPruningStore: Trigger PruneBlockData(latestBlockHeight)
AsyncPruningStore->>Store: DeleteBlockData(old heights)
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
RollkitBot
requested review from
a team,
tuxcanfly,
tzdybal and
gupadhyaya
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/config/pruning_config.go (3)
10-11: Fix typo in comment: "offen" should be "often"There's a typo in the comment for
FlagPruningIntervalthat should be corrected.- // FlagPruningInterval is a flag for specifying how offen prune blocks store + // FlagPruningInterval is a flag for specifying how often to prune blocks store
49-49: Fix typo in comment: "offen" should be "often"Similar to the earlier flag comment, there's a typo in the PruningConfig struct field comment.
- Interval uint64 `mapstructure:"interval" yaml:"interval" comment:"how offen the pruning process should run"` + Interval uint64 `mapstructure:"interval" yaml:"interval" comment:"how often the pruning process should run"`
45-54: Consider adding validation for configuration valuesThere's no validation for the values in the
PruningConfigstruct. It might be useful to add a validation method to ensure that:
- The strategy is one of the predefined constants
KeepRecentis at least 2 for non-nonestrategiesIntervalis greater than 0This would prevent misconfigurations that could lead to data loss or performance issues.
pkg/store/pruning.go (5)
67-70: Incorrect comment for GetSignatureByHash methodThe method comment doesn't match the function's behavior. It mentions returning a signature for a block at a given height, but the method actually returns a signature by block hash.
-// GetSignatureByHash returns signature for a block at given height, or error if it's not found in Store. +// GetSignatureByHash returns signature for a block with given block header hash, or error if it's not found in Store.
72-75: Incorrect comment for GetSignature methodThe method comment doesn't match the function's behavior. It mentions returning a signature for a block with a given hash, but the method actually returns a signature by block height.
-// GetSignature returns signature for a block with given block header hash, or error if it's not found in Store. +// GetSignature returns signature for a block at given height, or error if it's not found in Store.
115-118: Consider logging pruning operationsAdding logging when pruning operations occur would help with debugging and monitoring the pruning process. Consider adding log statements that indicate when pruning is skipped and when it's performed, including how many blocks are pruned.
100-132: Consider implementing batch deletion for better performanceThe current implementation deletes blocks one by one in a loop, which might not be efficient for large pruning operations. Consider implementing a batch deletion mechanism if supported by the underlying store, or at least adding a mechanism to limit the number of blocks pruned in a single operation to avoid long-running transactions.
124-129: Add recovery mechanism for partially completed pruningIf pruning fails partway through (e.g., due to a crash or error), there's no mechanism to recover or continue from where it left off. Consider adding a way to track pruning progress, so it can be resumed if interrupted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/config/config.go(2 hunks)pkg/config/defaults.go(1 hunks)pkg/config/pruning_config.go(1 hunks)pkg/store/pruning.go(1 hunks)pkg/store/store.go(2 hunks)pkg/store/types.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/config/defaults.go (1)
pkg/config/pruning_config.go (1)
PruningConfigDefault(27-31)
pkg/config/config.go (2)
pkg/config/pruning_config.go (4)
PruningConfig(45-54)FlagPruningStrategy(7-7)FlagPruningKeepRecent(9-9)FlagPruningInterval(11-11)node/node.go (1)
Node(21-25)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Run Unit Tests
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/config/defaults.go (1)
54-54: Integration of pruning configuration looks good.The pruning configuration has been correctly added to the default node configuration, initialized with the
PruningConfigDefaultvalues from the pruning_config.go file.pkg/store/types.go (2)
19-20: Good addition of block deletion capability.This new method is essential for implementing the pruning functionality. The method signature and documentation are clear and consistent with the rest of the interface.
50-54: Well-designed PruningStore interface.The new interface follows good design practices:
- Embeds the existing Store interface for composition
- Adds only the specific pruning functionality needed
- Allows for clean separation between basic and pruning-capable stores
This design makes it easy to wrap an existing Store implementation with pruning capabilities.
pkg/store/store.go (2)
95-95: Improved error message clarity.The error message now provides more context about the operation that failed.
140-140: Valid optimization opportunity identified.The TODO comment correctly identifies a potential optimization - avoiding unnecessary marshaling/unmarshaling operations. This would be a good future improvement.
pkg/config/config.go (1)
173-174: Pruning field added appropriately to NodeConfig.The new field is properly tagged for serialization with mapstructure and yaml, consistent with other fields in the struct.
pkg/config/pruning_config.go (2)
27-31:❓ Verification inconclusive
Verify if the default KeepRecent value is appropriate
The default configuration keeps 362,880 blocks which is quite high. Assuming 1-minute blocks, this would retain approximately a year of block data. Consider if this is the appropriate default value for most users or if a smaller value would be more storage-efficient.
🏁 Script executed:
#!/bin/bash # Check if there's any documentation or comments elsewhere in the code that explains this specific value rg -i "362880" --type goLength of output: 75
Confirm rationale for default KeepRecent retention
I didn’t find any comments or docs explaining why KeepRecent is set to 362,880 (~1 year at 1‑minute intervals). This may be higher than most users need:
• pkg/config/pruning_config.go:29 (KeepRecent: 362880)
Please verify this aligns with your storage and performance goals. If not, consider reducing the default or documenting the reasoning.
32-36:❓ Verification inconclusive
Verify the safety of keeping only 2 recent blocks
The
PruningConfigEverythingstrategy keeps only 2 recent blocks. This seems very aggressive and could potentially cause issues if the node needs historical blocks for state synchronization or verification. Consider adding a comment explaining the risks of this strategy.
🏁 Script executed:
#!/bin/bash # Check if there's any minimum safety threshold mentioned elsewhere in the codebase rg -i "minimum.*blocks|minimum.*height" --type goLength of output: 51
No documented minimum for KeepRecent
I wasn’t able to find any existing minimum‐blocks safety threshold in the codebase. Keeping only 2 recent blocks is very aggressive and may break state sync or light‑client verification. Please review and ensure this value is safe for your use case:
- File: pkg/config/pruning_config.go (lines 32–36)
- Consider raising
KeepRecentto a more conservative default (e.g. 100)- Add a comment (or README entry) explaining the risks of aggressive pruning
- Optionally enforce a minimum threshold in code or via config validation
pkg/store/pruning.go (1)
44-50: LGTM: Good practice calling PruneBlockData before saving new blocksThe implementation correctly calls pruning before saving new blocks, which ensures that storage is managed efficiently.
add comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/store/pruning.go (1)
120-123:⚠️ Potential issueCritical bug in startHeight calculation
There's a significant logic error in the calculation of
startHeight. Themin(0, endHeight-s.Config.KeepRecent)will always return 0 since the first argument is 0 and the second is likely to be negative. This means it will always start pruning from height 0, potentially causing permanent data loss.Additionally, the comment on line 120 states "Must keep at least 2 blocks" but the code is using
s.Config.KeepRecentwhich could be any value.Apply this fix:
- // Must keep at least 2 blocks(while strategy is everything). + // Calculate pruning range: keep blocks from (endHeight-KeepRecent) to height endHeight := height - s.Config.KeepRecent - startHeight := min(0, endHeight-s.Config.KeepRecent) + startHeight := uint64(1) // Start from the first block + // Avoid deleting necessary blocks if endHeight is too low + if endHeight <= startHeight { + return nil + }
🧹 Nitpick comments (1)
pkg/store/pruning.go (1)
124-129: Improve error handling for block deletion.The current implementation stops pruning at the first error. Consider adding more robust error handling to continue pruning even if individual block deletions fail, perhaps by logging errors and continuing.
for i := startHeight; i <= endHeight; i++ { err = s.DeleteBlockData(ctx, i) if err != nil { - return err + // Log the error but continue pruning + // Consider adding a logger field to the struct + // s.logger.Error("Failed to delete block", "height", i, "error", err) + continue } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/store/pruning.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/store/pruning.go (5)
pkg/store/types.go (2)
Store(10-48)PruningStore(50-54)pkg/config/pruning_config.go (2)
PruningConfig(45-54)PruningConfigStrategyNone(15-15)types/signed_header.go (1)
SignedHeader(22-27)types/block.go (2)
Data(33-36)Signature(39-39)types/state.go (1)
State(19-38)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: lint / markdown-lint
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/store/pruning.go (5)
11-15: Good struct design with clean separation of concerns.The
DefaultPruningStorestruct embedsStoreand maintains a separateConfigfield, demonstrating good composition principles and separation of concerns.
19-25: LGTM: Constructor is clean and well-designed.This constructor properly initializes the
DefaultPruningStorestruct and returns it as aPruningStoreinterface type, adhering to interface-based design principles.
42-50: Good implementation of pruning integration with SaveBlockData.Calling
PruneBlockDatabefore saving a block ensures pruning happens regularly during normal operation. The error handling and wrapping are also well-implemented.
100-119: Pruning trigger logic looks good.The implementation correctly:
- Skips pruning if strategy is set to "none"
- Skips pruning if not at the configured interval
- Skips pruning if height is less than or equal to the configured
KeepRecentvalueThis ensures pruning only occurs when appropriate and prevents pruning blocks that should be kept.
27-98: LGTM: Delegation methods are correctly implemented.All the methods that delegate to the embedded
Storeimplementation are correctly forwarding their calls. This pattern ensures that the pruning store maintains the same interface while adding pruning functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/store/pruning.go (1)
58-60: Misleading comment about keeping "at least 2 blocks"The comment states "Must keep at least 2 blocks" but the code is using
s.Config.KeepRecentwhich could be configured to any value. This comment should be updated to match the actual implementation.- // Must keep at least 2 blocks(while strategy is everything). + // Calculate pruning boundary: keep the most recent s.Config.KeepRecent blocks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/pruning.go(1 hunks)pkg/store/types.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/store/types.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/store/pruning.go (5)
pkg/store/types.go (2)
Store(10-48)PruningStore(50-54)pkg/config/pruning_config.go (2)
PruningConfig(45-54)PruningConfigStrategyNone(15-15)pkg/store/store.go (1)
DefaultStore(27-29)types/signed_header.go (1)
SignedHeader(22-27)types/block.go (2)
Data(33-36)Signature(39-39)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/store/pruning.go (5)
12-16: Good structural design with embedded interfaceThe
DefaultPruningStoredesign appropriately embeds theStoreinterface and adds a configuration field for pruning. This approach follows the composition over inheritance principle and allows for clean extension of the storage functionality.
18-18: LGTM - Good compile-time interface verificationUsing the
var _ PruningStore = &DefaultPruningStore{}pattern ensures at compile-time that the struct implements the required interface.
20-26: LGTM - Clean constructor implementationThe constructor properly initializes the
DefaultPruningStoreby wrapping the provided datastore in aDefaultStoreand setting the pruning configuration.
28-36: LGTM - Proper error handling in SaveBlockDataGood approach to run pruning before saving new block data, with proper error handling.
54-56: LGTM - Appropriate pruning conditionsThe conditions for skipping pruning are appropriate - checking the interval and ensuring there are enough blocks to maintain the required history.
|
Addressed the question. Not sure it's best to prune blocks while calling |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
I think SaveBlockData must do in main progress. If async saving new blocks, there is a risk for losing some new blocks.
|
pkg/store/pruning.go
Outdated
| type AsyncPruningStore struct { | ||
| PruningStore | ||
|
|
||
| latestBlockDataHeight uint64 | ||
| flushInterval time.Duration | ||
| } |
There was a problem hiding this comment.
The AsyncPruningStore implementation has a potential thread safety issue. The latestBlockDataHeight field is updated in SaveBlockData() and read in a separate goroutine started by Start(), but there's no synchronization mechanism (such as a mutex) to protect this shared state. This could lead to race conditions where the pruning goroutine might operate on an inconsistent view of the latest height.
Consider adding a mutex to protect access to latestBlockDataHeight or using atomic operations to ensure thread-safe updates and reads of this value:
import "sync/atomic"
// Use atomic.Uint64 for thread-safe access
var latestHeight atomic.Uint64
// When updating
latestHeight.Store(newValue)
// When reading
currentHeight := latestHeight.Load()| type AsyncPruningStore struct { | |
| PruningStore | |
| latestBlockDataHeight uint64 | |
| flushInterval time.Duration | |
| } | |
| type AsyncPruningStore struct { | |
| PruningStore | |
| latestBlockDataHeight atomic.Uint64 | |
| flushInterval time.Duration | |
| } | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
pkg/store/pruning.go
Outdated
| func (s *AsyncPruningStore) Start(ctx context.Context) { | ||
| // todo: use ctx for cancellation | ||
| ticker := time.NewTicker(s.flushInterval) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| // Currently PruneBlockData only returns nil. | ||
| _ = s.PruneBlockData(ctx, s.latestBlockDataHeight) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The Start method should check if the context is already canceled before entering the loop. If ctx.Done() is triggered immediately after calling Start(), the ticker will be created but the deferred ticker.Stop() won't execute until the function returns, which could lead to a goroutine leak.
Consider adding a check at the beginning:
func (s *AsyncPruningStore) Start(ctx context.Context) {
if ctx.Err() != nil {
return
}
ticker := time.NewTicker(s.flushInterval)
defer ticker.Stop()
// rest of the function...
}This ensures resources are properly cleaned up when the context is already canceled.
| func (s *AsyncPruningStore) Start(ctx context.Context) { | |
| // todo: use ctx for cancellation | |
| ticker := time.NewTicker(s.flushInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| // Currently PruneBlockData only returns nil. | |
| _ = s.PruneBlockData(ctx, s.latestBlockDataHeight) | |
| } | |
| } | |
| } | |
| func (s *AsyncPruningStore) Start(ctx context.Context) { | |
| if ctx.Err() != nil { | |
| return | |
| } | |
| ticker := time.NewTicker(s.flushInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| // Currently PruneBlockData only returns nil. | |
| _ = s.PruneBlockData(ctx, s.latestBlockDataHeight) | |
| } | |
| } | |
| } | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
block/async_store.go (1)
12-17: Consider more descriptive field naming.The struct design is sound, but the field name
pscould be more descriptive for better code readability.type AsyncPruner struct { - ps store.PruningStore + pruningStore store.PruningStore flushInterval time.Duration }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
block/async_store.go(1 hunks)node/full.go(4 hunks)node/light.go(1 hunks)pkg/store/pruning.go(1 hunks)pkg/store/types.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- node/light.go
- pkg/store/types.go
- node/full.go
- pkg/store/pruning.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
block/async_store.go (1)
pkg/store/types.go (1)
PruningStore(50-54)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Summary
🔇 Additional comments (3)
block/async_store.go (3)
1-8: LGTM - Clean package structure and appropriate imports.The package declaration and imports are well-organized and include only the necessary dependencies for the asynchronous pruning functionality.
10-10: LGTM - Reasonable default flush interval.A 1-second default flush interval strikes a good balance between pruning frequency and system performance.
19-29: LGTM - Well-implemented constructor with proper validation.The constructor properly validates the flush interval and provides a sensible default, following good defensive programming practices.
|
Still checking for pruning range, could review without it @alpe @tac0turtle |
| } | ||
| if err := batch.Commit(ctx); err != nil { | ||
| return fmt.Errorf("failed to commit batch: %w", err) | ||
| } |
There was a problem hiding this comment.
how do we ensure that we delete everything? there might be more prefix stores in the future?
There was a problem hiding this comment.
Structures like block and header may need to refactor in future
I think currently it's enough to be same with SaveBlockData
| startHeight = endHeight - s.Config.Interval | ||
| } | ||
|
|
||
| for i := startHeight; i < endHeight; i++ { |
There was a problem hiding this comment.
As this is a public method, it can be called by other threads as well. It would not cost much to exit early when pruning is not completed.
|
Thanks for review @alpe. Addressed some comments |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
block/async_store.go(1 hunks)node/full.go(4 hunks)pkg/cmd/run_node.go(1 hunks)pkg/config/pruning_config.go(1 hunks)pkg/store/pruning.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cmd/run_node.go
- node/full.go
- pkg/store/pruning.go
- pkg/config/pruning_config.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
block/async_store.go (1)
pkg/store/types.go (1)
PruningStore(50-54)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Summary
🔇 Additional comments (4)
block/async_store.go (4)
1-9: Package declaration and imports look good.The package structure and imports are clean and follow Go conventions.
11-11: Default flush interval is well-chosen.The 10-second interval aligns with the block time defaults and provides a reasonable balance for pruning frequency.
13-19: Well-designed struct with clear purpose.The AsyncPruner struct contains appropriate fields and the documentation clearly explains its purpose.
21-28: Constructor is properly implemented.The NewAsyncPruner constructor follows Go conventions and correctly initializes all required fields.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The code changes introduce a new store type, DefaultPruningStore, to prune storage, along with an AsyncPruner to periodically prune block data in the background. The changes also include command-line flags for pruning strategy, number of recent blocks to keep, and pruning operation frequency. The review identified a critical issue in the endHeight calculation in pkg/store/pruning.go that would prevent blocks from being pruned, and a critical issue in pkg/store/store.go where the hash-to-height index was not being deleted, leading to dangling index entries. Additionally, the review identified a high severity issue in pkg/store/pruning.go where errors from s.DeleteBlockData were being ignored.
| } | ||
|
|
||
| // Must keep at least 2 blocks(while strategy is everything). | ||
| endHeight := height - 1 - s.config.KeepRecent |
There was a problem hiding this comment.
The calculation for endHeight seems to be off by one, which will result in not pruning a block that should be pruned.
If we want to keep KeepRecent blocks, the highest block to prune is height - KeepRecent. The loop for i := startHeight; i < endHeight; i++ prunes up to endHeight - 1.
With the current logic, endHeight is height - 1 - s.config.KeepRecent. This means the loop prunes up to height - s.config.KeepRecent - 2, leaving one block unpruned.
To fix this, endHeight should be height - s.config.KeepRecent. This makes the loop prune up to height - s.config.KeepRecent - 1, which is what we want.
| endHeight := height - 1 - s.config.KeepRecent | |
| endHeight := height - s.config.KeepRecent |
| func (s *DefaultStore) DeleteBlockData(ctx context.Context, height uint64) error { | ||
| batch, err := s.db.Batch(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create a new batch for deleting block data: %w", err) | ||
| } | ||
|
|
||
| if err := batch.Delete(ctx, ds.NewKey(getHeaderKey(height))); err != nil { | ||
| return fmt.Errorf("failed to delete header blob in batch: %w", err) | ||
| } | ||
| if err := batch.Delete(ctx, ds.NewKey(getDataKey(height))); err != nil { | ||
| return fmt.Errorf("failed to delete data blob in batch: %w", err) | ||
| } | ||
| if err := batch.Delete(ctx, ds.NewKey(getSignatureKey(height))); err != nil { | ||
| return fmt.Errorf("failed to delete signature of block blob in batch: %w", err) | ||
| } | ||
| if err := batch.Commit(ctx); err != nil { | ||
| return fmt.Errorf("failed to commit batch: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The current implementation of DeleteBlockData does not delete the hash-to-height index entry that is created in SaveBlockData. This will lead to dangling index entries pointing to non-existent blocks, which can cause issues when trying to retrieve blocks by hash.
To fix this, DeleteBlockData should first retrieve the block header to get its hash, and then delete the corresponding index entry along with the other block data.
I've also added handling for ds.ErrNotFound when fetching the header, as the block might have already been pruned.
func (s *DefaultStore) DeleteBlockData(ctx context.Context, height uint64) error {
// Get the header to retrieve the block hash for index deletion.
header, err := s.GetHeader(ctx, height)
if err != nil {
// If block not found, it might have been already pruned.
if errors.Is(err, ds.ErrNotFound) {
return nil
}
return fmt.Errorf("failed to get header for block %d: %w", height, err)
}
hash := header.Hash()
batch, err := s.db.Batch(ctx)
if err != nil {
return fmt.Errorf("failed to create a new batch for deleting block data: %w", err)
}
if err := batch.Delete(ctx, ds.NewKey(getHeaderKey(height))); err != nil {
return fmt.Errorf("failed to delete header blob in batch: %w", err)
}
if err := batch.Delete(ctx, ds.NewKey(getDataKey(height))); err != nil {
return fmt.Errorf("failed to delete data blob in batch: %w", err)
}
if err := batch.Delete(ctx, ds.NewKey(getSignatureKey(height))); err != nil {
return fmt.Errorf("failed to delete signature of block blob in batch: %w", err)
}
if err := batch.Delete(ctx, ds.NewKey(getIndexKey(hash))); err != nil {
return fmt.Errorf("failed to delete index key in batch: %w", err)
}
if err := batch.Commit(ctx); err != nil {
return fmt.Errorf("failed to commit batch: %w", err)
}
return nil
}| for i := startHeight; i < endHeight; i++ { | ||
| // Could ignore for errors like not found. | ||
| _ = s.DeleteBlockData(ctx, i) | ||
| } |
There was a problem hiding this comment.
Errors from s.DeleteBlockData are being ignored. The comment suggests that ErrNotFound could be ignored, but other errors (like issues with the underlying datastore) should be handled.
Ignoring these errors can hide serious problems and leave the pruning process in an incomplete state without any notification. The error should be propagated up to the AsyncPruner, which will log it.
With the recommended changes to DeleteBlockData in pkg/store/store.go, it will correctly handle ErrNotFound by returning nil, so any error returned from it will be significant.
if err := s.DeleteBlockData(ctx, i); err != nil {
return err
}
Add new store type DefaultPruningStore to prune storage.
Related: #2093
Summary by CodeRabbit