Skip to content

Conversation

DracoLi
Copy link
Contributor

@DracoLi DracoLi commented Aug 26, 2025

Why this should be merged

For tests where we don't need to use on an on-disk block store, we can use this blockdb.MemoryDatabase.
For example, #4208 uses this MemoryDatabase to test the meterblockdb

How this works

Use blockdb.MemoryDatabase to create a block database that implements database.BlockDatabase. It writes and reads blocks from memory.

How this was tested

Tests that uses the MemoryDatabase. Currently just in #4208

Need to be documented in RELEASES.md?

No

@DracoLi DracoLi marked this pull request as ready for review August 28, 2025 17:51
@Copilot Copilot AI review requested due to automatic review settings August 28, 2025 17:51
Copy link
Contributor

@Copilot Copilot AI left a 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 introduces an in-memory implementation of the BlockDatabase interface for testing purposes, allowing tests to avoid using on-disk storage when not necessary.

  • Adds a new MemoryDatabase struct that implements the BlockDatabase interface
  • Provides thread-safe operations using read-write mutexes
  • Includes proper error handling for closed database states and empty blocks

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@DracoLi DracoLi requested review from a team and joshua-kim and removed request for a team August 28, 2025 19:58
}

// WriteBlock stores a block in memory
func (m *MemoryDatabase) WriteBlock(height BlockHeight, block BlockData) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blockData is a slice so why do we copy it and not just save the reference? This is for tests. Do we expect the original slice to be changed?

Copy link
Contributor Author

@DracoLi DracoLi Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't expect it to be changed in tests but it definitely could, which might cause confusion when debugging.
I opted to copy it since that mirrors the block database behaviour.

@DracoLi DracoLi requested a review from joshua-kim September 2, 2025 18:37
@joshua-kim joshua-kim moved this to Backlog 🧊 in avalanchego Sep 8, 2025
@joshua-kim joshua-kim moved this from Backlog 🧊 to In Progress 🏗️ in avalanchego Sep 9, 2025
@joshua-kim joshua-kim moved this from In Progress 🏗️ to In Review 🔎 in avalanchego Sep 9, 2025
defer m.mu.Unlock()

m.closed = true
m.blocks = nil
Copy link
Contributor

@joshua-kim joshua-kim Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random comment: I guess we don't technically have to clear out this db since all future operations are stopped and this operation is idempotent - but I don't think we care either way... so feel free to ignore this...

Copy link
Contributor Author

@DracoLi DracoLi Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting to nil does allow the blocks to be garbage collected in case the db reference is still around.

@DracoLi DracoLi requested a review from joshua-kim September 9, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review 🔎
Development

Successfully merging this pull request may close these issues.

3 participants