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

[CHIA-786] simplify hard-fork consensus rules #18208

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jun 18, 2024

Purpose:

Now that the hard fork has activated, some of the new rules can be applied unconditionally, even on older blocks.
Doing so simplifies the code and tests.

Specifically, these flags (affecting the behavior of chia_rs's run_block_generator()) are moved to be set unconditionally, where they were previously only set for block heights >= HARD_FORK_HEIGHT.

ENABLE_SOFTFORK_CONDITION

Enable 3 features:
* the SOFTFORK condition code, which mimics the softfork operator. You specify its cost as the first argument. This allows soft-forking in new condition codes.
* Unknown condition codes with cost. Certain condition codes are reserved to have a pre-defined cost, but are no-ops. This enables soft-forking in meaning to thes condition codes.
* Enables the new AGG_SIG_* conditions (e.g. AGG_SIG_PUZZLE, AGG_SIG_AMOUNT, etc.)

ENABLE_BLS_OPS_OUTSIDE_GUARD

Enables the BLS operators directly in chialisp programs. Previously they were only enabled under the softfork guard

ENABLE_FIXED_DIV

Allows operator / to have negative operands.

AGG_SIG_ARGS

Harmonizes the argument parsing of the AGG_SIG_ME and AGG_SIG_UNSAFE conditions. Previously they required exactly 2 parameters and the terminator did not have to be NIL. With this flag, they behave like all other conditions, additional parameters are ignored in consensus mode and disallowed in mempool mode.

ALLOW_BACKREFS

Allows the block generator blob to be serialized using back-refs.

The main change of this PR is in chia/full_node/mempool_check_conditions.py. The other changes are to update tests that previously ensured these feature would not activate until the hard-fork height.

Setting these flags unconditionally is safe based on the assumption that no block prior to the hard fork activation issued any of these conditions or had programs include any of these operators. It's theoretically possible for a block to issue conditions or execute operators unknown at the time, and consensus rules effectively ignore them.

The validity of this PR hinges on all blocks prior to the hard fork activation remain valid.

Current Behavior:

The consensus rules are slightly different for blocks whose height is < HARD_FORK_HEIGHT.

New Behavior:

The consensus rules are uniform across all blocks (except for other soft-forks still in flight).

Testing Notes:

https://grafana.fmt.chiatechlab.com/d/oBgwkDI7z/blockchain-sync-tests?orgId=1&var-ref=hard-fork-has-activated&var-repo=All&from=now-7d&to=now&var-network=mainnet&var-network=testnet11&var-network=testneta&refresh=1h

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jun 18, 2024
@arvidn arvidn marked this pull request as ready for review June 20, 2024 07:59
@arvidn arvidn requested a review from a team as a code owner June 20, 2024 07:59
@arvidn arvidn changed the title simplify hard-fork consensus rules [CHIA-786] simplify hard-fork consensus rules Jun 20, 2024
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jun 20, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jun 20, 2024
…updated consensus rules to apply unconditionally to block height
Copy link

coveralls-official bot commented Jun 22, 2024

Pull Request Test Coverage Report for Build 9610120052

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 18 of 18 (100.0%) changed or added relevant lines in 5 files are covered.
  • 90 unchanged lines in 26 files lost coverage.
  • Overall coverage decreased (-0.08%) to 90.85%

Files with Coverage Reduction New Missed Lines %
chia/data_layer/data_store.py 1 94.37%
chia/data_layer/download_data.py 1 83.58%
chia/daemon/keychain_proxy.py 1 72.57%
chia/_tests/util/test_priority_mutex.py 1 99.66%
chia/_tests/util/test_action_scope.py 1 99.01%
chia/farmer/farmer.py 1 72.23%
chia/timelord/timelord_launcher.py 1 69.34%
chia/consensus/block_body_validation.py 1 95.79%
chia/_tests/util/full_sync.py 1 88.95%
chia/rpc/wallet_rpc_api.py 1 89.33%
Totals Coverage Status
Change from base Build 9614112249: -0.08%
Covered Lines: 100157
Relevant Lines: 110182

💛 - Coveralls

@arvidn arvidn requested a review from AmineKhaldi June 22, 2024 15:16
@arvidn arvidn closed this Jun 24, 2024
@arvidn arvidn reopened this Jun 24, 2024
Copy link
Contributor

@AmineKhaldi AmineKhaldi left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Was this tested with a full sync from scratch? I'm assuming that's the link in test notes but that is not loading for me, so I thought to confirm.

@arvidn
Copy link
Contributor Author

arvidn commented Jul 3, 2024

yes, it has successfully synced from scratch

@arvidn arvidn requested a review from emlowe July 3, 2024 15:52
@Starttoaster Starttoaster merged commit 260c7a3 into main Jul 5, 2024
731 of 732 checks passed
@Starttoaster Starttoaster deleted the hard-fork-has-activated branch July 5, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants