Make InstantSeal Instant again by seunlanlege · Pull Request #11186 · openethereum/parity-ethereum

seunlanlege · 2019-10-21T19:23:33Z

this call to update_sealing calls requires_reseal

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L1266-L1276

and requires_reseal does a few checks and updates SealingWork.next_allowed_reseal

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L637-L663

for InstantSeal sealing_state() is always SealingState::Ready

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/engines/instant-seal/src/lib.rs#L75

so should_disable_sealing would be false for InstantSeal and so next_allowed_reseal is updated based on reseal_min_period

Now update_sealing calls Miner.seal_and_import_block_internally which in turn calls ImportSealedBlock::import_sealed_block which in turn calls Miner.chain_new_blocks.

Miner.chain_new_blocks resets next_allowed_reseal to Instant::now() if a block was successfully imported.

https://github.com/paritytech/parity-ethereum/blob/6993ec95312f3ee446345ecb2591f0c66a9b4fcb/ethcore/src/miner/miner.rs#L1415-L1420

Previously update_sealing was called immediately after an internal import with the hopes of sealing a new block with local transactions (#9660) but it did so without the knowledge of the transaction_queue. And so if there were no transactions to seal, next_allowed_reseal is updated, but chain_new_blocks is never called to reset it back to Instant::now()

And now the next time a transaction is sent, it won't trigger a block sealing because reseal_allowed() returns false

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L1009-L1011

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L233-L238

This PR adds a new method to the Engine trait should_reseal_on_update and checks if there are local pending transactions before calling update_sealing after a block import.

should_reseal_on_update tells the miner that the engine is interested in sealing a new block if there are local pending transactions.

closes #11173

tomusdrw

I'm a bit unsure about this. I feel that this options should be respected by InstantSeal engine as they are respected by every other engine.

It's trivial to set both of those options in CLI to 0 if someone really wants the blocks to be created as fast as possible. In any other case the block should be created at most every reseal_min_period ms if there are transactions in the pool. And I would consider InstantSeal to produce empty blocks every reseal_max_period (if configured).

If this is a fix to some issues we have with InstantSeal, I don't think it's the right direction. It would be better to re-architecture the way engines and miners interact and new block production is triggered (perhaps using Futures, like in substrate) to clean this up.

seunlanlege · 2019-10-21T19:48:24Z

It would be better to re-architecture the way engines and miners interact and new block production is triggered (perhaps using Futures, like in substrate) to clean this up.

I completely agree with this.

dvdplm · 2019-10-21T21:18:58Z

Previously the InstantSeal engine would only seal a block once and then never until reseal_allowed()

So as per the issue reported in #11173 (repeatable) something changed in v2.5.8+ – have you found out what the change was that "broke" this? I know you had a hypothesis that this code was involved but I do not understand the connection between that and the code snippets in the description above. Can you elaborate on that?

Is it like this: the fix in #10995 is good but uncovered this quirk where our default config values do not work with InstantSeal? Or…?

Either way, we need a fix here of some sort, code or docs, so we don't have to spend days digging through code to help users. Do we need better defaults in the instant_seal.json? Is that where per-engine config should happen? Currently InstantSeal has no parameters at all but maybe it should?

seunlanlege · 2019-10-22T07:41:45Z

this call to update_sealing calls requires_reseal

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L1266-L1276

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L637-L663

for InstantSeal sealing_state() is always SealingState::Ready

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/engines/instant-seal/src/lib.rs#L75

so should_disable_sealing would be false for InstantSeal and so next_allowed_reseal is updated based on reseal_min_period

And now the next time a transaction is sent, it won't trigger a block sealing because reseal_allowed() returns false

https://github.com/paritytech/parity-ethereum/blob/6960d35abb9f11c27b47226e402ec617bfe585f1/ethcore/src/miner/miner.rs#L1009-L1011

Hopefully this makes sense.

seunlanlege · 2019-10-22T07:50:58Z

Currently InstantSeal has no parameters at all but maybe it should?

I worry about having to specify params to an engine to do what its name says it does.

Do we need better defaults in the instant_seal.json? Is that where per-engine config should happen?

With the way the code is structured, the Miner "speaks for" the Engine as to when sealing should occur, so the only way to achieve this is to let the engine itself decide for itself if sealing should occur or not.

tomusdrw · 2019-10-22T16:18:31Z

Thanks for the explanations @seunlanlege. Your hypothesis makes a lot of sense and I see the purpose of this PR better now.

However I'd be more inclined to just introduce a thread to InstantSeal that would periodically (every reseal_min_period?) trigger sealing, just as we have for Clique.

Why I don't like this approach I feel it's cleaner and safer than overriding reseal_min_period for this specific engine. Long-term it would be best to re-work the miner-engine interaction to actually have engine controlling the sealing times as it seems to be becoming a common pattern.

dvdplm · 2019-10-22T20:17:05Z

I worry about having to specify params to an engine to do what its name says it does.

@seunlanlege Good point. (And thank you for the great description, it sort of makes a little bit more sense now, maybe, much appreciated.)

@tomusdrw Regarding the "stepper thread" approach: what if reseal_min_period is 0, or very short? Or are you saying we should enforce a minimum resealing period?

tomusdrw · 2019-10-22T20:25:57Z

Regarding the "stepper thread" approach: what if reseal_min_period is 0, or very short? Or are you saying we should enforce a minimum resealing period?

No, not at all. InstantSeal is creating blocks as soon as there are any transactions (i.e. it doesn't create empty blocks). So having reseal_min_period set to 0 just means that blocks are created as soon as transactions are created. If we want to change InstantSeal to produce empty blocks as well, I'd say it would be sensible to rather produce a new block every reseal_max_period.

dvdplm · 2019-10-22T20:38:26Z

Hmm. I am still confused, apologies. In Clique the stepper thread ends up calling update_sealing() every 2 seconds (SEALING_FREQ) – if we do the same for InstantSeal wouldn't it end up sealing blocks at most every 2 seconds?
The purpose of reseal_min_period is to accumulate more txs I guess?

tomusdrw · 2019-10-23T06:46:01Z

@dvdplm Yes, but block sealing is also triggered in other situation (like transaction import). If we call update_sealing every 2 seconds the blocks will be actually be created at least every 2 seconds, cause otherwise incoming transactions should trigger sealing as well (but at most reseal_min_period).

Actually reseal_min_period is more related to PoW, where finding a block seal take siginifcant amount of time. From one perspective we want to include as much transactions in a block as possible, but at the same time re-setting the mining equpiment has some constant cost, so we don't want to send new header hash too often to avoid just idling. Hence reseal_min_period says: "Create new pending block, but not if we just created one recently."

…lock, some line formatting

seunlanlege · 2019-10-29T10:49:30Z

@tomusdrw could you take a look at this?

tomusdrw

I don't feel that's the way to go. We need something engine-specific not modify miner to force wasteful block production on every engine.

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

tomusdrw

Looks good, but we can get away without this additional check to the transactions pool.

seunlanlege · 2019-11-10T09:39:29Z

needs one more approval cc @niklasad1 @ordian

ordian

Looks good, just one question.

ordian · 2019-11-10T10:26:13Z

 				if self.seal_and_import_block_internally(chain, block) {
 					trace!(target: "miner", "update_sealing: imported internally sealed block");
 				}
+				return


this return doesn't change anything, right?

yeah it doesn't

ordian · 2019-11-10T10:33:50Z

 						service_transaction_checker.as_ref(),
 					);
 					queue.cull(client);
-					if is_internal_import {


was this previously true only for instant seal engine? otherwise, should it be something like

if engine.should_reseal_on_update() { // force update_sealing here to skip `reseal_required` checks chain.update_sealing(ForceUpdateSealing::Yes); } else if is_internal_import { chain.update_sealing(ForceUpdateSealing::No); }

yeah it was only for InstantSeal.

* Make InstantSeal Instant again * update_sealing if there are transactions in pool after impoerting a block, some line formatting * Apply suggestions from code review Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * InstantSeal specific behaviour * introduce engine.should_reseal_on_update, remove InstantSealService * remove unused code * add force param to update_sealing * better docc * even better docs * revert code changes, doc corrections, sort dep * code optimization * fix test * fix bench

* [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086) * Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053) * Add cargo-remote dir to .gitignore (?) * Update light client headers: ropsten 6631425 foundation 8798209 (#11201) * Update list of bootnodes for xDai chain (#11236) * ethcore/res: add mordor testnet configuration (#11200) * [chain specs]: activate Istanbul on mainnet (#11228) * [builtin]: support multiple prices and activations in chain spec (#11039) * [receipt]: add sender & receiver to RichReceipts (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * Made ecrecover implementation trait public (#11188) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Insert explicit warning into the panic hook (#11225) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Cleanup stratum a bit (#11161) * Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Type annotation for next_key() matching of json filter options (#11192) * Upgrade jsonrpc to latest (#11206) * [dependencies]: jsonrpc 14.0.1 (#11183) * Upgrade to jsonrpc v14 (#11151) * Switching sccache from local to Redis (#10971) * Snapshot restoration overhaul (#11219) * Add new line after writing block to hex file. (#10984) * Pause pruning while snapshotting (#11178) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Fix block detail updating (#11015) * Make InstantSeal Instant again #11186 * Filter out some bad ropsten warp snapshots (#11247)

Make InstantSeal Instant again

4a95a09

seunlanlege requested review from ordian and tomusdrw and removed request for ordian October 21, 2019 19:26

seunlanlege added A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations. M4-core ⛓ Core client code / Rust. labels Oct 21, 2019

tomusdrw reviewed Oct 21, 2019

View reviewed changes

update_sealing if there are transactions in pool after impoerting a b…

078de7e

…lock, some line formatting

seunlanlege force-pushed the seun-fix-instant-seal branch from 5c7027c to 078de7e Compare October 24, 2019 23:13

tomusdrw suggested changes Oct 29, 2019

View reviewed changes

seunlanlege and others added 4 commits October 29, 2019 20:34

Apply suggestions from code review

9f72414

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

InstantSeal specific behaviour

1224d25

introduce engine.should_reseal_on_update, remove InstantSealService

b4fd964

remove unused code

430d3a5

tomusdrw approved these changes Nov 9, 2019

View reviewed changes

Comment thread ethcore/src/miner/miner.rs Outdated

Comment thread ethcore/src/miner/miner.rs Outdated

Comment thread ethcore/engine/src/engine.rs Outdated

ordian added this to the 2.7 milestone Nov 9, 2019

seunlanlege added 3 commits November 9, 2019 16:07

add force param to update_sealing

6de0262

better docc

a83218b

even better docs

533a8b7

tomusdrw approved these changes Nov 9, 2019

View reviewed changes

Comment thread ethcore/engine/src/engine.rs Outdated

Comment thread ethcore/engines/instant-seal/Cargo.toml Outdated

Comment thread ethcore/src/miner/miner.rs Outdated

seunlanlege added 2 commits November 9, 2019 16:44

revert code changes, doc corrections, sort dep

5a68882

code optimization

d946819

tomusdrw approved these changes Nov 9, 2019

View reviewed changes

seunlanlege added 2 commits November 9, 2019 17:12

fix test

1fd4a17

fix bench

b6dc687

niklasad1 added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Nov 9, 2019

ordian approved these changes Nov 10, 2019

View reviewed changes

ordian merged commit 887aa62 into master Nov 10, 2019

ordian deleted the seun-fix-instant-seal branch November 10, 2019 10:41

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 10, 2019

soc1c mentioned this pull request Nov 11, 2019

v2.6.5 beta #11240

Merged

35 tasks

soc1c mentioned this pull request Nov 11, 2019

v2.5.10 stable #11239

Merged

36 tasks

seunlanlege added a commit that referenced this pull request Nov 11, 2019

Make InstantSeal Instant again (#11186)

d43ae58

Conversation

seunlanlege commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

seunlanlege commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvdplm commented Oct 21, 2019

Uh oh!

seunlanlege commented Oct 22, 2019

Uh oh!

seunlanlege commented Oct 22, 2019

Uh oh!

tomusdrw commented Oct 22, 2019

Uh oh!

dvdplm commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw commented Oct 22, 2019

Uh oh!

dvdplm commented Oct 22, 2019

Uh oh!

tomusdrw commented Oct 23, 2019

Uh oh!

seunlanlege commented Oct 29, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seunlanlege commented Nov 10, 2019

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

ordian Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

seunlanlege Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

ordian Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

seunlanlege Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seunlanlege commented Oct 21, 2019 •

edited

Loading

seunlanlege commented Oct 21, 2019 •

edited

Loading

dvdplm commented Oct 22, 2019 •

edited

Loading