Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Manual Seal: Calculate the block's post hash #10498

Conversation

JoshOrndorff
Copy link
Contributor

This PR fixes manual seal so that the block hash it returns to the RPC caller is the same hash that can later be used to look the block up in the database.

Previously manual seal was returning the hash of the block as was returned from the Producer. That is to say before any signature or other kind of seal was added.

This bug was caught by Moonbeam's CI, which also shows that this changes fixes it.

cc @seunlanlege

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

A test would be nice

@bkchr bkchr added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Dec 15, 2021
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Dec 16, 2021
JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Dec 16, 2021
JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Dec 16, 2021
// Make sure we return the same post-hash that will be calculated when importing the block
// This is important in case the digest_provider added any signature, seal, ect.
let mut post_header = header.clone();
post_header.digest_mut().logs.extend(params.post_digests.iter().cloned());
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if there's any security risk to calculating the hash up front at this point rather than cloning the header and doing the hash later on?

Copy link
Member

Choose a reason for hiding this comment

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

Why should that be a security risk?

@bkchr
Copy link
Member

bkchr commented Jan 8, 2022

@JoshOrndorff what is the status of a test?

@bkchr
Copy link
Member

bkchr commented Jan 10, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

tgmichel pushed a commit to moonbeam-foundation/substrate that referenced this pull request Jan 10, 2022
(cherry picked from commit 3a5aa8c)
@paritytech-processbot paritytech-processbot bot merged commit 15b154a into paritytech:master Jan 10, 2022
@JoshOrndorff
Copy link
Contributor Author

Thank you @tgmichel

tgmichel pushed a commit to moonbeam-foundation/substrate that referenced this pull request Jan 18, 2022
(cherry picked from commit 3a5aa8c)
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* calculate the post hash

* Add test

Co-authored-by: tgmichel <telmo@purestake.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* calculate the post hash

* Add test

Co-authored-by: tgmichel <telmo@purestake.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants