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

feat(manager): max skew based on time instead of batches #1140

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

srene
Copy link
Contributor

@srene srene commented Oct 16, 2024

PR Standards

This PR substitutes the max batch skew calculation, and instead of pausing block production after N unsubmitted batches (Config parameter), it pauses block production when the gap between the last submitted block and the last produced bock is higher than a config value.

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A


Close #1139

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@srene srene requested a review from a team as a code owner October 16, 2024 09:58
@srene srene marked this pull request as draft October 16, 2024 09:58
@srene srene linked an issue Oct 16, 2024 that may be closed by this pull request
block/state.go Fixed Show fixed Hide fixed
block/state.go Fixed Show fixed Hide fixed
block/submit.go Fixed Show fixed Hide fixed
block/state.go Fixed Show fixed Hide fixed
@srene srene self-assigned this Oct 17, 2024
@srene srene marked this pull request as ready for review October 17, 2024 11:51
block/executor.go Outdated Show resolved Hide resolved
types/state.go Outdated
Comment on lines 80 to 85
func (s *State) GetSkewTime() time.Duration {
if s.LastBlockTime.Before(s.LastSubmittedBlockTime) {
return 0
}
return s.LastBlockTime.Sub(s.LastSubmittedBlockTime)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's more mathematically correct to just keep the negative here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure im following, why do we want to return a negative gap time?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's more mathematically natural, there is no reason to go to zero, it's an unexpected thing to do

block/state.go Outdated
@@ -112,6 +112,7 @@ func (e *Executor) UpdateStateAfterInitChain(s *types.State, res *abci.ResponseI
}
// We update the last results hash with the empty hash, to conform with RFC-6962.
copy(s.LastResultsHash[:], merkle.HashFromByteSlices(nil))
s.LastSubmittedBlockTime = time.Now()
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I understand this, why it happens after init chain?

Copy link
Contributor Author

@srene srene Oct 18, 2024

Choose a reason for hiding this comment

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

its a way to initialize the LastSubmittedBlockTime to measure the gap between produced and submitted. this way the gap can be still measured when there is no submitted batch yet...

block/manager.go Outdated Show resolved Hide resolved
@srene srene marked this pull request as draft October 17, 2024 13:07
block/state.go Fixed Show fixed Hide fixed
block/submit.go Fixed Show fixed Hide fixed
@@ -110,6 +111,8 @@
}
// We update the last results hash with the empty hash, to conform with RFC-6962.
copy(s.LastResultsHash[:], merkle.HashFromByteSlices(nil))

s.SetLastSubmittedBlockTime(time.Now())

Check warning

Code scanning / CodeQL

Calling the system time Warning

Calling the system time may be a possible source of non-determinism
@srene srene marked this pull request as ready for review October 18, 2024 09:36
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 71.65354% with 36 lines in your changes missing coverage. Please review.

Project coverage is 17.03%. Comparing base (54e2fa5) to head (f8938b3).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
types/pb/dymint/state.pb.go 43.18% 24 Missing and 1 partial ⚠️
settlement/grpc/grpc.go 0.00% 10 Missing ⚠️
settlement/dymension/utils.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1140      +/-   ##
==========================================
- Coverage   19.54%   17.03%   -2.51%     
==========================================
  Files         165      174       +9     
  Lines       41268    51520   +10252     
==========================================
+ Hits         8065     8776     +711     
- Misses      32034    41366    +9332     
- Partials     1169     1378     +209     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace max_batch_skew config param to max_time_skew block production not stopping after max_batch_skew
2 participants