-
Notifications
You must be signed in to change notification settings - Fork 1
NonStop from Pascal to Lorentz HF #57
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
Conversation
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
| } | ||
|
|
||
| nextForkBoundaryHeightMinus1 := uint64(boundaryHeight.Height) - 1 | ||
| log.GetLogger().Info("ForkSpec height required", "ts", x.Timestamp, "height", boundaryHeight, "nextForkBoundaryHeightMinus1", nextForkBoundaryHeightMinus1) |
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.
The height corresponding to the HF timestamp can be immediately checked in this log.
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.
Pull Request Overview
This PR introduces support for eliminating the need to reset ELC when transitioning from Pascal to Lorentz by refactoring the epoch calculation logic to leverage fork specifications instead of a fixed block count per epoch. Key changes include:
- Removing direct reliance on constant.BlocksPerEpoch and replacing it with fork spec–derived boundary and epoch calculations.
- Updating multiple modules, tests, and protobuf definitions to support new fork parameters and boundary height logic.
- Adjusting the integration workflow sleep interval to match the new expected block counts.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tool/testdata/internal/misbehavior.go | Updated logging from a fixed epoch calculation to using fork spec boundary logic. |
| tool/testdata/internal/header.go | Removed dependency on constant; simplified header printing logic. |
| tool/testdata/internal/create_client.go | Refactored client creation to use new consensus state fields for epoch hash logging. |
| tests/prover_network_test.go | Modified tests to use new fork spec calculations with a dynamic “skip” constant. |
| module/vote.go | Revised vote attestation logic by determining the “epoch” condition via validator extraction. |
| module/util.go | Removed legacy epoch conversion functions that are now superseded by fork spec logic. |
| module/setup.go | Refactored header update logic to compute submitting heights using fork parameters. |
| module/prover.go | Updated header preparation to fetch fork parameters dynamically from configuration. |
| module/proof.go | Modified withValidators to pass fork specs and correctly compute boundary values. |
| module/parlia.pb.go | Regenerated protobuf definitions with added EpochLength and MaxTurnLength fields. |
| module/fork_spec_test.go | Added a comprehensive test suite for fork spec selection and boundary epoch calculations. |
| module/fork_spec.go | Introduced new fork spec handling including boundary height and epoch segmentation logic. |
| module/facade.go | Removed legacy epoch functions no longer needed after fork spec integration. |
| module/constant/production.go | Eliminated the BlocksPerEpoch constant as it is no longer used. |
| README.md | Updated supported version info and added guidelines regarding ForkSpec configuration. |
| .github/workflows/ci.yml | Adjusted wait time for integration tests based on new block intervals. |
Comments suppressed due to low confidence (2)
module/vote.go:42
- [nitpick] Using extractValidatorSetAndTurnLength solely to set an 'isEpoch' flag may hide unexpected errors. Consider explicitly handling the error to ensure that a failure in validator extraction is not silently treated as a non-epoch scenario.
if _, _, err := extractValidatorSetAndTurnLength(header); err != nil {
module/setup.go:149
- [nitpick] The logic in makeSubmittingHeights is quite complex; adding inline comments to describe the conditions for appending nextForkBoundaryHeightMinus1 and the iteration over epoch candidates would improve maintainability.
func makeSubmittingHeights(latestFinalizedHeight uint64, savedLatestHeight uint64, firstUnsaved uint64, nextForkBoundaryTs *uint64, nextForkBoundaryHeightMinus1 uint64) []uint64 {
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
| boundaryHeight = v | ||
| } else { | ||
| logger.Debug("seek fork height", "currentHeight", currentHeight, "ts", ts) | ||
| for i := int64(currentHeight); i >= 0; i-- { |
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.
Optional optimization: Consider estimating heights near hardfork points using ForkSpec's block generation time to reduce header query overhead. This isn't a required fix but could improve performance for post-hardfork requests.
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.
It will be supported after the next version release. There is a grace period until the next Maxwell HF.
Signed-off-by: Naohiro Yoshida <naohiro.yoshida@datachain.jp>
Support for eliminating the need to reset ELC when switching from Pascal to Lorentz.
The stacking of blocks from trusted to latest is assumed to be 100 blocks, not epoch length.
This is because the least common multiple of the epoch lengths of pascal and lorentz and maxwell is 100.
It is better to take action such that the epoch length of the final ForkSpec is used eventually.