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(hardfork): built-in miner shouldn't ignore the extension field in block templates #2876

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

yangby-cryptape
Copy link
Collaborator

Issue

The built-in miner will ignore the extension field in block templates.

Reason

When call as_builder() to convert a Block to a BlockBuilder, all extra fields will be ignored, because the generated code of BlockBuilder couldn't know any about extra fields.

And the extension field is an extra field on the molecule-serialized struct Block, it will be ignored, too.

Solution

We already have the advanced Block builder, which is not generated.

The as_advanced_builder will handle the extension field.

How to reproduce the issue and test the solution?

  • Set the extension in the TxPoolService::build_block_template(..) to the follow value:

    let extension = if candidate_number % 2 == 0 {
        let bytes: Bytes = format!("block number = {}", candidate_number).as_bytes().pack();
        Some(bytes.into())
    } else {
        None
    };
  • Start a dev chain with func = "Eaglesong" and run a built-in miner.

  • Check the extension field in all blocks.

@yangby-cryptape yangby-cryptape marked this pull request as ready for review July 28, 2021 02:37
@yangby-cryptape yangby-cryptape requested a review from a team as a code owner July 28, 2021 02:37
@quake
Copy link
Member

quake commented Jul 29, 2021

bors r=quake,zhangsoledad

@bors
Copy link
Contributor

bors bot commented Jul 29, 2021

Build succeeded:

@bors bors bot merged commit bb43d5f into nervosnetwork:develop Jul 29, 2021
@yangby-cryptape yangby-cryptape deleted the pr/fix-miner branch July 29, 2021 05:04
@doitian doitian mentioned this pull request Aug 10, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants