Skip to content

Proposer/BlockBuilder: Accept proof recorder & extensions#9947

Merged
bkchr merged 22 commits intomasterfrom
bkchr-proposer-extension
Nov 30, 2025
Merged

Proposer/BlockBuilder: Accept proof recorder & extensions#9947
bkchr merged 22 commits intomasterfrom
bkchr-proposer-extension

Conversation

@bkchr
Copy link
Copy Markdown
Member

@bkchr bkchr commented Oct 6, 2025

This pull request fundamentally changes how Proposer and BlockBuilder are handling the proof recorder and extensions. Before this pull request the proof recorder was initialized by the BlockBuilder and the proposer statically enabled proof recording or disabled it. With this pull request the proof recorder is passed from the the caller down to the block builder. This also moves the responsibility for extracting the final storage proof to the caller and is not part of the block builder logic anymore. The extensions are now also configurable by the caller and are not longer "guessed" by the block builder.

This pull request also remvoes the cumulus-client-proposer crate as it is not really required anymore.

This pull request fundamentally changes how `Proposer` and `BlockBuilder` are handling the proof recorder and extensions.
Before this pull request the proof recorder was initialized by the `BlockBuilder` and the proposer statically enabled
proof recording or disabled it. With this pull request the proof recorder is passed from the the caller down to the
block builder. This also moves the responsibility for extracting the final storage proof to the caller and is not
part of the block builder logic anymore. The extensions are now also configurable by the caller and are not longer "guessed" by the block builder.

This pull request also remvoes the `cumulus-client-proposer` crate as it is not really required anymore.
@bkchr bkchr requested review from a team and skunert October 6, 2025 15:20
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Oct 6, 2025
@bkchr
Copy link
Copy Markdown
Member Author

bkchr commented Oct 7, 2025

/cmd fmt


let storage_proof_recorder = params.storage_proof_recorder.unwrap_or_default();

if !params.extra_extensions.is_registered(ProofSizeExt::type_id()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Situation here is not so nice. If this check is triggered it means that the proof recorder provided via the args needs to be registered here also in the ProofSizeExt. Otherwise there might be bugs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But I'm doing this? The recorder passed via the params is registered here.

Copy link
Copy Markdown
Contributor

@michalkucharczyk michalkucharczyk Nov 20, 2025

Choose a reason for hiding this comment

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

I think that if other ProofSizeExt is already registered, it may not use storage_proof_recorder passed in params? Maybe we should overwrite extension here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if I read the code correctly - but I see "implicit" assumption that storage_proof_recorder is exactly the same instance that shall be registered in extensions. From what I see it is not enforced - potentially causing some troubles in future.

The code overall looks fine, so this potential discrepancy is only the minor that could be improved. Maybe we don't need to pass extra_extension, and we could create the one down in the block-builder/src/lib.rs? Not sure if there are other use cases for extra_extensions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added this as some kind of backwards compatibility here, but you are right. I will return an error if the extension exists, but the storage_proof_recorder is None.

@skunert skunert requested a review from a team November 10, 2025 11:06
@skunert
Copy link
Copy Markdown
Contributor

skunert commented Nov 10, 2025

For other reviewers who might wonder what the context of this change is:

Before this PR, the block-builder and basic-authorship where conttrolling the proof recording. Users of these components where just telling it to record a proof, and they would handle it. However, this only works well for single blocks.

Since we will be interested in bundling multiple blocks into a single PoV, the storage proof needs to be recorded "incrementally". With the changes introduced here, higher level components can instantiate the proof recorder and define ignored nodes based on previous blocks for the same PoV. The block-builder itself does not need to know anything about the bundling itself.

bkchr and others added 2 commits November 17, 2025 20:34
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/19506817141
Failed job name: cargo-clippy

],
parachain_inherent_data,
extra_inherent_data: other_inherent_data,
proposal_duration: authoring_duration,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Should we utilize the adjusted_authoring_duration instead here?

Copy link
Copy Markdown
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM!

/// If the proof recording was requested, but `None` is given, this will return
/// `Err(NoProofRecorded)`.
fn into_proof(storage_proof: Option<StorageProof>) -> Result<Self::Proof, NoProofRecorded>;
/// When `Some`, a storage proof will be recorded and included in the proposal.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DQ: How exactly will it be included? Proposal does not have dedicated field for recorder proof.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I will change the wording here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could P be removed?

C::Api: BabeApi<B>,
P: Send + Sync,
{
type Proof = P;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here: P is no longer needed.


let BuiltBlock { block, proof, .. } = builder.build()?;
let BuiltBlock { block, .. } = builder.build()?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change

max_duration: std::time::Duration::from_secs(1),
block_size_limit: None,
storage_proof_recorder: None,
extra_extensions: Default::default(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here

Comment on lines 101 to +106
call_api_at: self.call_api_at,
proof_recorder: None,
inherent_digests: Default::default(),
parent_block: self.parent_block,
parent_number,
extra_extensions: Extensions::new(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: ..Default::default() could be used

inherent_digests: Default::default(),
parent_block: self.parent_block,
parent_number,
extra_extensions: Extensions::new(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and here...

bkchr and others added 3 commits November 24, 2025 13:19
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
@bkchr bkchr enabled auto-merge November 24, 2025 12:36
@bkchr
Copy link
Copy Markdown
Member Author

bkchr commented Nov 29, 2025

/cmd prdoc --audience node_dev --bum major

@bkchr bkchr added this pull request to the merge queue Nov 30, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2025
@bkchr bkchr added this pull request to the merge queue Nov 30, 2025
Merged via the queue into master with commit cb4262e Nov 30, 2025
249 of 254 checks passed
@bkchr bkchr deleted the bkchr-proposer-extension branch November 30, 2025 10:01
bee344 pushed a commit that referenced this pull request Dec 2, 2025
This pull request fundamentally changes how `Proposer` and
`BlockBuilder` are handling the proof recorder and extensions. Before
this pull request the proof recorder was initialized by the
`BlockBuilder` and the proposer statically enabled proof recording or
disabled it. With this pull request the proof recorder is passed from
the the caller down to the block builder. This also moves the
responsibility for extracting the final storage proof to the caller and
is not part of the block builder logic anymore. The extensions are now
also configurable by the caller and are not longer "guessed" by the
block builder.

This pull request also remvoes the `cumulus-client-proposer` crate as it
is not really required anymore.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T0-node This PR/Issue is related to the topic “node”.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants