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

[Storehouse] Implement Register store #4940

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Conversation

zhangchiqing
Copy link
Member

@zhangchiqing zhangchiqing commented Nov 6, 2023

This PR implements the RegisterStore.

Closes: #4659

@zhangchiqing zhangchiqing changed the base branch from master to leo/storehouse-in-memory-regsiter-store November 6, 2023 23:04
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fa477d9) 56.23% compared to head (f7d8ede) 61.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4940      +/-   ##
==========================================
+ Coverage   56.23%   61.15%   +4.92%     
==========================================
  Files         969       20     -949     
  Lines       90431     1555   -88876     
==========================================
- Hits        50854      951   -49903     
+ Misses      35789      551   -35238     
+ Partials     3788       53    -3735     
Flag Coverage Δ
unittests 61.15% <ø> (+4.92%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zhangchiqing zhangchiqing marked this pull request as ready for review November 6, 2023 23:10
@zhangchiqing zhangchiqing force-pushed the leo/storehouse-in-memory-regsiter-store branch from 2bdcffa to 0424af0 Compare November 7, 2023 19:38

type PrunedError struct {
PrunedHeight uint64
Height uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment explaining what is the difference between height and pruned height

@sideninja
Copy link
Contributor

@zhangchiqing is this PR the same as the one I already reviewed?

@zhangchiqing zhangchiqing force-pushed the leo/storehouse-in-memory-regsiter-store branch 2 times, most recently from d66ec98 to 99bae7c Compare November 10, 2023 16:26
Base automatically changed from leo/storehouse-in-memory-regsiter-store to master November 10, 2023 19:26
Comment on lines +40 to +41
// finalizationProcessingLoop notify the register store when a block is finalized
// and handle the error if any
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// finalizationProcessingLoop notify the register store when a block is finalized
// and handle the error if any
// finalizationProcessingLoop notifies the register store when a block is finalized.

engine/execution/storehouse/register_store.go Show resolved Hide resolved
engine/execution/storehouse/register_store.go Show resolved Hide resolved
Comment on lines +170 to +173
regs, err := r.memStore.GetUpdatedRegisters(next, blockID)
if errors.Is(err, ErrNotExecuted) {
// next block is not executed yet
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

could we join the GetUpdatedRegisters an Prune calls into one call.

something like PopUpdatedRegisters

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't prune before successfully storing it in diskstore, otherwise if there is a query getting register at the pruned height, it will fail, because the in-mem store has pruned its value, and disk store has not stored it yet.

I also can't store into disk store first, because I need to retrieve the updated registers from memStore first.

// if in memory store returns PrunedError, and register height is above the pruned height,
// then it means the block is connected to the pruned block of in memory store, which is
// a finalized block and executed block, so we can get its value from on disk store.
if height > prunedError.PrunedHeight {
Copy link
Contributor

Choose a reason for hiding this comment

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

would this always be one bigger than pruned height? how could it be bigger than that?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be a few blocks bigger, not just one. For instance:

X is 3 at height 10,
X is 6 at height 15.
block 12 is finalized, now (X: 3, height: 10) is stored in OnDiskRegisterStore, and (X: 6, height: 15) is stored in InMemoryRegisterStore.
Then GetRegister(14, X) will first query memStore, which returns prunedError with prunedHeight as 12. So the height 14 is bigger than prunedHeight 12 by 2 (bigger than one), now we need to query diskStore with (X, 12), which returns 3. Meaning X at height 14 is the same as X at height 12, because X wasn't updated from height 11 to 14.

return fmt.Errorf("cannot save register to memStore: %w", err)
}

err = r.OnBlockFinalized()
Copy link
Contributor

Choose a reason for hiding this comment

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

will it be ok if the OnBlockFinalized is not run since it's being used by another goroutine?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's ok, because:

  1. another goroutine will recursively call OnBlockFinalized for the next block, until there is no more finalized blocks.
  2. it's also ok to miss a call to onBlockFinalized, because the correct register value can still be returned regardless it's stored in in-memory store or on-disk store. We are also resistant from crash-failure, because during startup recovery, we will re-execute all blocks since on-disk store's latest executed height.

return fmt.Errorf("cannot save %v registers to disk store for height %v: %w", len(regs), next, err)
}

err = r.memStore.Prune(next, blockID)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it ok to prune with every block finalized, would it be a valuable improvement if we only prune once we don't have more finalized blocks? not sure how expensive is to prune just an idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

prune should be fast and cheap, since it's just acquire a lock and updating some index in memory.

I think the performance is quite similar comparing the following two approach:

  1. pruning once to prune 3 finalized blocks
  2. pruning 3 times each time to prune 1 finalized block

@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 15, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 15, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 16, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 16, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 16, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2023
@zhangchiqing zhangchiqing added this pull request to the merge queue Nov 16, 2023
Merged via the queue into master with commit ab5b93f Nov 16, 2023
54 checks passed
@zhangchiqing zhangchiqing deleted the leo/register-store branch November 16, 2023 17:32
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.

[Storehouse] Implement RegisterStore
4 participants