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

Include the seal when populating the header for a new block #11475

Merged
merged 12 commits into from
Feb 14, 2020
Merged

Include the seal when populating the header for a new block #11475

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
12abda9
debug signer
dvdplm Feb 6, 2020
f1acd9a
Merge branch 'master' into dp/fix/maximumEmptySteps-11445
dvdplm Feb 6, 2020
fc04c69
Don't panic if empty_steps_transition already happened
dvdplm Feb 7, 2020
722ca99
Cleanup some code in Aura
dvdplm Feb 7, 2020
b1be876
Include the seal when populating the header for a new block (fixes #1…
dvdplm Feb 10, 2020
917c094
Merge branch 'master' into dp/fix/maximumEmptySteps-11445
dvdplm Feb 10, 2020
3103b2f
cleanup
dvdplm Feb 11, 2020
47ee9b1
cleanup2
dvdplm Feb 11, 2020
a2c704a
Review grumbles
dvdplm Feb 13, 2020
221afa7
Update ethcore/engines/authority-round/src/lib.rs
dvdplm Feb 14, 2020
1298901
Update ethcore/engines/authority-round/src/lib.rs
dvdplm Feb 14, 2020
2c984e7
Update ethcore/src/block.rs
dvdplm Feb 14, 2020
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
34 changes: 22 additions & 12 deletions ethcore/engines/authority-round/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,17 +720,22 @@ fn header_signature(header: &Header, empty_steps_transition: u64) -> Result<Sign
.as_val::<H520>().map(Into::into)
}

// extracts the raw empty steps vec from the header seal. should only be called when there are 3 fields in the seal
// (i.e. header.number() >= self.empty_steps_transition)
fn header_empty_steps_raw(header: &Header) -> &[u8] {
header.seal().get(2).expect("was checked with verify_block_basic; has 3 fields; qed")
// Extracts the empty steps from the header seal. Returns data only when there are 3 fields in the seal
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
// (i.e. header.number() >= self.empty_steps_transition).
fn header_empty_steps_raw(header: &Header) -> Option<&[u8]> {
header.seal().get(2).map(Vec::as_slice )
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
}

// extracts the empty steps from the header seal. should only be called when there are 3 fields in the seal
// Extracts the empty steps from the header seal. Returns data only when there are 3 fields in the seal
// (i.e. header.number() >= self.empty_steps_transition).
fn header_empty_steps(header: &Header) -> Result<Vec<EmptyStep>, ::rlp::DecoderError> {
let empty_steps = Rlp::new(header_empty_steps_raw(header)).as_list::<SealedEmptyStep>()?;
Ok(empty_steps.into_iter().map(|s| EmptyStep::from_sealed(s, header.parent_hash())).collect())
header_empty_steps_raw(header).map_or(Ok(vec![]), |raw| {
let empty_steps = Rlp::new(raw).as_list::<SealedEmptyStep>()?;
Ok(empty_steps
.into_iter()
.map(|s| EmptyStep::from_sealed(s, header.parent_hash()))
.collect())
})
}

// gets the signers of empty step messages for the given header, does not include repeated signers
Expand Down Expand Up @@ -788,7 +793,7 @@ fn verify_external(header: &Header, validators: &dyn ValidatorSet, empty_steps_t
let correct_proposer = validators.get(header.parent_hash(), header_step as usize);
let is_invalid_proposer = *header.author() != correct_proposer || {
let empty_steps_rlp = if header.number() >= empty_steps_transition {
Some(header_empty_steps_raw(header))
header_empty_steps_raw(header)
} else {
None
};
Expand Down Expand Up @@ -970,11 +975,15 @@ impl AuthorityRound {
/// Drops all `EmptySteps` less than or equal to the passed `step`, irregardless of the parent hash.
fn clear_empty_steps(&self, step: u64) {
let mut empty_steps = self.empty_steps.lock();
*empty_steps = empty_steps.split_off(&EmptyStep {
if empty_steps.is_empty() {
return
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
}
let next_empty_steps = empty_steps.split_off(&EmptyStep {
step: step + 1,
parent_hash: Default::default(),
signature: Default::default(),
});
*empty_steps = next_empty_steps
}

fn store_empty_step(&self, empty_step: EmptyStep) {
Expand Down Expand Up @@ -3013,9 +3022,10 @@ mod tests {
let params = AuthorityRoundParams::from(deserialized.params);
for ((block_num1, address1), (block_num2, address2)) in
params.block_reward_contract_transitions.iter().zip(
[(0u64, BlockRewardContract::new_from_address(Address::from_str("2000000000000000000000000000000000000002").unwrap())),
(7u64, BlockRewardContract::new_from_address(Address::from_str("3000000000000000000000000000000000000003").unwrap())),
(42u64, BlockRewardContract::new_from_address(Address::from_str("4000000000000000000000000000000000000004").unwrap())),
[
(0u64, BlockRewardContract::new_from_address(Address::from_str("2000000000000000000000000000000000000002").unwrap())),
(7u64, BlockRewardContract::new_from_address(Address::from_str("3000000000000000000000000000000000000003").unwrap())),
(42u64, BlockRewardContract::new_from_address(Address::from_str("4000000000000000000000000000000000000004").unwrap())),
].iter())
{
assert_eq!(block_num1, block_num2);
Expand Down
3 changes: 2 additions & 1 deletion ethcore/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ impl<'x> OpenBlock<'x> {
self.block.header.set_timestamp(header.timestamp());
self.block.header.set_uncles_hash(*header.uncles_hash());
self.block.header.set_transactions_root(*header.transactions_root());
self.block.header.set_seal(header.seal().to_vec());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fix for #11445 is here: without this the LockedBlock built by enact() lacks the seal and this causes the state root to be wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right for me though. The idea with block types is that you go from:
OpenBlock -> ClosedBlock -> LockedBlock -> SealedBlock
and only the last type is guaranteed to have the seal.

How come the seal is included in the state root anyway? It's two separate things and really I don't see a way how seal affects the state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added comment + ref to this PR. It still feels a bit dirty to do this here and I'm open to alternatives.

Copy link
Collaborator

@niklasad1 niklasad1 Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

question: is this not called from Miner::seal_and_import_block_internally code path? It seems like Miner::prepare_block doesn't call populate_from, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean, but the reverse call-graph for populate_from() looks like this:

populate_from() is called by…
	enact() is called by…
		enact_verified() is called by…
			check_and_lock_block() is called by…
				import_verified_blocks() is called by… (in impl Importer)
					import_verified_blocks() is called by… (in impl ImportBlock for Client)
						message() is called by… (ClientIoMessage::BlockVerified)
						flush_queue()

In other words, populate_from() is only called on the block verification path, not for block producing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, yeah that's what I meant.

// TODO: that's horrible. set only for backwards compatibility
if header.extra_data().len() > self.engine.maximum_extra_data_size() {
warn!("Couldn't set extradata. Ignoring.");
Expand Down Expand Up @@ -554,7 +555,7 @@ mod tests {
b.close_and_lock()
}

/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header. Seal the block aferwards
/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header. Seal the block afterwards
fn enact_and_seal(
block_bytes: Vec<u8>,
engine: &dyn Engine,
Expand Down
2 changes: 1 addition & 1 deletion parity/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ impl GasPricerConfig {
}
}

#[derive(Debug, PartialEq)]
#[derive(Debug, PartialEq, Clone)]
pub struct MinerExtras {
pub author: Address,
pub engine_signer: Address,
Expand Down
2 changes: 2 additions & 0 deletions parity/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ fn execute_impl<Cr, Rr>(
let fetch = fetch::Client::new(FETCH_FULL_NUM_DNS_THREADS).map_err(|e| format!("Error starting fetch client: {:?}", e))?;

let txpool_size = cmd.miner_options.pool_limits.max_count;

// create miner
let miner = Arc::new(Miner::new(
cmd.miner_options,
Expand All @@ -502,6 +503,7 @@ fn execute_impl<Cr, Rr>(
account_utils::miner_local_accounts(account_provider.clone()),
)
));

miner.set_author(miner::Author::External(cmd.miner_extras.author));
miner.set_gas_range_target(cmd.miner_extras.gas_range_target);
miner.set_extra_data(cmd.miner_extras.extra_data);
Expand Down