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

[util] implement block time calculator #3951

Merged
merged 3 commits into from
Nov 3, 2023
Merged

[util] implement block time calculator #3951

merged 3 commits into from
Nov 3, 2023

Conversation

envestcc
Copy link
Member

@envestcc envestcc commented Oct 26, 2023

Description

There are several reasons for introcuding block time calculator:

  • After the Shanghai upgrade of Ethereum, hard-fork judgment is now based on timestamps. We need to convert specified block heights into timestamps.
  • When calculating the votes for NFT Bucket, we need to convert block numbers into staking times.
  • We need a unified calculation method that takes into account the variation of BlockInterval.

It calculates the corresponding time for a given height. It supports accurate querying of historical blocks as well as prediction for future blocks. The prediction for future blocks is calculated based on the latest height and block interval.

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Code refactor or improvement
  • [] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
  • [] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [] make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] Any dependent changes have been merged and published in downstream modules

@envestcc envestcc changed the title [util] implement block time calculator [shanghai] implement block time calculator Oct 27, 2023
@envestcc envestcc changed the title [shanghai] implement block time calculator [shanghai]-1 implement block time calculator Oct 27, 2023
return &BlockTimeCalculator{
getBlockInterval: getBlockInterval,
getTipHeight: getTipHeight,
getHistoryBlockTime: getHistoryBlockTime,
Copy link
Member

@dustinxie dustinxie Oct 31, 2023

Choose a reason for hiding this comment

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

check nil for 3 input funcs

}

getBlockIntervalFn func(uint64) time.Duration
getTipHeightFn func() uint64
Copy link
Member

@dustinxie dustinxie Oct 31, 2023

Choose a reason for hiding this comment

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

nit: below get block time func may return error, should define getTipHeightFn (uint64, error) as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the method TipHeight() uint64 of the blockchain does not return an error. We can keep it this way.


getBlockIntervalFn func(uint64) time.Duration
getTipHeightFn func() uint64
getHistoryblockTimeFn func(uint64) (time.Time, error)
Copy link
Member

Choose a reason for hiding this comment

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

i think call it getBlockTimeFn is good and clear enough?

if err != nil {
return time.Time{}, err
}
return tipBlockTime.Add(time.Duration(height-tipHeight) * btc.getBlockInterval(height)), nil
Copy link
Member

@dustinxie dustinxie Oct 31, 2023

Choose a reason for hiding this comment

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

  1. height is in the future, so should use btc.getBlockInterval(tipHeight)
  2. should check possible overflow of the result, time.Duration is int64

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

Copy link
Member Author

@envestcc envestcc Nov 1, 2023

Choose a reason for hiding this comment

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

For 2, If it overflows, it means it will be about 300 years later, so there is no need to consider

@envestcc envestcc marked this pull request as ready for review November 1, 2023 02:08
@envestcc envestcc marked this pull request as draft November 1, 2023 04:24
@envestcc envestcc changed the base branch from shanghai to master November 1, 2023 12:49
@envestcc envestcc changed the title [shanghai]-1 implement block time calculator [util] implement block time calculator Nov 1, 2023
@envestcc envestcc marked this pull request as ready for review November 1, 2023 13:28
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #3951 (0a113cf) into master (e1f0636) will increase coverage by 0.80%.
Report is 95 commits behind head on master.
The diff coverage is 77.33%.

❗ Current head 0a113cf differs from pull request most recent head b547d27. Consider uploading reports for the commit b547d27 to get more accurate results

@@            Coverage Diff             @@
##           master    #3951      +/-   ##
==========================================
+ Coverage   75.38%   76.18%   +0.80%     
==========================================
  Files         303      329      +26     
  Lines       25923    28005    +2082     
==========================================
+ Hits        19541    21335    +1794     
- Misses       5360     5572     +212     
- Partials     1022     1098      +76     
Files Coverage Δ
action/candidate_register.go 90.00% <100.00%> (ø)
action/candidate_update.go 89.01% <100.00%> (+0.12%) ⬆️
action/claimreward.go 90.62% <100.00%> (-0.42%) ⬇️
action/depositreward.go 87.50% <100.00%> (-0.56%) ⬇️
action/protocol/context.go 67.91% <100.00%> (+0.73%) ⬆️
action/protocol/execution/evm/evmstatedbadapter.go 66.77% <ø> (ø)
action/protocol/execution/protocol.go 42.10% <100.00%> (ø)
action/protocol/rewarding/reward.go 90.07% <100.00%> (+0.60%) ⬆️
...tion/protocol/staking/candidate_buckets_indexer.go 89.31% <100.00%> (-0.47%) ⬇️
...col/staking/ethabi/stake_composite_bucketscount.go 100.00% <100.00%> (ø)
... and 68 more

... and 3 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link

sonarcloud bot commented Nov 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@envestcc envestcc merged commit 9715764 into master Nov 3, 2023
4 checks passed
@envestcc envestcc deleted the block2time branch November 3, 2023 08:19
@envestcc envestcc linked an issue Nov 17, 2023 that may be closed by this pull request
6 tasks
@envestcc envestcc mentioned this pull request Nov 24, 2023
6 tasks
@envestcc envestcc removed a link to an issue Nov 24, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants