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.
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
Add chain data pruning experimental feature #4686
Add chain data pruning experimental feature #4686
Changes from 15 commits
fce33c1
be8f081
af59579
16de0e9
3e67042
0d433a3
0e545c5
4e0e627
9313b02
3e3596b
d0807ff
4c1d792
95e86a5
a63542a
580942a
328005b
c34154f
64ba456
a42a933
322b327
cd37f0f
ac79611
51c6c6d
96c2934
cbc460b
99b273f
cc4648e
8315393
5992b66
5d4da4e
118178e
91102a7
962adb6
1368c55
d41661b
2a439f1
d29a286
e2c088c
25d514d
571a022
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can domain object be created instead so that 3 values don't need to passed through
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.
Updated
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.
Is there a way to avoid this magic value? If syncing takes longer than a week then this will fail?
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.
Yes, ideally if the pruner has the syncing context to know the current pivot block and if the syncing has finished, then it can avoid pruning beyond the pivot block before syncing is completed. Do you think we should do it in a separate ticket?
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 think we should do this in another ticket.
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.
Rename to MAX_CHAIN_DATA_PRUNING_MIN_BLOCKS_RETAINED? This not just the default but also the maximum number of blocks that will be pruned.
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.
This is actually the minimum value for the blocks to retain. I've also changed it to be much smaller.
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.
Is there a reason this type doesn't match the PositiveNumber type above?
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.
Converting and returning long makes it easy to compare with block number (also in long) in use of outside. Using Positive number is to ensure user can only supply positive frequency.
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.
Could maybe wrap in a ChainDataPruningConfiguration object?
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.
Done
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.
Would be nicer to drive this based on the chain data pruning enabled flag rather than setting and using a static global field
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.
Yea, I tried that a bit but it seems like that would involve bringing besu command into the protocol context. I consider it to be a temporary solution until #4703 is merged. Then, we could just do a cleanup to get rid of this global field completely.
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.
We shouldn't need to bring the entire Besu command into the protocol context. At most, the chain pruning config values would need to be passed through. A compromise could be to make the ChainPruner a singleton instead.
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.
Actually, It seems cleaner to add a flag in merge context instead, following what @gfukushima did in #4735
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.
updated to have a temporary flag in protocol context instead. Global variable gets removed.
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.
Wondering if this is an unnecessary log or too much for DEBUG level...If you think there is value in this log, I would lean towards putting this at TRACE level and/or only logging went something significant occurs, either a state change where we can print some details about what changed; or a (potentially ignorable) error.
If I'm reading it correctly, BlockAddedEvent will only give details about the block event (which is already logged elsewhere) rather than anything useful about the pruning operation.
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.
Yep, the logging here is unnecessary. Removed.