-
Notifications
You must be signed in to change notification settings - Fork 70
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
refactor: move executer to block package #638
Merged
mtsitrin
merged 23 commits into
main
from
mtsitrin/635-refactor-move-executer-to-block-package
Apr 16, 2024
Merged
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
4270665
adding retries on hub client
mtsitrin 2eb737c
handling queries for non existing data
mtsitrin b81f064
fixed UT
mtsitrin 50fbf2b
added additional check as reponse is ptr
mtsitrin 4eb4983
moved log interface to types. moved test logger to tetutil
mtsitrin 820f234
cleaning block manager
mtsitrin 9efb82c
pr comments
mtsitrin 62e50fe
Merge remote-tracking branch 'origin/main' into mtsitrin/551-retries-…
mtsitrin 9b93160
Merge branch 'main' into cleanup
mtsitrin a4fda42
Merge branch 'mtsitrin/551-retries-for-hub-client' into mtsitrin/632-…
mtsitrin a990b38
comments
mtsitrin b0b568a
adding m.store.NextHeight()
mtsitrin 204e133
moved executer to block package
mtsitrin 9ad450b
seperate executor
mtsitrin 5949934
Merge branch 'main' into mtsitrin/635-refactor-move-executer-to-block…
mtsitrin 37d77f3
split alignStoreWithAppIfNeeded for readability
mtsitrin 975e792
minor cleanup per pr comments
mtsitrin 0839d56
more cleaning
mtsitrin 2498162
renamed BlockExecutor
mtsitrin 85d5ed6
Merge branch 'main' into mtsitrin/635-refactor-move-executer-to-block…
mtsitrin fed61e8
linter
mtsitrin 6cf934f
Apply suggestions from code review
mtsitrin 23d8c35
missing change
mtsitrin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,24 +13,31 @@ import ( | |
// applyBlock applies the block to the store and the abci app. | ||
// steps: save block -> execute block with app -> update state -> commit block to app -> update store height and state hash. | ||
// As the entire process can't be atomic we need to make sure the following condition apply before | ||
// we're applying the block in the happy path: block height - 1 == abci app last block height. | ||
// In case the following doesn't hold true, it means we crashed after the commit and before updating the store height. | ||
// In that case we'll want to align the store with the app state and continue to the next block. | ||
// - block height is the expected block height on the store (height + 1). | ||
// - block height is the expected block height on the app (last block height + 1). | ||
func (m *Manager) applyBlock(ctx context.Context, block *types.Block, commit *types.Commit, blockMetaData blockMetaData) error { | ||
if block.Header.Height != m.store.Height()+1 { | ||
// We crashed after the commit and before updating the store height. | ||
m.logger.Error("Block not applied. Wrong height", "block height", block.Header.Height, "store height", m.store.Height()) | ||
return nil | ||
// TODO (#330): allow genesis block with height > 0 to be applied. | ||
// TODO: add switch case to have defined behavior for each case. | ||
// validate block height | ||
if block.Header.Height != m.store.NextHeight() { | ||
m.logger.Error("Block not applied. wrong height", "block height", block.Header.Height, "expected height", m.store.NextHeight()) | ||
return types.ErrInvalidBlockHeight | ||
} | ||
|
||
m.logger.Debug("Applying block", "height", block.Header.Height, "source", blockMetaData.source) | ||
|
||
// Check if alignment is needed due to incosistencies between the store and the app. | ||
isAlignRequired, err := m.alignStoreWithApp(ctx, block) | ||
// Check if the app's last block height is the same as the currently produced block height | ||
isBlockAlreadyApplied, err := m.isHeightAlreadyApplied(block.Header.Height) | ||
if err != nil { | ||
return err | ||
} | ||
if isAlignRequired { | ||
// In case the following true, it means we crashed after the commit and before updating the store height. | ||
// In that case we'll want to align the store with the app state and continue to the next block. | ||
if isBlockAlreadyApplied { | ||
err := m.UpdateStateFromApp() | ||
if err != nil { | ||
return err | ||
} | ||
m.logger.Debug("Aligned with app state required. Skipping to next block", "height", block.Header.Height) | ||
return nil | ||
} | ||
|
@@ -117,17 +124,22 @@ func (m *Manager) attemptApplyCachedBlocks(ctx context.Context) error { | |
m.applyCachedBlockMutex.Lock() | ||
defer m.applyCachedBlockMutex.Unlock() | ||
|
||
prevCachedBlock, exists := m.prevBlock[m.store.Height()+1] | ||
for { | ||
expectedHeight := m.store.NextHeight() | ||
|
||
for exists { | ||
m.logger.Debug("Applying cached block", "height", m.store.Height()+1) | ||
prevCachedBlock, blockExists := m.prevBlock[expectedHeight] | ||
prevCachedCommit, commitExists := m.prevCommit[expectedHeight] | ||
|
||
err := m.applyBlock(ctx, prevCachedBlock, m.prevCommit[m.store.Height()+1], blockMetaData{source: gossipedBlock}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bug fixed as commit cache not checked |
||
if !blockExists || !commitExists { | ||
break | ||
} | ||
|
||
m.logger.Debug("Applying cached block", "height", expectedHeight) | ||
err := m.applyBlock(ctx, prevCachedBlock, prevCachedCommit, blockMetaData{source: gossipedBlock}) | ||
if err != nil { | ||
m.logger.Debug("apply previously cached block", "err", err) | ||
return err | ||
} | ||
prevCachedBlock, exists = m.prevBlock[m.store.Height()+1] | ||
} | ||
|
||
for k := range m.prevBlock { | ||
|
@@ -139,37 +151,46 @@ func (m *Manager) attemptApplyCachedBlocks(ctx context.Context) error { | |
return nil | ||
} | ||
|
||
// alignStoreWithApp is responsible for aligning the state of the store and the abci app if necessary. | ||
func (m *Manager) alignStoreWithApp(ctx context.Context, block *types.Block) (bool, error) { | ||
isRequired := false | ||
// Validate incosistency in height wasn't caused by a crash and if so handle it. | ||
// isHeightAlreadyApplied checks if the block height is already applied to the app. | ||
func (m *Manager) isHeightAlreadyApplied(blockHeight uint64) (bool, error) { | ||
proxyAppInfo, err := m.executor.GetAppInfo() | ||
if err != nil { | ||
return isRequired, errors.Wrap(err, "get app info") | ||
return false, errors.Wrap(err, "get app info") | ||
} | ||
if uint64(proxyAppInfo.LastBlockHeight) != block.Header.Height { | ||
return isRequired, nil | ||
|
||
isBlockAlreadyApplied := uint64(proxyAppInfo.LastBlockHeight) == blockHeight | ||
|
||
// TODO: add switch case to validate better the current app state | ||
|
||
return isBlockAlreadyApplied, nil | ||
} | ||
|
||
// UpdateStateFromApp is responsible for aligning the state of the store from the abci app | ||
func (m *Manager) UpdateStateFromApp() error { | ||
proxyAppInfo, err := m.executor.GetAppInfo() | ||
if err != nil { | ||
return errors.Wrap(err, "get app info") | ||
} | ||
|
||
isRequired = true | ||
m.logger.Info("Skipping block application and only updating store height and state hash", "height", block.Header.Height) | ||
appHeight := uint64(proxyAppInfo.LastBlockHeight) | ||
|
||
// update the state with the hash, last store height and last validators. | ||
m.lastState.AppHash = *(*[32]byte)(proxyAppInfo.LastBlockAppHash) | ||
m.lastState.LastStoreHeight = block.Header.Height | ||
m.lastState.LastStoreHeight = appHeight | ||
m.lastState.LastValidators = m.lastState.Validators.Copy() | ||
|
||
resp, err := m.store.LoadBlockResponses(block.Header.Height) | ||
resp, err := m.store.LoadBlockResponses(appHeight) | ||
if err != nil { | ||
return isRequired, errors.Wrap(err, "load block responses") | ||
return errors.Wrap(err, "load block responses") | ||
} | ||
copy(m.lastState.LastResultsHash[:], tmtypes.NewResults(resp.DeliverTxs).Hash()) | ||
|
||
_, err = m.store.UpdateState(m.lastState, nil) | ||
if err != nil { | ||
return isRequired, errors.Wrap(err, "update state") | ||
return errors.Wrap(err, "update state") | ||
} | ||
m.store.SetHeight(block.Header.Height) | ||
return isRequired, nil | ||
m.store.SetHeight(appHeight) | ||
return nil | ||
} | ||
|
||
func (m *Manager) executeBlock(ctx context.Context, block *types.Block, commit *types.Commit) (*tmstate.ABCIResponses, error) { | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed bug when applying cache, as no error is returned but expected