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

Rewind fix #6

Merged
merged 11 commits into from
Jul 16, 2024
Merged

Rewind fix #6

merged 11 commits into from
Jul 16, 2024

Conversation

Brindrajsinh-Chauhan
Copy link

@Brindrajsinh-Chauhan Brindrajsinh-Chauhan commented Apr 15, 2024

This PR adds the ability to force a diskRoot update after a time threshold. This can be enabled by the following in the config file
AllowForceUpdate - To enable/disable Force update of the state
CommitThreshold - Number of state changes before force update

Current Scenario

The snapshot layers maintains a layer of 128 different block roots. Blocks with no transactions have the same root thus do not get updated to the layers. Snapshots beyond that point are flattened into one. Flattened layer is only merged with the snapshot on the disk once the memory of the flattened layers reaches the limit of ~4MB. This could take a lot of time. When a node falls over or has a unclean shutdown and enters a missing head state during start up, it rewinds back to this diskRoot, which could mean a significant roll back based on the activity/size of transactions.

Proposed Scenario

This PR adds a forcing function that is triggered based on commit threshold. After the first 128 layers of snapshot, the flattened layers are merged with the diskRoot when either the memory limit is reached or the commit threshold has been crossed. This allows more frequent update of the diskRoot thus potentially preventing large roll backs

Related eth-boot PR - https://github.com/kaleido-io/photic-eth-boot/pull/112

@Brindrajsinh-Chauhan Brindrajsinh-Chauhan force-pushed the rewind-fix branch 2 times, most recently from 8d46965 to 00ea7a2 Compare April 22, 2024 20:38
Copy link

@jimthematrix jimthematrix left a comment

Choose a reason for hiding this comment

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

approved with minor suggestions

@@ -137,18 +137,22 @@ type CacheConfig struct {
SnapshotLimit int // Memory allowance (MB) to use for caching snapshot entries in memory
Preimages bool // Whether to store preimage of trie key to the disk

SnapshotNoBuild bool // Whether the background generation is allowed
SnapshotWait bool // Wait for snapshot construction on startup. TODO(karalabe): This is a dirty hack for testing, nuke it
AllowForceUpdate bool // Enable to force snapshots based on commit counts

Choose a reason for hiding this comment

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

Suggested change
AllowForceUpdate bool // Enable to force snapshots based on commit counts
AllowForceUpdate bool // Enable to force root snapshots based on the configured commits threshold

SnapshotNoBuild bool // Whether the background generation is allowed
SnapshotWait bool // Wait for snapshot construction on startup. TODO(karalabe): This is a dirty hack for testing, nuke it
AllowForceUpdate bool // Enable to force snapshots based on commit counts
CommitThreshold int // Number of commits to force a root snapshot

Choose a reason for hiding this comment

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

Suggested change
CommitThreshold int // Number of commits to force a root snapshot
CommitThreshold int // Threshold of commits to force a root snapshot update

@@ -1368,6 +1374,7 @@ func (bc *BlockChain) writeBlockWithState(block *types.Block, receipts []*types.

current := block.NumberU64()
// Flush limits are not considered for the first TriesInMemory blocks.
log.Debug("Trie in memory", "current", current, "inMemory", TriesInMemory)

Choose a reason for hiding this comment

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

would this be a large struct to print? if so maybe change to Trace level

@@ -75,6 +75,9 @@ var (
bloomDestructHasherOffset = 0
bloomAccountHasherOffset = 0
bloomStorageHasherOffset = 0

// Count for number of commits before fore disk root update

Choose a reason for hiding this comment

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

Suggested change
// Count for number of commits before fore disk root update
// Count for number of commits before forcing disk root update

@Brindrajsinh-Chauhan Brindrajsinh-Chauhan merged commit 419638f into release/1.11 Jul 16, 2024
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.

2 participants