Don't prepare blocks when not a validator.

[afck]

When adding a non-validator node, I see lots of warnings. It looks like on_close_block Miner::prepare_block is called anyway! One missing piece that's marked as TODO is extending Engine::seals_internally, so that it returns Some(false) if I'm not a validator (or it's not my turn, I guess).
But I'm not even sure whether that will solve the problem.

[afck]

Unassigning for now. This is just a warning and doesn't break the network, so I'll work on the contract changes first.

[varasev]

Does it throw an error for the current version of pos contracts? I removed requires from BlockReward.reward function: poanetwork/posdao-contracts@50b4c19

[varasev]

Also, those warnings are supposed to appear when engine_signer is defined in config, aren't they?

[afck]

Yes, it still behaves the same way with poanetwork/posdao-contracts@50b4c19. The contracts aren't at fault here.

Also, those warnings are supposed to appear when engine_signer is defined in config, aren't they?

That's when they appear currently, yes, because seals_internally just always returns Some(true) if a signer is defined. The TODO is about checking in addition whether it's actually our turn to create a block, so that we don't waste CPU time on creating lots of blocks that in the end we refuse to seal.

Note that it's not even warnings, just trace and debug messages:

2019-02-07 09:59:25  IO Worker #0 TRACE miner  update_sealing
2019-02-07 09:59:25  IO Worker #0 TRACE miner  requires_reseal: sealing enabled
2019-02-07 09:59:25  IO Worker #0 TRACE miner  requires_reseal: should_disable_sealing=false; forced=true, has_local=false, internal=Some(true), had_requests=false
2019-02-07 09:59:25  IO Worker #0 TRACE miner  update_sealing: preparing a block
2019-02-07 09:59:25  IO Worker #0 TRACE miner  prepare_block: No existing work - making new block
2019-02-07 09:59:25  IO Worker #0 TRACE engine  Multi ValidatorSet retrieved for block 0.
2019-02-07 09:59:25  IO Worker #0 TRACE engine  New block issued #30 ― calling emitInitiateChange()
2019-02-07 09:59:25  IO Worker #0 DEBUG miner  Attempting to push 1 transactions.
2019-02-07 09:59:25  IO Worker #0 DEBUG miner  Adding tx 0xcecfbd59e20fafc4a8ab4a36d7fc23f81b1899ce0e218db071d5aff0cb99b3c3 took 16 ms
2019-02-07 09:59:25  IO Worker #0 DEBUG miner  Pushed 1 transactions in 16 ms
2019-02-07 09:59:25  IO Worker #0 TRACE reward  Block reward benefactors: [(0xbbcaa8d48289bb1ffcf9808d9aa4b1d215054c78, Author)]
2019-02-07 09:59:25  IO Worker #0 TRACE miner  update_sealing: engine indicates internal sealing
2019-02-07 09:59:25  IO Worker #0 TRACE miner  seal_block_internally: attempting internal seal.
2019-02-07 09:59:25  IO Worker #0 DEBUG engine  Zooming to epoch for block 0xf88f…8154
2019-02-07 09:59:25  IO Worker #0 TRACE engine  Fetched proposer for step 309905993: 0x522d…89b2
2019-02-07 09:59:25  IO Worker #0 TRACE engine  generate_seal: 0xbbca…4c78 not a proposer for step 309905993.

But they show that the miner determines (via both seals_internally and forced_sealing() in this case) that it can seal. It then creates a new block, adds our contract calls and user transactions to it, and only then, in AuthorityRound::generate_seal realizes it can't actually seal the block.
Pushing transactions onto blocks involves executing them; I think that's quite wasteful in terms of CPU.

[varasev]

Ok, we could deal with this, I guess?

[afck]

Yes, it shouldn't break anything. It would still be nice to fix it, to avoid a lot of spam in the logs and wasted CPU time.

[DemiMarie]

Problem: can Call be fooled by malicious validators? If so, I don’t see any way to avoid a memory leak of cached reports.

[afck]

That's a good point; maybe we should just set some fixed limit for the size of the cache. (Did you mean to post in #97 instead?)

[afck]

The ideal fix for this would:

• Never start preparing a block with a number where we know it isn't our turn.
• Always start preparing a block a short while before it is our turn, so that when our turn arrives, we just finished preparing.

In particular we need to find out when exactly Miner::prepare_block and Engine::seals_internally are called.

Miner::prepare_block is private. It's called in Miner::prepare_pending_block (also private) if there's no block in the sealing queue, and in MinerService::update_sealing if Miner::requires_reseal is true on the best block number. Miner::prepare_pending_block is called in Miner::prepare_and_update_sealing if Engine::seals_internally is Some(true), and in MinerService::work_package if Engine::seals_internally is None. Miner::prepare_and_update_sealing is called in MinerService::import_external_transactions and MinerService::import_own_transactions. Let's find out where those four MinerService methods are called:

• MinerService::update_sealing is called in (one of the above and) MinerService::chain_new_blocks if new external blocks were imported. Apart from that, it is only called in Client's impl of EngineClient::update_sealing, which in turn is called in <AuthorityRound as Engine>::step and <TransitionHandler as IoHandler<()>>::timeout.
• MinerService::work_package is called from the rpc crate and <StratumJobDispatcher as JobDispatcher>::job.
• MinerService::import_external_transactions' call sites are <Client as IoClient>::queue_transactions and <Miner as MinerService>::import_claimed_local_transaction, which is also called from rpc.
• MinerService::import_own_transactions is called in lots of places.

I'm confused but it looks like preparing a new block is considered whenever new transactions or blocks are imported as well as periodically on a timeout.

Since in our case seals_internally will never return None (we don't require external input) it seems reasonable to just return Some(true) if the latest block known to the client indicates that it's our turn to seal the next one. (Assuming that preparing a block is quick enough to easily do it within our turn—that might be a problem for large block sizes or fast block rates!) But it certainly would be better to start slightly sooner than that.

[phahulin]

Just to put it here: it looks like new transactions, indeed, can cause a reseal of the block, depending on configuration:

     --reseal-on-txs=[SET]
        Specify which transactions should force the node to
        reseal a block. SET is one of: none - never reseal on
        new transactions; own - reseal only on a new local
        transaction; ext - reseal only on a new external
        transaction; all - reseal on all new transactions.
        (default: own)

[vkomenda]

Possibly this option is not taking a proper effect because reseal_on_txs has already been set to none in the configuration for each node and other validator's blocks still get prepared.