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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 80 additions & 1 deletion client/consensus/manual-seal/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ mod tests {
use sc_consensus::ImportedAux;
use sc_transaction_pool::{BasicPool, Options, RevalidationType};
use sc_transaction_pool_api::{MaintainedTransactionPool, TransactionPool, TransactionSource};
use sp_runtime::generic::BlockId;
use sp_inherents::InherentData;
use sp_runtime::generic::{BlockId, Digest, DigestItem};
use substrate_test_runtime_client::{
AccountKeyring::*, DefaultTestClientBuilderExt, TestClientBuilder, TestClientBuilderExt,
};
Expand All @@ -265,6 +266,35 @@ mod tests {

const SOURCE: TransactionSource = TransactionSource::External;

struct TestDigestProvider<C> {
_client: Arc<C>,
}
impl<B, C> ConsensusDataProvider<B> for TestDigestProvider<C>
where
B: BlockT,
C: ProvideRuntimeApi<B> + Send + Sync,
{
type Transaction = TransactionFor<C, B>;

fn create_digest(
&self,
_parent: &B::Header,
_inherents: &InherentData,
) -> Result<Digest, Error> {
Ok(Digest { logs: vec![] })
}

fn append_block_import(
&self,
_parent: &B::Header,
params: &mut BlockImportParams<B, Self::Transaction>,
_inherents: &InherentData,
) -> Result<(), Error> {
params.post_digests.push(DigestItem::Other(vec![1]));
Ok(())
}
}

#[tokio::test]
async fn instant_seal() {
let builder = TestClientBuilder::new();
Expand Down Expand Up @@ -519,4 +549,53 @@ mod tests {
// assert that fork block is in the db
assert!(client.header(&BlockId::Hash(imported.hash)).unwrap().is_some())
}

#[tokio::test]
async fn manual_seal_post_hash() {
let builder = TestClientBuilder::new();
let (client, select_chain) = builder.build_with_longest_chain();
let client = Arc::new(client);
let spawner = sp_core::testing::TaskExecutor::new();
let pool = Arc::new(BasicPool::with_revalidation_type(
Options::default(),
true.into(),
api(),
None,
RevalidationType::Full,
spawner.clone(),
0,
));
let env = ProposerFactory::new(spawner.clone(), client.clone(), pool.clone(), None, None);

let (mut sink, commands_stream) = futures::channel::mpsc::channel(1024);
let future = run_manual_seal(ManualSealParams {
block_import: client.clone(),
env,
client: client.clone(),
pool: pool.clone(),
commands_stream,
select_chain,
// use a provider that pushes some post digest data
consensus_data_provider: Some(Box::new(TestDigestProvider { _client: client.clone() })),
create_inherent_data_providers: |_, _| async { Ok(()) },
});
std::thread::spawn(|| {
let rt = tokio::runtime::Runtime::new().unwrap();
rt.block_on(future);
});
let (tx, rx) = futures::channel::oneshot::channel();
sink.send(EngineCommand::SealNewBlock {
parent_hash: None,
sender: Some(tx),
create_empty: true,
finalize: false,
})
.await
.unwrap();
let created_block = rx.await.unwrap().unwrap();

// assert that the background task returned the actual header hash
let header = client.header(&BlockId::Number(1)).unwrap().unwrap();
assert_eq!(header.hash(), created_block.hash);
}
}
7 changes: 6 additions & 1 deletion client/consensus/manual-seal/src/seal_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,14 @@ pub async fn seal_block<B, BI, SC, C, E, TP, CIDP>(
digest_provider.append_block_import(&parent, &mut params, &inherent_data)?;
}

// 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?


match block_import.import_block(params, HashMap::new()).await? {
ImportResult::Imported(aux) =>
Ok(CreatedBlock { hash: <B as BlockT>::Header::hash(&header), aux }),
Ok(CreatedBlock { hash: <B as BlockT>::Header::hash(&post_header), aux }),
other => Err(other.into()),
}
};
Expand Down