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.

Sign up for GitHub

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

Jump to bottom

BREAK CHANGE! Consensus: epoch_duration_target now affects epoch length in Dummy mode #4097

Merged
merged 8 commits into from
Aug 17, 2023
Merged

BREAK CHANGE! Consensus: epoch_duration_target now affects epoch length in Dummy mode #4097

merged 8 commits into from
Aug 17, 2023

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Aug 11, 2023 •
edited
Loading

What problem does this PR solve?

Related issue: #4092

Problem Summary:

What's Changed:

Related changes

  • let epoch_duration_target affect epoch length when permanent_difficulty_in_dummy is enabled in Dummy mode.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note 

Note: This PR introduce a breaking change for `Dummy` mode. 
When the CKB is in `Dummy` mode and the `permanent_difficulty_in_dummy` option is set to `true`,
the epoch length will be influenced by the `epoch_duration_target` parameter.
Specifically, the epoch length will be calculated as `epoch_duration_target / 8`. 

For example, if `epoch_duration_target` is configured as `14400`,
the resulting epoch length will be calculated as: `14400` / `8` = `1800`.

@eval-exec eval-exec force-pushed the exec/try-epoch_duration_target branch from 332690f to 27af3cd Compare August 11, 2023 08:45
@eval-exec eval-exec force-pushed the exec/try-epoch_duration_target branch from 27af3cd to 84d4bf9 Compare August 11, 2023 09:08
@phroi phroi mentioned this pull request Aug 11, 2023
Closed
quake
quake reviewed Aug 11, 2023
View reviewed changes
spec/src/consensus.rs Outdated Show resolved Hide resolved
@quake quake mentioned this pull request Aug 11, 2023
Closed
@eval-exec eval-exec force-pushed the exec/try-epoch_duration_target branch from 27e69ae to 84d4bf9 Compare August 11, 2023 09:31
@eval-exec eval-exec requested a review from zhangsoledad August 11, 2023 09:52
@eval-exec eval-exec changed the title Consensus: let epoch_duration_target affect epoch length Consensus: Allow epoch_duration_target to impact epoch length. Aug 11, 2023
@eval-exec eval-exec changed the title Consensus: Allow epoch_duration_target to impact epoch length. Consensus: Allow epoch_duration_target to impact epoch length in Dummy mode. Aug 11, 2023
@eval-exec eval-exec force-pushed the exec/try-epoch_duration_target branch from 69a7766 to 219eb75 Compare August 11, 2023 10:02
@eval-exec eval-exec marked this pull request as ready for review August 11, 2023 10:02
@eval-exec eval-exec requested a review from a team as a code owner August 11, 2023 10:02
@eval-exec
Copy link
Collaborator Author

eval-exec commented Aug 11, 2023 •
edited
Loading

make cli-test has been successfully executed.

I invite @zhangsoledad to review this PR and provide feedback.

@eval-exec eval-exec added the b:consensus Break consensus label Aug 11, 2023
phroi added a commit to dckb-rescuer/v1-interface that referenced this pull request Aug 11, 2023
@phroi
Switch to nervosnetwork/ckb#4097 for custom epoch length
70abc0b
@quake quake added this pull request to the merge queue Aug 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023
@eval-exec
Copy link
Collaborator Author

eval-exec commented Aug 14, 2023 •
edited
Loading

ckb/test/src/specs/dao/satoshi_dao_occupied.rs

Line 145 in 1ca9a32

spec.params.epoch_duration_target = Some(2);

I think previous usage of epoch_duration_target is wrong,
epoch_duration_target isn't how many blocks in a epoch, it's a duration. I'm going to modify Some(2) to Some(2 * 8). fixing...

@eval-exec
Copy link
Collaborator Author

eval-exec commented Aug 14, 2023 •
edited
Loading

Last 2 commits fixed CI job issue.

@eval-exec eval-exec requested a review from quake August 14, 2023 08:56
@quake quake added this pull request to the merge queue Aug 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 15, 2023
@eval-exec
Copy link
Collaborator Author

It's strange, all integration tests passed on my machine, but they failed on the GitHub merge queue. Investigating why...

https://github.com/nervosnetwork/ckb/actions/runs/5862389610/job/15894109895

@eval-exec eval-exec changed the title Consensus: Allow epoch_duration_target to impact epoch length in Dummy mode. BREAK CHANGE! Consensus: Allow epoch_duration_target to impact epoch length in Dummy mode. Aug 15, 2023
@eval-exec eval-exec marked this pull request as draft August 15, 2023 05:59
@eval-exec eval-exec marked this pull request as ready for review August 15, 2023 06:25
@eval-exec eval-exec requested a review from quake August 17, 2023 01:46
@quake quake added this pull request to the merge queue Aug 17, 2023
Merged via the queue into nervosnetwork:develop with commit 65c03b2 Aug 17, 2023
@eval-exec eval-exec changed the title BREAK CHANGE! Consensus: Allow epoch_duration_target to impact epoch length in Dummy mode. BREAK CHANGE! Consensus: epoch_duration_target now affects epoch length in Dummy mode Aug 17, 2023
@eval-exec eval-exec mentioned this pull request Aug 17, 2023
Merged
zhangsoledad added a commit that referenced this pull request Aug 17, 2023
@zhangsoledad
Merge pull request #4105 from eval-exec/exec/try-epoch_duration_targe…
a2abcc8 
…t.backport

Backport #4097 to `rc/v0.111.x`
@doitian doitian mentioned this pull request Sep 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenyukang chenyukang chenyukang left review comments

@EthanYuan EthanYuan EthanYuan approved these changes

@quake quake quake approved these changes

@zhangsoledad zhangsoledad Awaiting requested review from zhangsoledad

@doitian doitian Awaiting requested review from doitian

@driftluo driftluo Awaiting requested review from driftluo

Assignees
No one assigned
Labels
b:consensus Break consensus
Projects
CKB Kanban
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eval-exec @quake @chenyukang @EthanYuan