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

Don't save every state on archive node #210

Merged
merged 11 commits into from
Sep 27, 2023
Merged

Conversation

magicxyyz
Copy link
Contributor

No description provided.

@magicxyyz magicxyyz changed the base branch from master to recreate-state-for-rpcs March 29, 2023 14:42
@magicxyyz magicxyyz marked this pull request as ready for review April 11, 2023 18:07
@PlasmaPower PlasmaPower requested a review from tsahee June 9, 2023 06:42
core/blockchain.go Outdated Show resolved Hide resolved
core/blockchain.go Outdated Show resolved Hide resolved
TriesInMemory: DefaultTriesInMemory,
TrieRetention: 30 * time.Minute,
MaxNumberOfBlocksToSkipStateSaving: 127,
MaxAmountOfGasToSkipStateSaving: 15 * 1000 * 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults in geth should keep geth (ethereum) behavior as much as possible.
These two should definitely be 0.
Defaults in nitro can be debated (I tend for 0 there too but that's different)

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the nitro side, I think trying to match the amount of gas inbetween saved states as L1 (15 million gas) would be good. Saving state for every Arbitrum block isn't as reasonable as it is for L1, but saving it every 15 million gas would mean that the rate of archive disk growth in terms of chain speed should be the same.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then again, this would be something that'd be very easy to miss in an upgrade and would mean that the node is now somewhat permanently missing state. So maybe we should keep it at 0 for the upgrade, I'm not sure. We can talk about it next standup.

@cla-bot cla-bot bot added the s label Jun 15, 2023
tsahee
tsahee previously approved these changes Jun 15, 2023
Base automatically changed from recreate-state-for-rpcs to master August 15, 2023 17:53
@PlasmaPower PlasmaPower dismissed tsahee’s stale review August 15, 2023 17:53

The base branch was changed.

@@ -1441,7 +1449,29 @@ func (bc *BlockChain) writeBlockWithState(block *types.Block, receipts []*types.
}
// If we're running an archive node, always flush
Copy link
Member

Choose a reason for hiding this comment

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

Probably good to update comment to reflect changed code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment, I am open for any suggestions for improvement

Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@joshuacolvin0 joshuacolvin0 merged commit 9ad66d6 into master Sep 27, 2023
3 checks passed
@joshuacolvin0 joshuacolvin0 deleted the dont-save-every-state branch September 27, 2023 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants