fault_proving(headers): create new version of the block header, with tx_id_commitment #2648

rymnc · 2025-01-29T15:31:43Z

Linked Issues/PRs

Description

This pull request introduces a new feature called "fault-proving" across various modules and files. It includes modifications to configuration files, schema updates, and changes to the handling of block generation and transaction IDs.

Reviewers guide

ignore all the toml changes, its just a bunch of fault-proving features with deps
Focus on crates/types/src/blockchain for changes to the block header
gql codeowners: Focus on crates/client for breaking change to the graphql api
producer/executor codeowners: The chainId has to be bubbled all the way from the producer until the generation of a block header from a partial block header because the tx_id_commitment needs it

you may filter the non-toml files by filtering them here -

Fault-Proving Feature Integration:

benches/Cargo.toml, crates/chain-config/Cargo.toml, crates/client/Cargo.toml, crates/compression/Cargo.toml, crates/database/Cargo.toml, crates/fuel-core/Cargo.toml: Added fault-proving feature dependencies. [1] [2] [3] [4] [5] [6]

Code Updates for Fault-Proving:

benches/src/db_lookup_times_utils/seed.rs, crates/chain-config/src/config/randomize.rs, crates/fuel-core/src/database/block.rs, crates/fuel-core/src/executor.rs, crates/fuel-core/src/query/message/test.rs: Updated block generation code to conditionally include fault-proving feature parameters. [1] [2] [3] [4] [5]

Schema and Type Changes:

crates/client/assets/schema.sdl, crates/client/src/client/schema/block.rs, crates/client/src/client/types/block.rs, crates/fuel-core/src/schema/block.rs: Added txIdCommitment field and HeaderVersion::V2 to support the fault-proving feature. [1] [2] [3] [4]

Snapshot and Test Updates:

crates/client/src/client/schema/snapshots/fuel_core_client__client__schema__block__tests__block_by_height_query_gql_output.snap, crates/client/src/client/schema/snapshots/fuel_core_client__client__schema__block__tests__block_by_id_query_gql_output.snap, crates/client/src/client/schema/snapshots/fuel_core_client__client__schema__block__tests__blocks_connection_query_gql_output.snap, crates/client/src/client/schema/snapshots/fuel_core_client__client__schema__chain__tests__chain_gql_query_output.snap: Updated snapshots to include txIdCommitment field. [1] [2] [3] [4]

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]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

netrome · 2025-02-04T12:21:37Z

Started looking at this. Just gotta give a kudos for the first impression of the PR. This stuff makes me happy :)

netrome

Great stuff overall. I have some thoughts and questions, but the only blockers I can see is a dangling todo and a comment that should be updated.

Also it would be nice to have a test for the commitment computation.

crates/types/src/blockchain/header.rs

crates/types/Cargo.toml

crates/types/src/blockchain/block.rs

crates/types/src/blockchain/header.rs

crates/types/src/blockchain/header/v2.rs

netrome · 2025-02-04T12:56:15Z

crates/types/src/blockchain/header/v2.rs

+    /// fault proving relevant data
+    pub fault_proving: FaultProvingHeader,


I wonder if we should put this within the ApplicationHeader, but not in the GeneratedApplicationFields. Because the ApplicationHeader says: "/// Contains everything except consensus related data.". Otherwise, we should update the docs for the ApplicationHeader. I'm okay either way, it's not obvious what's right here to me.

it's a design decision :) i find it cleaner to be separate and more evident about what the difference is between this header and the previous one

Fair enough.

Also, I think it should be part of the Application header. Otherwise, you break the idea of AplicationHeader existence.

The application header exists to remove all fields related to the execution or the execution result from the header. All information that can be known only after the block is produced or executed go there.
ConsensusHeaders contains all the information to verify the validity of the block(that it was created with valid height, DA height, time). The application headers contain everything in the case if you want to verify the execution.

crates/client/src/client/schema/block.rs

netrome

Thanks for updating. Looks good to me!

crates/types/src/blockchain/header.rs

MitchTurner · 2025-02-05T17:42:30Z

crates/types/src/blockchain/header.rs

-    }
+/// Helpful methods for all variants of the block header.
+#[enum_dispatch]
+pub(crate) trait GetBlockHeaderFields {


Why not include a:

#[cfg(feature = "fault-proving")] fn fault_proving(&self) -> Option<FaultProvingHeader;

I assume this will still work with the #[enum_dispatch]

yagni, we just want to provide access to the tx_id_commitment and nothing else for now

MitchTurner · 2025-02-05T18:04:05Z

crates/types/src/blockchain/block.rs

+            #[cfg(feature = "fault-proving")]
+            Block::V1(BlockV1 {
+                header:
+                    BlockHeader::V2(crate::blockchain::header::BlockHeaderV2 {


This pattern match is pretty big. Would it be easier to read if you just used the new getter traits here, and didn't bother matching on the the BlockHeader variant?

I might be missing something that needs special attention, but that's kinda the point of my comment :)

oh we have to do this, if we don't the compiler yells at us saying we haven't matched on a variant of the block header ;)

crates/types/src/services/block_producer.rs

…fault proving fix: clippy fix: changelog

Dentosal

LGTM

MitchTurner

Nice

…ring compilation (#2653) ## Linked Issues/PRs  ## Description  found a lot of `SerdeDeCustom` errors while compiling #2648 without the fault-proving feature flag in [the build script](crates/services/upgradable-executor/build.rs) ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) 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] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else?

## Linked Issues/PRs  - #2648 ## 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]](diffhunk://#diff-379b2602e8ed0e8048291c20eff0d6ab59d4cfc1e6c5c9c6d5513b33008ba1c1L366-R367) [[2]](diffhunk://#diff-379b2602e8ed0e8048291c20eff0d6ab59d4cfc1e6c5c9c6d5513b33008ba1c1L2715-R2715) [[3]](diffhunk://#diff-379b2602e8ed0e8048291c20eff0d6ab59d4cfc1e6c5c9c6d5513b33008ba1c1L2754-R2754) [[4]](diffhunk://#diff-379b2602e8ed0e8048291c20eff0d6ab59d4cfc1e6c5c9c6d5513b33008ba1c1L3389-R3389) * 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]](diffhunk://#diff-bffa43e1ce0d3b4c653637fd70da7d6321dd2528d4bbf496dce57ed2488c552fL8-R11) [[2]](diffhunk://#diff-bffa43e1ce0d3b4c653637fd70da7d6321dd2528d4bbf496dce57ed2488c552fL64-R67) [[3]](diffhunk://#diff-bffa43e1ce0d3b4c653637fd70da7d6321dd2528d4bbf496dce57ed2488c552fR76-R83) ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) 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] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else?

rymnc added the no changelog Skip the CI check of the changelog modification label Jan 29, 2025

rymnc force-pushed the fault-proving/full-block-header-changes branch from 2633cdc to cc07609 Compare January 29, 2025 15:40

rymnc self-assigned this Jan 29, 2025

rymnc changed the title ~~fault_proving(headers): create new version of the block header, with fault proving~~ fault_proving(headers): create new version of the block header, with tx_id_commitment Jan 29, 2025

rymnc linked an issue Jan 29, 2025 that may be closed by this pull request

fault_proving(compression): add merkle root of transaction ids to compressed block header #2569

Closed

rymnc force-pushed the fault-proving/full-block-header-changes branch 23 times, most recently from e9374ea to cde0dc9 Compare January 31, 2025 09:37

rymnc removed the no changelog Skip the CI check of the changelog modification label Jan 31, 2025

rymnc mentioned this pull request Jan 31, 2025

chore(wasm-executor): add nicer error if you missed a feature flag during compilation #2653

Merged

9 tasks

rymnc force-pushed the fault-proving/full-block-header-changes branch from cde0dc9 to bb0368e Compare February 3, 2025 06:47

rymnc marked this pull request as ready for review February 3, 2025 06:47

rymnc requested review from xgreenx, Dentosal, MitchTurner, netrome, acerone85, rafal-ch and AurelienFT as code owners February 3, 2025 06:47

netrome requested changes Feb 4, 2025

View reviewed changes

rymnc force-pushed the fault-proving/full-block-header-changes branch from bb0368e to 0306777 Compare February 4, 2025 14:33

netrome previously approved these changes Feb 4, 2025

View reviewed changes

rymnc force-pushed the fault-proving/full-block-header-changes branch from b03734a to 888cd03 Compare February 4, 2025 15:28

MitchTurner reviewed Feb 5, 2025

View reviewed changes

rymnc dismissed netrome’s stale review via b3a933b February 6, 2025 09:56

rymnc force-pushed the fault-proving/full-block-header-changes branch 2 times, most recently from 2fc9470 to c1a4cc0 Compare February 6, 2025 10:02

fault_proving(headers): create new version of the block header, with …

a6e4f34

…fault proving fix: clippy fix: changelog

rymnc force-pushed the fault-proving/full-block-header-changes branch from c1a4cc0 to a6e4f34 Compare February 6, 2025 10:03

rymnc requested review from MitchTurner and netrome February 6, 2025 10:03

Dentosal approved these changes Feb 6, 2025

View reviewed changes

netrome approved these changes Feb 6, 2025

View reviewed changes

MitchTurner approved these changes Feb 6, 2025

View reviewed changes

rymnc merged commit cf23e4b into master Feb 6, 2025
33 checks passed

rymnc deleted the fault-proving/full-block-header-changes branch February 6, 2025 17:26

rymnc mentioned this pull request Feb 7, 2025

fix(headers): move tx_id_commitment to application_headers #2678

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fault_proving(headers): create new version of the block header, with tx_id_commitment #2648

fault_proving(headers): create new version of the block header, with tx_id_commitment #2648

rymnc commented Jan 29, 2025 •

edited

Loading

netrome commented Feb 4, 2025 •

edited

Loading

netrome left a comment •

edited

Loading

netrome Feb 4, 2025

rymnc Feb 4, 2025

netrome Feb 4, 2025

xgreenx Feb 6, 2025

netrome left a comment

MitchTurner Feb 5, 2025

rymnc Feb 6, 2025

MitchTurner Feb 5, 2025 •

edited

Loading

rymnc Feb 6, 2025

Dentosal left a comment

MitchTurner left a comment

		/// fault proving relevant data
		pub fault_proving: FaultProvingHeader,

fault_proving(headers): create new version of the block header, with tx_id_commitment #2648

fault_proving(headers): create new version of the block header, with tx_id_commitment #2648

Conversation

rymnc commented Jan 29, 2025 • edited Loading

Linked Issues/PRs

Description

Reviewers guide

Fault-Proving Feature Integration:

Code Updates for Fault-Proving:

Schema and Type Changes:

Snapshot and Test Updates:

Checklist

Before requesting review

After merging, notify other teams

netrome commented Feb 4, 2025 • edited Loading

netrome left a comment • edited Loading

Choose a reason for hiding this comment

netrome Feb 4, 2025

Choose a reason for hiding this comment

rymnc Feb 4, 2025

Choose a reason for hiding this comment

netrome Feb 4, 2025

Choose a reason for hiding this comment

xgreenx Feb 6, 2025

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

MitchTurner Feb 5, 2025

Choose a reason for hiding this comment

rymnc Feb 6, 2025

Choose a reason for hiding this comment

MitchTurner Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

rymnc Feb 6, 2025

Choose a reason for hiding this comment

Dentosal left a comment

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

rymnc commented Jan 29, 2025 •

edited

Loading

netrome commented Feb 4, 2025 •

edited

Loading

netrome left a comment •

edited

Loading

MitchTurner Feb 5, 2025 •

edited

Loading