-
Notifications
You must be signed in to change notification settings - Fork 104
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
Conversation
core/blockchain.go
Outdated
TriesInMemory: DefaultTriesInMemory, | ||
TrieRetention: 30 * time.Minute, | ||
MaxNumberOfBlocksToSkipStateSaving: 127, | ||
MaxAmountOfGasToSkipStateSaving: 15 * 1000 * 1000, |
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.
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)
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.
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.
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.
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.
core/blockchain.go
Outdated
@@ -1441,7 +1449,29 @@ func (bc *BlockChain) writeBlockWithState(block *types.Block, receipts []*types. | |||
} | |||
// If we're running an archive node, always flush |
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.
Probably good to update comment to reflect changed code
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.
I've added a comment, I am open for any suggestions for improvement
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.
LGTM
No description provided.