Skip to content
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

[Indexer] Add index storage interfaces and base in-memory implementation #34

Merged
merged 80 commits into from
Feb 27, 2024

Conversation

sideninja
Copy link
Contributor

Related: #17

This is the first part of the PRs that will implement persistent storage. An interfaces have been defined for the indexers as well as a simple in-memory implementation that satisfies the definitions. A simple test suite has been added that validates expected and defined behaviour of the interface implementations.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Copy link
Contributor

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

Looks good to me, just some minor comments

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

mostly comments about the memory storage which is temporary. feel free to ignore if you're planning to move away from it quickly

storage/errors/error.go Show resolved Hide resolved
storage/index.go Show resolved Hide resolved
// Retrieve the block using the blockHeightsIDs map
blockID, exists := s.base.blockHeightsIDs[height]
if !exists {
return nil, errors.NotFound
Copy link
Contributor

Choose a reason for hiding this comment

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

this would indicate an inconsistent state. log an error?

// Retrieve the block using the blocksIDs map
block, exists := s.base.blocksIDs[blockID]
if !exists {
return nil, errors.NotFound
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

s.base.firstHeight = block.Height
}
if s.base.lastHeight == unknownHeight || block.Height > s.base.lastHeight {
s.base.lastHeight = block.Height
Copy link
Contributor

Choose a reason for hiding this comment

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

does it matter if blocks are store sequentially?

r.base.receiptBlockIDTxIDs[receipt.BlockHash] = receipt.TxHash

if _, ok := r.base.bloomHeight[receipt.BlockNumber.Int64()]; ok {
return errors.Duplicate
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would mean that there were 2 receipts with the same ID but different BlockNumber. That seems like an inconsistent state


receipt, exists := r.base.receiptsTxIDs[txID]
if !exists {
return nil, errors.NotFound
Copy link
Contributor

Choose a reason for hiding this comment

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

this would indicate an inconsistent state. log an error?

r.base.mu.RLock()
defer r.base.mu.RUnlock()

if start.Cmp(end) > -1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
if start.Cmp(end) > -1 {
if start.Cmp(end) >= 0 {

@sideninja sideninja merged commit 344f20f into main Feb 27, 2024
1 check failed
@m-Peter m-Peter deleted the gregor/index/storage-index branch March 8, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants