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

feat: optimize Godwoken finality mechanism #836

Merged
merged 16 commits into from
Nov 30, 2022
Merged

feat: optimize Godwoken finality mechanism #836

merged 16 commits into from
Nov 30, 2022

Conversation

keroro520
Copy link
Contributor

@keroro520 keroro520 commented Nov 1, 2022 •
edited by Flouse
Loading

@keroro520 keroro520 self-assigned this Nov 1, 2022
@keroro520 keroro520 changed the title feat(types): extend RollupConfig finality utils feat: optimize Godwoken finality mechanism Nov 6, 2022
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from d0d2336 to cba5aef Compare November 6, 2022 15:43
@keroro520 keroro520 changed the base branch from breaking-change-scripts-next-upgrade to develop November 6, 2022 15:47
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from e7948f8 to 643273b Compare November 6, 2022 16:24
@Flouse Flouse requested review from zeroqn and jjyr November 7, 2022 02:41
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from 643273b to 4abcbe3 Compare November 7, 2022 05:03
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 7, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 7, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 7, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 7, 2022
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch 2 times, most recently from 6d16ca2 to 3e96828 Compare November 7, 2022 15:51
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 8, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 8, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 8, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 10, 2022
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from 2034831 to db0dabe Compare November 14, 2022 05:21
@keroro520 keroro520 marked this pull request as draft November 14, 2022 05:32
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch 3 times, most recently from 1fd5c07 to 52ab220 Compare November 16, 2022 05:53
@gw-bot
Copy link

gw-bot bot commented Nov 16, 2022 •
edited
Loading

Running integration test

Workflow Run Id: 3477986299

Components:

Manually running integration test

Post a comment contains

/itest
[prebuilds: tag]
[godwoken: branch/ref]
[scripts: branch/ref]
[polyjuice: branch/ref]
[web3: branch/ref]
[kicker: branch/ref]
[tests: branch/ref]

Note: [] means optional, for example

/itest
prebuilds: dev-202203280240
godwoken: develop
scripts: 81676d9d53ffdf5bbaa60483928d07da16eb4a88
polyjuice: e37553b9

Run Result

success

@keroro520 keroro520 marked this pull request as ready for review November 21, 2022 06:15
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from 4db4042 to 3ed5cc7 Compare November 21, 2022 06:15
@gw-bot
Copy link

gw-bot bot commented Nov 21, 2022 •
edited
Loading

Running integration test

Workflow Run Id: 3511948660

Components:

Manually running integration test

Post a comment contains

/itest
[prebuilds: tag]
[godwoken: branch/ref]
[scripts: branch/ref]
[polyjuice: branch/ref]
[web3: branch/ref]
[kicker: branch/ref]
[tests: branch/ref]

Note: [] means optional, for example

/itest
prebuilds: dev-202203280240
godwoken: develop
scripts: 81676d9d53ffdf5bbaa60483928d07da16eb4a88
polyjuice: e37553b9

Run Result

success

@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 21, 2022
@godwokenrises godwokenrises deleted a comment from gw-bot bot Nov 21, 2022
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch 3 times, most recently from fae95b6 to 264fe46 Compare November 29, 2022 05:12
@keroro520 keroro520 requested review from jjyr, blckngm and zeroqn November 29, 2022 05:33
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from 264fe46 to 9487d16 Compare November 29, 2022 07:17
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from 9487d16 to ba19213 Compare November 29, 2022 07:43
@Flouse Flouse mentioned this pull request Nov 29, 2022
Open
@keroro520 keroro520 force-pushed the new-finality-rule-based-on-timestamp branch from d269603 to 1ffaf94 Compare November 30, 2022 04:05
blckngm
blckngm reviewed Nov 30, 2022
View reviewed changes
crates/utils/src/calc_finalizing_range.rs
// NOTE: To ensure that at least one finalized block is found below, start a binary search at
// `upgrade_global_state_version_to_v2 - 1`.
l = l.saturating_sub(1);
let mut r = block.raw().number().unpack().saturating_sub(1);
Copy link
Contributor

@blckngm blckngm Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here's my analysis/proof:

Loop invaraint: l is finalized. r + 1 is not finalized.

l < mid <= r.

So l = mid and r = mid - 1 shrinks the range and maintains the loop invariant.

When the loop ends, l is the last finalized block.

Copy link
Contributor

Choose a reason for hiding this comment

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

l = upgrade_global_state_version_to_v2 - 1 conveniently satisfies that “l is finalized”. If we start with l = upgrade_global_state_version_to_v2 it's not necessarily true.

@jjyr jjyr merged commit 8ff7593 into godwokenrises:develop Nov 30, 2022
@jjyr jjyr mentioned this pull request Nov 30, 2022
Merged
@jjyr jjyr mentioned this pull request Dec 9, 2022
Merged
@Flouse Flouse mentioned this pull request Dec 20, 2022
Closed
9 tasks
@Flouse Flouse mentioned this pull request Dec 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blckngm blckngm blckngm approved these changes

@jjyr jjyr jjyr approved these changes

@zeroqn zeroqn Awaiting requested review from zeroqn

Assignees

@keroro520 keroro520

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keroro520 @jjyr @blckngm @zeroqn