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

fix(headers): move tx_id_commitment to application_headers #2678

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

rymnc
Copy link
Member

@rymnc rymnc commented Feb 7, 2025

Linked Issues/PRs

Description

We get rid of enum_dispatch because if you try to derive the impl on a trait with a generic, you also have to apply the generic to the enum ~ which we don't need. rather than having 2 traits, one generic and one not, opted to cut enum_dispatch altogether.

idk why we were directly accessing the block header fields in the whole codebase without using accessor methods, even when we had defined an enum for block header. this PR fixes that.

if fault-proving is enabled, we now generate BlockV1 with BlockHeaderV2. tx_id_commitment has been moved to GeneratedApplicationFieldsV2, renamed GeneratedApplicationFields to GeneratedApplicationFieldsV1. maybe we can remove pub from a lot of fields and convert to pub(crate) but i can do that in a follow up PR.

I still favour having a separate FaultProvingHeader field on the header, since we anyway have to define a new hash function for the ApplicationHeader<GeneratedApplicationFieldsV2>, it was less verbose before.

Reviewers guide

  • all of the changes in crates other than types is just using getters/setters where necessary
  • in types/header, refactored, removed enum_dispatch due to generic usage that didn't apply on parent enum
  • moved tx_id_commitment from fault_proving_header to application_header. New struct GeneratedApplicationFieldsV2 to accommodate it.

AI generated desc

This pull request includes several changes to improve the consistency and readability of the code by replacing direct field accesses with method calls for accessing block header fields. The most important changes include updates to the BlockHeader field access, test adjustments, and feature additions in the Cargo.toml file.

Block Header Field Access Updates:

  • Updated impl LastBlockConfig in crates/chain-config/src/config/state.rs to use method calls for accessing block header fields instead of direct field accesses.
  • Modified impl From<&BlockHeader> for CompressedBlockHeader in crates/compression/src/compressed_block_payload/v1.rs to use method calls for accessing block header fields.
  • Updated impl Header in crates/fuel-core/src/schema/block.rs to use method calls for accessing block header fields.

Test Adjustments:

  • Adjusted various test cases in crates/fuel-core/src/executor.rs to use method calls for accessing block header fields. [1] [2] [3] [4]
  • Updated test cases in crates/fuel-core/src/service/adapters/consensus_parameters_provider.rs to use method calls for setting consensus parameters version.

Feature Additions:

  • Added new feature fault-proving in Cargo.toml for crates/services/consensus_module.
  • Updated imports and function implementations in crates/services/consensus_module/poa/src/verifier.rs to support the new feature and improve code structure. [1] [2] [3]

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@rymnc rymnc requested a review from xgreenx as a code owner February 7, 2025 17:21
@rymnc rymnc added the no changelog Skip the CI check of the changelog modification label Feb 7, 2025
@rymnc rymnc marked this pull request as draft February 7, 2025 17:21
@rymnc rymnc force-pushed the fix/fault-proving-header-fixes branch 4 times, most recently from c2174b2 to 12903c5 Compare February 7, 2025 17:33
@rymnc rymnc force-pushed the fix/fault-proving-header-fixes branch from 12903c5 to f9fc060 Compare February 7, 2025 18:06
@rymnc rymnc marked this pull request as ready for review February 7, 2025 19:44
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

One nit, and one question. Only blocker from my side is that we should update the changelog:

  1. The previous entry from fault_proving(headers): create new version of the block header, with tx_id_commitment #2648 should be marked as breaking iirc.
  2. We should either modify or write a new entry for this PR if we change visibility of public fields in the block header.

Other than that, this looks good to me.

crates/types/src/blockchain/header/v2.rs Outdated Show resolved Hide resolved
crates/types/src/blockchain/header/v1.rs Show resolved Hide resolved
@netrome netrome removed the no changelog Skip the CI check of the changelog modification label Feb 7, 2025
@rymnc rymnc self-assigned this Feb 7, 2025
@rymnc rymnc added the breaking A breaking api change label Feb 7, 2025
@rymnc rymnc requested a review from netrome February 7, 2025 22:09
netrome
netrome previously approved these changes Feb 8, 2025
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Looks good now!

@@ -51,12 +45,11 @@ pub enum BlockHeader {
}

/// Helpful methods for all variants of the block header.
#[enum_dispatch]
pub(crate) trait GetBlockHeaderFields {
pub trait GetBlockHeaderFields<GAF> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see what is the use case of having a trait for getters(and setters in the case of the tests).

Because it is generic, it means that it will be implemented only for HeaderV1 and HeaderV2 only. It can't be implemented for the BlockHeader enum, so it can't be used with it. Then, why do we need it? Looks like just public getters from impl section should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

leftover code from enum_dispatch land, will fix

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in 28a816a

netrome
netrome previously approved these changes Feb 10, 2025
@rymnc rymnc enabled auto-merge (squash) February 11, 2025 08:42
@rymnc rymnc merged commit 44bd923 into master Feb 11, 2025
33 checks passed
@rymnc rymnc deleted the fix/fault-proving-header-fixes branch February 11, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants