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

dvdplm · 2020-02-10T19:59:14Z

Include the seal when populating the header for a new block so that the state root is calculated properly.

The issue here is that when emptyStepTransition and maximumEmptySteps are both set in the chain spec there are verification errors on block import, at Stage 5.
A block is issued by a node and the other nodes can't verify it: state root mismatch.
Now, the Seal fields contain EmptySteps after the transition and each empty step implies a reward for the validator that emitted the empty step. This means that the accounts involved get touched, so the state root changes. Before that last step of the verification we call check_and_lock_block() in client.rs which calls enact_verified() which calls enact().
enact() essentially copies over data from the incoming block header (the one with the Seal with the EmptySteps) but before this PR it does not copy over the seal from the block being imported to the newly prepared LockedBlock.
The rewards are bestowed in on_close_block() and then the state is committed (which sets the state root).
I think this is what is going on. I didn't use LockedBlock::seal() because it returns a SealedBlock and I wasn't sure if we should change the return type tbh.

Fixes #11445

* master: [eth classic chainspec]: remove `balance = 1` (#11459) just to make sure we don't screw up this again (#11455) backwards compatible call_type creation_method (#11450)

Before this the `header_empty_steps_raw` would panic if the chain has already progressed beyond the block number set in `emptyStepsTransition`. As this is a user accessible configuration option I don't think we should panic.

Nothing really interesting here, renames or removes some methods. Adds some docs and extends a test a bit to clarify the behaviour of the code.

…1445)

* master: Use parity-crypto updated to use upstream rust-secp256k1 (#11406) Cleanup some code in Aura (#11466) upgrade some of the dependencies (#11467) Some more release track changes to README.md (#11465) Update simple one-line installer due to switching to a single stable release track (#11463) update Dockerfile (#11461) Implement EIP-2124 (#11456)

dvdplm · 2020-02-11T08:27:01Z

ethcore/src/block.rs

@@ -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());


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.

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?

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

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?

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.

cool, yeah that's what I meant.

tomusdrw

I need some clarifications on the original issue, but for me it seems unlikely that it actually affect state root at all. However if we can prove that it actually solves the issue, then let's merge it but let's also add docs what the hell happens here.

ethcore/engines/authority-round/src/lib.rs

tomusdrw · 2020-02-13T12:18:36Z

ethcore/src/block.rs

@@ -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());


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?

tomusdrw · 2020-02-13T12:21:50Z

Oh, apologies. I just read the description and now I understand how seal can affect state via block rewards. I think it's a good idea to add this as a documentation for seal copying line in populate_from

niklasad1

looks good to me, but wouldn't hurt to get another pair of eyes on this one because I'm not super intimate with this.

andresilva

changes lgtm. like @tomusdrw it also wasn't obvious to me the effects of the seal in state root calculation when we discussed this some days ago.
what I don't know is if this issue was always there or if it surfaced due to some recent change. I believe it must be the later because I tested the empty steps feature when I implemented it, and as it stands (before this PR) it is impossible to use it.

ethcore/engines/authority-round/src/lib.rs

ethcore/src/block.rs

Co-Authored-By: André Silva <[email protected]>

dvdplm · 2020-02-14T15:37:07Z

what I don't know is if this issue was always there or if it surfaced due to some recent change. I believe it must be the later because I tested the empty steps feature when I implemented it, and as it stands (before this PR) it is impossible to use it.

Yeah, that's my biggest concern too: it must have been broken for quite while. I guess it's possible that users run old versions and haven't run into this yet.

niklasad1 · 2020-02-14T16:10:30Z

Yeah, that's my biggest concern too: it must have been broken for quite while. I guess it's possible that users run old versions and haven't run into this yet.

I still suspect this Clique Engine PR but I might be wrong :)

* master: (27 commits) Faster kill_garbage (#11514) [EngineSigner]: don't sign message with only zeroes (#11524) fix compilation warnings (#11522) [ethcore cleanup]: various unrelated fixes from `#11493` (#11507) Add benchmark for transaction execution (#11509) Add Smart Contract License v1.0 Misc fixes (#11510) [dependencies]: unify `rustc-hex` (#11506) Activate on-chain randomness in POA Sokol (#11505) Grab bag of cleanup (#11504) Implement eth/64 (EIP-2364) and drop support for eth/62 (#11472) [dependencies]: remove `util/macros` (#11501) OpenEthereum bootnodes are added (#11499) [ci benches]: use `all-features` (#11496) [verification]: make test-build compile standalone (#11495) complete null-signatures removal (#11491) Include the seal when populating the header for a new block (#11475) fix compilation warnings (#11492) cargo update -p cmake (#11490) update to published rlp-derive (#11489) ...

dvdplm added 5 commits February 6, 2020 13:24

debug signer

12abda9

Merge branch 'master' into dp/fix/maximumEmptySteps-11445

f1acd9a

* master: [eth classic chainspec]: remove `balance = 1` (#11459) just to make sure we don't screw up this again (#11455) backwards compatible call_type creation_method (#11450)

Don't panic if empty_steps_transition already happened

fc04c69

Before this the `header_empty_steps_raw` would panic if the chain has already progressed beyond the block number set in `emptyStepsTransition`. As this is a user accessible configuration option I don't think we should panic.

Cleanup some code in Aura

722ca99

Nothing really interesting here, renames or removes some methods. Adds some docs and extends a test a bit to clarify the behaviour of the code.

Include the seal when populating the header for a new block (fixes #1…

b1be876

…1445)

dvdplm changed the title ~~Thank you for your Pull Request!~~ Include the seal when populating the header for a new block Feb 10, 2020

dvdplm self-assigned this Feb 10, 2020

dvdplm added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 11, 2020

cleanup

3103b2f

dvdplm commented Feb 11, 2020

View reviewed changes

cleanup2

47ee9b1

dvdplm requested a review from tomusdrw February 11, 2020 16:13

tomusdrw reviewed Feb 13, 2020

View reviewed changes

Review grumbles

a2c704a

tomusdrw approved these changes Feb 14, 2020

View reviewed changes

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 14, 2020

niklasad1 approved these changes Feb 14, 2020

View reviewed changes

dvdplm requested a review from andresilva February 14, 2020 12:48

andresilva approved these changes Feb 14, 2020

View reviewed changes

ethcore/engines/authority-round/src/lib.rs Outdated Show resolved Hide resolved

ethcore/engines/authority-round/src/lib.rs Outdated Show resolved Hide resolved

ethcore/src/block.rs Outdated Show resolved Hide resolved

dvdplm and others added 3 commits February 14, 2020 16:30

Update ethcore/engines/authority-round/src/lib.rs

221afa7

Co-Authored-By: André Silva <[email protected]>

Update ethcore/engines/authority-round/src/lib.rs

1298901

Co-Authored-By: André Silva <[email protected]>

Update ethcore/src/block.rs

2c984e7

Co-Authored-By: André Silva <[email protected]>

niklasad1 merged commit 856a075 into master Feb 14, 2020

niklasad1 deleted the dp/fix/maximumEmptySteps-11445 branch February 14, 2020 18:01

dvdplm added the B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. label Feb 14, 2020

niklasad1 mentioned this pull request Feb 17, 2020

Don't panic if empty_steps_transition already happened #11464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dvdplm commented Feb 10, 2020 •

edited

Loading

dvdplm Feb 11, 2020

tomusdrw Feb 13, 2020

dvdplm Feb 13, 2020

niklasad1 Feb 14, 2020 •

edited

Loading

dvdplm Feb 14, 2020

niklasad1 Feb 14, 2020

tomusdrw left a comment

tomusdrw Feb 13, 2020

tomusdrw commented Feb 13, 2020

niklasad1 left a comment •

edited

Loading

andresilva left a comment

dvdplm commented Feb 14, 2020

niklasad1 commented Feb 14, 2020 •

edited

Loading

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

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

Conversation

dvdplm commented Feb 10, 2020 • edited Loading

dvdplm Feb 11, 2020

Choose a reason for hiding this comment

tomusdrw Feb 13, 2020

Choose a reason for hiding this comment

dvdplm Feb 13, 2020

Choose a reason for hiding this comment

niklasad1 Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

dvdplm Feb 14, 2020

Choose a reason for hiding this comment

niklasad1 Feb 14, 2020

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Feb 13, 2020

Choose a reason for hiding this comment

tomusdrw commented Feb 13, 2020

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

dvdplm commented Feb 14, 2020

niklasad1 commented Feb 14, 2020 • edited Loading

dvdplm commented Feb 10, 2020 •

edited

Loading

niklasad1 Feb 14, 2020 •

edited

Loading

niklasad1 left a comment •

edited

Loading

niklasad1 commented Feb 14, 2020 •

edited

Loading