x/blockdb follow-up correctness, recovery and documentation fixes

[rrazvan1]

1. Reject immutable config/header mismatches instead of silently overriding config

logConfigAndHeaderMismatches currently logs at Info and continues when the configured MinimumHeight or MaxDataFileSize differs from the persisted index header:

avalanchego/x/blockdb/database.go

Lines 1000 to 1018 in 0eb8166

	func (s *Database) logConfigAndHeaderMismatches() {
	// Some config values cannot be changed after index initialization.
	// If they do not match the index header, log an info that
	// the index header values will be used instead.
	if s.config.MinimumHeight != s.header.MinHeight {
	s.log.Info(
	"MinimumHeight in config does not match the index header. The MinimumHeight in the index header will be used.",
	zap.Uint64("configMinimumHeight", s.config.MinimumHeight),
	zap.Uint64("headerMinimumHeight", s.header.MinHeight),
	)
	}
	if s.config.MaxDataFileSize != s.header.MaxDataFileSize {
	s.log.Info(
	"MaxDataFileSize in config does not match the index header. The MaxDataFileSize in the index header will be used.",
	zap.Uint64("configMaxDataFileSize", s.config.MaxDataFileSize),
	zap.Uint64("headerMaxDataFileSize", s.header.MaxDataFileSize),
	)
	}
	}

Starting successfully with different configuration may hide an existing bug somewhere else.

2. Add a unit test for MinHeight index offsets

avalanchego/x/blockdb/database.go

Line 600 in 0eb8166

relativeHeight := height - s.header.MinHeight

Removing MinHeight is not breaking any existing test.

3. Verify that an indexed record's height matches the requested height

Database.Get reads and unmarshals a blockEntryHeader, but it does not verify that bh.Height equals the requested height:

avalanchego/x/blockdb/database.go

Lines 448 to 509 in 0eb8166

	// Get retrieves a block by its height.
	// Returns database.ErrNotFound if the block is not found.
	func (s *Database) Get(height BlockHeight) (BlockData, error) {
	s.closeMu.RLock()
	defer s.closeMu.RUnlock()
	if s.closed {
	s.log.Error("Failed Get: database closed", zap.Uint64("height", height))
	return nil, database.ErrClosed
	}
	indexEntry, err := s.readBlockIndex(height)
	if err != nil {
	return nil, err
	}
	totalReadSize, err := safemath.Add(uint64(sizeOfBlockEntryHeader), uint64(indexEntry.Size))
	if err != nil {
	return nil, fmt.Errorf("failed to compute total read size: %w", err)
	}
	buf := make([]byte, int(totalReadSize))
	// loop to retry fetching the data file if it got closed between get and read.
	// If not closed, we read the block header and data.
	for {
	dataFile, localOffset, fileIndex, err := s.getDataFileAndOffset(indexEntry.Offset)
	if err != nil {
	return nil, fmt.Errorf("failed to get data file and offset: %w", err)
	}
	if _, err := dataFile.ReadAt(buf, int64(localOffset)); err != nil {
	if errors.Is(err, os.ErrClosed) {
	s.fileCache.Evict(fileIndex)
	continue
	}
	s.log.Error("Failed to read block: failed to read block data from file",
	zap.Uint64("height", height),
	zap.Uint64("localOffset", localOffset),
	zap.Uint32("blockSize", indexEntry.Size),
	zap.Error(err),
	)
	return nil, fmt.Errorf("failed to read block header and data: %w", err)
	}
	break
	}
	var bh blockEntryHeader
	if err := bh.UnmarshalBinary(buf[:int(sizeOfBlockEntryHeader)]); err != nil {
	return nil, fmt.Errorf("failed to deserialize block header: %w", err)
	}
	compressedData := buf[int(sizeOfBlockEntryHeader):]
	decompressed, err := s.compressor.Decompress(compressedData)
	if err != nil {
	return nil, fmt.Errorf("failed to decompress block data: %w", err)
	}
	// Verify checksum on uncompressed data
	calculatedChecksum := calculateChecksum(decompressed)
	if calculatedChecksum != bh.Checksum {
	return nil, fmt.Errorf("checksum mismatch: calculated %d, stored %d", calculatedChecksum, bh.Checksum)
	}
	return decompressed, nil

If an index entry is corrupted and points to another valid record, decompression and checksum verification can succeed because the checksum covers the payload, not the requested height. Get(heightA) can therefore return height B's valid payload without reporting corruption.

4. Close indexFile when loadOrInitializeHeader fails

openAndInitializeIndex assigns the opened file to s.indexFile and immediately returns the result of loadOrInitializeHeader:

avalanchego/x/blockdb/database.go

Lines 927 to 935 in 0eb8166

	func (s *Database) openAndInitializeIndex() error {
	indexPath := filepath.Join(s.config.IndexDir, indexFileName)
	openFlags := os.O_RDWR | os.O_CREATE
	var err error
	s.indexFile, err = os.OpenFile(indexPath, openFlags, defaultFilePermissions)
	if err != nil {
	return fmt.Errorf("failed to open index file %s: %w", indexPath, err)
	}
	return s.loadOrInitializeHeader()

If opening succeeds but header loading or initialization fails, New returns without closing s.indexFile.

5. Retry Sync if the cached file was concurrently closed

Database.Sync obtains a cached data-file handle and calls f.Sync() once. A concurrent cache eviction may close the handle between those operations:

avalanchego/x/blockdb/database.go

Lines 552 to 593 in 0eb8166

	// Sync calls sync on all data files in the range [start, end],
	// assuming data are written in-order. If no data exists at start or end,
	// nothing is synced.
	func (s *Database) Sync(start, end uint64) error {
	s.closeMu.RLock()
	defer s.closeMu.RUnlock()
	if s.closed {
	s.log.Error("Failed Sync: database closed",
	zap.Uint64("start", start),
	zap.Uint64("end", end),
	)
	return database.ErrClosed
	}
	firstIdx, err := s.getDataFileIndexForHeight(start)
	if err != nil {
	if errors.Is(err, database.ErrNotFound) {
	return nil
	}
	return err
	}
	lastIdx, err := s.getDataFileIndexForHeight(end)
	if err != nil {
	if errors.Is(err, database.ErrNotFound) {
	return nil
	}
	return err
	}
	for idx := firstIdx; idx <= lastIdx; idx++ {
	f, err := s.getOrOpenDataFile(idx)
	if err != nil {
	return fmt.Errorf("failed to open data file %d: %w", idx, err)
	}
	if err := f.Sync(); err != nil {
	return fmt.Errorf("failed to sync data file %d: %w", idx, err)
	}
	}
	return nil
	}

Get and writeBlockAt already handle this by detecting os.ErrClosed, evicting the stale cache entry, reopening, and retrying.

avalanchego/x/blockdb/database.go

Lines 470 to 490 in 0eb8166

	// loop to retry fetching the data file if it got closed between get and read.
	// If not closed, we read the block header and data.
	for {
	dataFile, localOffset, fileIndex, err := s.getDataFileAndOffset(indexEntry.Offset)
	if err != nil {
	return nil, fmt.Errorf("failed to get data file and offset: %w", err)
	}
	if _, err := dataFile.ReadAt(buf, int64(localOffset)); err != nil {
	if errors.Is(err, os.ErrClosed) {
	s.fileCache.Evict(fileIndex)
	continue
	}
	s.log.Error("Failed to read block: failed to read block data from file",
	zap.Uint64("height", height),
	zap.Uint64("localOffset", localOffset),
	zap.Uint32("blockSize", indexEntry.Size),
	zap.Error(err),
	)
	return nil, fmt.Errorf("failed to read block header and data: %w", err)
	}
	break

avalanchego/x/blockdb/database.go

Lines 1087 to 1114 in 0eb8166

	// loop to retry fetching the data file if it got closed between get and write.
	// If not closed, we write the block and return.
	for {
	dataFile, localOffset, fileIndex, err := s.getDataFileAndOffset(offset)
	if err != nil {
	return fmt.Errorf("failed to get data file for writing block %d: %w", bh.Height, err)
	}
	if _, err := dataFile.WriteAt(combinedBuf, int64(localOffset)); err != nil {
	if errors.Is(err, os.ErrClosed) {
	// ensure the file is evicted, otherwise we'll retry forever
	s.fileCache.Evict(fileIndex)
	continue
	}
	return fmt.Errorf("failed to write block to data file at offset %d: %w", offset, err)
	}
	if s.config.SyncToDisk {
	if err := dataFile.Sync(); err != nil {
	if errors.Is(err, os.ErrClosed) {
	s.fileCache.Evict(fileIndex)
	continue
	}
	return fmt.Errorf("failed to sync data file after writing block %d: %w", bh.Height, err)
	}
	}
	return nil
	}

6. Document how MaxDataFiles should be correlated with the expected concurrency

DatabaseConfig.MaxDataFiles is describe as the maximum number of cached data-file descriptors:
The cache closes a file immediately when it is evicted, so when more distinct data files are accessed concurrently than the cache can store, active handles may be evicted.

7. Remove the unnecessary f != nil check in lru cache evict function

avalanchego/x/blockdb/database.go

Lines 221 to 225 in 0eb8166

	fileCache: lru.NewCacheWithOnEvict(config.MaxDataFiles, func(_ int, f *os.File) {
	if f != nil {
	f.Close()
	}
	}),

8. Concurrent Puts reserve space before writing their index entries. A later write can update the global nextDataWriteOffset while an earlier reservation is still incomplete.

After a crash, if the earlier block data is durable but its index entry is not, recovery sees the data extent equal to the checkpoint and skips scanning it.

My suggestion here is having a new field separate from nextDataWriteOffset, called committedDataWriteOffset that is persisted in the indexHeader instead of nextDataWriteOffset. So nextDataWriteOffset is used at runtime, but committedDataWriteOffset for indexHeader.
committedDataWriteOffset should only be updated after dataFile.Sync() is called.