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: support set hardfork from command line #1422

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

driftluo
Copy link
Contributor

@driftluo driftluo commented Sep 12, 2023

What this PR does / why we need it?

This PR adds features which support setting hardfork from the command line

like this:

./target/debug/axon hardfork \
  -c devtools/chain/config.toml \
  --hardfork-start-number 1 --feature f --feature e

What is the impact of this PR?

No Breaking Change

PR relation:

CI Settings

CI Usage

Tip: Check the CI you want to run below, and then comment /run-ci.

CI Switch

  • E2E Tests
  • Web3 Compatible Tests
  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests

CI Description

CI Name Description
Chaos CI Test the liveness and robustness of Axon under terrible network condition
Cargo Clippy Run cargo clippy --all --all-targets --all-features
Coverage Test Get the unit test coverage report
E2E Test Run end-to-end test to check interfaces
Code Format Run cargo +nightly fmt --all -- --check and cargo sort -gwc
Web3 Compatible Test Test the Web3 compatibility of Axon
v3 Core Test Run the compatibility tests provided by Uniswap V3
OCT 1-5 | 6-10 | 11 | 12-15 | 16-19 Run the compatibility tests provided by OpenZeppelin

@driftluo driftluo requested a review from a team as a code owner September 12, 2023 09:17
@driftluo driftluo force-pushed the support-set-hardfork-from-command-line branch from 07d6f62 to c97f6b0 Compare September 12, 2023 09:22
@driftluo driftluo requested review from KaoImin and yangby-cryptape and removed request for jjyr and wenyuanhust September 12, 2023 09:22
Flouse

This comment was marked as duplicate.

KaoImin
KaoImin previously approved these changes Sep 13, 2023
Copy link
Collaborator

@yangby-cryptape yangby-cryptape left a comment

Choose a reason for hiding this comment

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

TL;DR: IMHO, the set operation should be a one-time operation. (click to see details)

Maybe there should be a new subcommand for this feature.

Looks at the following example:

  • Current tip number is 1000.
  • The user uses axon run --block-number 3000 --hardforks feature to start the chain.
  • When the chain reach 2000, the machine has to be restart, so axon is stopped.
  • Then, if the user want to restart Axon, he have to use axon run --block-number 3000 --hardforks feature to start the chain again.
  • After number 3000, the user only have to use axon run to start the chain.

So, the user have to know the current tip number of the node data (not the tip number of the chain, just the tip number of the node data) before execute axon run.
It's not reasonable.

I have a proposal (click to see details).
  • The hardfork features can be set without start the chain.

    axon hardfork set --number H --feature F

    if the setting is valid, then save it into persistent caches.
    When the chain is started, read them from the caches.

  • We can list the hardfork features which are not reached yet.

    axon hardfork list

    Read data from the persistent caches.

    Maybe we can list all enabled hardfork features, too, if it possible.

    p.s. It could be implemented later, not right now.

  • The hardfork features can be removed before the number reached.

    axon hardfork remove --number H: remove all hardfork features at H if exists.
    axon hardfork remove --feature F: remove hardfork feature F if exists.

    p.s. It could be implemented later, not right now.

@Flouse @KaoImin How about your opinions?

core/run/src/lib.rs Outdated Show resolved Hide resolved
@driftluo driftluo force-pushed the support-set-hardfork-from-command-line branch from c97f6b0 to 034bbeb Compare September 13, 2023 02:51
@KaoImin
Copy link
Contributor

KaoImin commented Sep 13, 2023

TL;DR: IMHO, the set operation should be a one-time operation. (click to see details)
Maybe there should be a new subcommand for this feature.

Looks at the following example:

  • Current tip number is 1000.
  • The user uses axon run --block-number 3000 --hardforks feature to start the chain.
  • When the chain reach 2000, the machine has to be restart, so axon is stopped.
  • Then, if the user want to restart Axon, he have to use axon run --block-number 3000 --hardforks feature to start the chain again.
  • After number 3000, the user only have to use axon run to start the chain.

So, the user have to know the current tip number of the node data (not the tip number of the chain, just the tip number of the node data) before execute axon run. It's not reasonable.

I have a proposal (click to see details).
@Flouse @KaoImin How about your opinions?

Noop, the command is an one time operation with the following steps:

  1. Stop the chain and download the new binary/image
  2. Determine the hardfork start number which must be larger than current tip
  3. Start Axon with --block_number n --hardforks feat_1

Once the hardfork proposal has become consensus, no additional arguments need to be added for the following re-start.

@Flouse
Copy link
Contributor

Flouse commented Sep 13, 2023

So, the user have to know the current tip number of the node data (not the tip number of the chain, just the tip number of the node data) before execute axon run. It's not reasonable.

I think it's easy for the chain maintainer to know the current tip number of the node.
As a node maintainer, it's a basic requirement, especially when he/she wants to setup the next hardfork.


And I agree with @yangby-cryptape that the --hardfork_start_number param should be verified first.

@driftluo driftluo force-pushed the support-set-hardfork-from-command-line branch from 034bbeb to 32a329c Compare September 13, 2023 03:00
@Flouse

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@driftluo driftluo force-pushed the support-set-hardfork-from-command-line branch from 32a329c to 131563a Compare September 13, 2023 03:06
@yangby-cryptape
Copy link
Collaborator

Once the hardfork proposal has become consensus, no additional arguments need to be added for the following re-start.

Can you guarantee that the hardfork proposal could become to a consensus feature at once?

Once the chain is stopped, maybe an unintended stop, the users have to decide which command should be used when restart the chain.

I think it's easy for the chain maintainer to know the current tip number of the node.

Consider that, is any system administrator will restart services manually?

If you write a service to restart axon automatically after it is stopped, you have to check the tip number of the chain data.

  • First, you have to modify the service script every time, when a new hardfork feature is on your queue.

  • And, how to do fetch a tip number of chain data? Because there is no command to fetch a tip number from chain data without run it at present.

  • The only way is to read the tip number from the log files. It doesn't look reliable.

@driftluo driftluo force-pushed the support-set-hardfork-from-command-line branch 2 times, most recently from e2cfbb8 to b56544d Compare September 13, 2023 03:53
@KaoImin
Copy link
Contributor

KaoImin commented Sep 13, 2023

Some addition points should be added:

  1. The number of blocks that each hardfork meet consensus should be recorded.
  2. The return of axon_getHardforkInfo should be changed as following:
#[derive(Serialize, Deserialize)]
pub struct HardforksStatus {
    pub enabled:    Vec<HardforkInfo>,
    pub determined: Vec<HardforkInfo>,
    pub proposing:  Vec<HardforkInfo>,
}

@yangby-cryptape
Copy link
Collaborator

Some addition points should be added:

... ...

I prefer to HashMap<HardforkInfo, Status> with

pub enum HardforkStatus {
    Proposed,
    Enabled,
    /.. other statuses ../
}

@Flouse Flouse requested a review from sunchengzhu September 14, 2023 02:25
@driftluo driftluo force-pushed the support-set-hardfork-from-command-line branch from b56544d to f698114 Compare September 15, 2023 06:56
KaoImin
KaoImin previously approved these changes Sep 19, 2023
KaoImin
KaoImin previously approved these changes Sep 19, 2023
@KaoImin
Copy link
Contributor

KaoImin commented Sep 19, 2023

/run-ci

@github-actions
Copy link

CI tests run on commit:

CI test list:

  • E2E Tests
  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests
  • Web3 Compatible Tests

Please check ci test results later.

@KaoImin KaoImin enabled auto-merge September 19, 2023 03:21
Flouse
Flouse previously approved these changes Sep 19, 2023
@KaoImin KaoImin added this pull request to the merge queue Sep 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2023
@yangby-cryptape
Copy link
Collaborator

I had some review suggestions, but not only about this PR. also for the previous PR of hardfork.

I will create an issue later to discuss them, later.

@KaoImin KaoImin added this pull request to the merge queue Sep 19, 2023
@KaoImin KaoImin removed this pull request from the merge queue due to a manual request Sep 19, 2023
Flouse and others added 2 commits September 19, 2023 12:45
* ci: add health-check shell script
* ci: remove blank line

---------
Co-authored-by: Flouse <[email protected]>
@driftluo driftluo dismissed stale reviews from Flouse, yangby-cryptape, and KaoImin via 04e3d05 September 19, 2023 04:45
@KaoImin KaoImin enabled auto-merge September 19, 2023 05:24
@KaoImin KaoImin added this pull request to the merge queue Sep 19, 2023
Merged via the queue into main with commit 7ae0a82 Sep 19, 2023
20 checks passed
@driftluo driftluo deleted the support-set-hardfork-from-command-line branch September 19, 2023 06:18
@Flouse Flouse added t:feature and removed feature labels Nov 17, 2023
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.

5 participants