Add SealingState; don't prepare block when not ready.

[afck]

This prevents the miner from creating new blocks for sealing even if it's not our turn to seal them.

However, it likely does not detect all cases yet where it's not our turn.

(Closes #70.)

[afck]

In my local test, it worked as expected, and it seemed to prevent most of the cases where we prepared unsealable blocks, but not all.

This looks like a safe change to me, but I wonder why seals_internally wasn't doing this in the first place. Let me know if you see any potential problems with this!

[DemiMarie]

Can we propose this upstream?

[DemiMarie]

I suspect the problem is missing synchronization.

Can we place a global lock around the engine and miner?

[afck]

Yes, I suspect that, too: Ideally, we do want to start preparing a block, even if our turn has not quite started yet, so that when it starts, we're ready to seal it. So basically we want to prepare one in our predecessor's turn as soon as we've received our predecessor's block, i.e. it shouldn't depend on time at all, but only on the latest imported block. So maybe this PR is the wrong approach after all?

Can we place a global lock around the engine and miner?

Somehow it seems in Parity every struct has all of its fields locked internally. It may be worth a try, but I suspect that global locks around engine and miner would cause them to deadlock, since there are places where (directly or indirectly) they call each other in both directions. 😬

[DemiMarie]

I think so. I suspect that the real answer is that anything we need to do in a block we are sealing, needs to be done in the actual function that seals the block.

[afck]

anything we need to do in a block we are sealing, needs to be done in the actual function that seals the block.

So you're essentially saying the issue doesn't have a solution, and we always need to prepare the block and try to seal it, like we currently do?

[afck]

I force-pushed a new approach that doesn't use self.step at all. (Not sure if it should.) Instead it only checks whether the block after the latest one is our turn.

[afck]

I also addressed the TODO: In on_prepare_block, we are always the block's author, so we don't have to take our address from the engine signer.

[DemiMarie]

@afck In retrospect, what I am saying is: on_prepare_block must be idempotent.

[DemiMarie]

What will happen if we are selected to validate for the first block after genesis? This should be checked.

Otherwise, this code looks good to me.

[DemiMarie]

Could this fail due to an I/O error involving the database?

[phahulin]

If I'm not mistaken, @DemiMarie 's remark can be extended to the case when at least one of the validators is down and skips blocks - then the network gets stuck because step is not advanced

2019-03-04 19:41:45  IO Worker #2 TRACE engine  Fetched proposer for step 310343469: 0xbbca…4c78
2019-03-04 19:41:45  IO Worker #2 TRACE engine  Not preparing block: not our turn. (Our address: 0x75df…9441)
...
2019-03-04 19:41:50  IO Worker #1 TRACE engine  Fetched proposer for step 310343469: 0xbbca…4c78
2019-03-04 19:41:50  IO Worker #1 TRACE engine  Not preparing block: not our turn. (Our address: 0x75df…9441)
...
2019-03-04 19:41:55  IO Worker #1 TRACE engine  Fetched proposer for step 310343469: 0xbbca…4c78
2019-03-04 19:41:55  IO Worker #1 TRACE engine  Not preparing block: not our turn. (Our address: 0x75df…9441)

[afck]

then the network gets stuck because step is not advanced

You're right! 😩 I don't have an idea yet how to solve this. But somehow generate_seal gets around that issue, so seals_internally should be able to do that, too…
I'll give it another try tomorrow.

[DemiMarie]

@afck is it in clear_empty_steps()?

[DemiMarie]

Fixed. We were using the wrong step value.

[DemiMarie]

@phahulin can you test this?

[phahulin]

@DemiMarie couldn't get past the first few (1-2) blocks with all nodes up:

2019-03-05 11:00:20  IO Worker #3 TRACE miner  update_sealing
2019-03-05 11:00:20  IO Worker #3 TRACE miner  requires_reseal: sealing enabled
2019-03-05 11:00:20  IO Worker #3 TRACE engine  Fetched proposer for step 310354564: 0x75df…9441
2019-03-05 11:00:20  IO Worker #3 TRACE miner  requires_reseal: should_disable_sealing=false; forced=true, has_local=false, internal=Some(true), had_requests=false
2019-03-05 11:00:20  IO Worker #3 TRACE miner  update_sealing: preparing a block
2019-03-05 11:00:20  IO Worker #3 TRACE miner  prepare_block: Already have previous work; updating and returning
2019-03-05 11:00:20  IO Worker #3 DEBUG miner  Attempting to push 1 transactions.
2019-03-05 11:00:20  IO Worker #3 DEBUG miner  Adding tx 0xb214dc5e9ee5c97ff243c2832db03a0c5ca414ec545f09406f036b770447e181 took 50 ms
2019-03-05 11:00:20  IO Worker #3 DEBUG miner  Pushed 0 transactions in 51 ms
2019-03-05 11:00:20  IO Worker #3 TRACE reward  Block reward benefactors: [(0x75df42383afe6bf5194aa8fa0e9b3d5f9e869441, Author)]
2019-03-05 11:00:20  IO Worker #3 TRACE engine  Fetched proposer for step 310354564: 0x75df…9441
2019-03-05 11:00:20  IO Worker #3 TRACE miner  update_sealing: engine indicates internal sealing
2019-03-05 11:00:20  IO Worker #3 TRACE miner  seal_block_internally: attempting internal seal.
2019-03-05 11:00:20  IO Worker #3 DEBUG engine  Aborting seal generation. The step or empty_steps have changed in the meantime. 340282366920938463463374607431768211454 != 340282366920938463463374607431768211443

I've seen Aborting seal... step or empty_steps have changed... before, but now it happens every step.

[afck]

I can confirm that it fails for me, too, both with and without the last commit, if I disable nodes 2 and 5.
With aura-pos, it works.

[afck]

…but disabling the sealing queue (i.e. explicitly never taking that branch) fixes it for me!
I'll try to find out why we ended up there in the first place; I don't understand when that block was prepared and queued…

[afck]

With this change it works for me.

Maybe Engine needs yet another getter so that we're able to either prevent this prepare_block call or to return None from prepare_block itself if the engine isn't ready for sealing.

[DemiMarie]

@afck It might, but it is probably a good idea to merge it as-is, unless @vkomenda or @phahulin say otherwise.

[phahulin]

IMO we shouldn't merge it yet, because the issue itself is not a blocker, and it's not yet clear (at least to me), what can we break by disabling the sealing queue.

[afck]

I agree; let's not merge something where we don't know what we're doing:
At least we should understand why this doesn't work with the sealing queue. Why doesn't the same problem already occur on aura-pos or stable…?

[afck]

OK, I think aura-pos and stable also don't use the sealing queue!

• This set_pending call is never made because we never return a Seal::Proposal.
• This one never happens because prepare_work is only called if seals_internally returns None, or in prepare_pending_block, which is not called here if seals_internally is always Some(true), and not called here, because work_package is probably also never called with Aura…?
• And this one is never reached because seals_internally never returns Some(false).

But I'm not sure yet what the proper solution is.

[DemiMarie]

@afck SealingState::Prepare is dead code. Do you plan to change that?

[afck]

Good point… so maybe we don't need SealingState at all, and just need to change the effect of Some(false) to what's now SealingState::None… I'll have a closer look tomorrow.

[DemiMarie]

I think that we should keep the SealingState enum, since it is much more descriptive than Option<bool>. Perhaps

enum SealingState {
    Ready,
    NotReady,
    External,
}

would be a good fit?

Also, if we never use the sealing queue, we can delete all of its supporting infrastructure.

[DemiMarie]

We should not include SealingState::Prepare but never construct it. Either there should be a use for it, or it should be removed.

Also, we should panic!() if we hit a branch that is unreachable.

[afck]

Thanks, I renamed the variants as you suggested, and removed Prepare.

I had originally meant Prepare to be what was Some(false) before, but it looks like Some(false) was only returned by some engines if there was no mining address. Since that means they can't ever seal a block anyway, I changed their return value to NotReady.

[DemiMarie]

Panic message needs to be updated. Otherwise, looks good.

[vkomenda]

Looks good. Some integration testing might be required as a separate task perhaps.

[afck]

I squashed and rebased.

[varasev]

@afck Please clarify how this can be tested? If I add a node which is not a validator to posdao-test-setup and launch this, there shouldn't be any warnings in its logs, right? So, if I don't see warnings for such a node, can this PR be considered as successful?

In other words, how can I understand that these changes work fine?

[afck]

I think it rather affects nodes that are validators: Without this change, they print messages that they are not ready to seal, because they prepare blocks all the time, even if it's not their turn:

TRACE engine  generate_seal: 0xbbca…4c78 not a proposer for step 309905993

But for some reason they're not even warnings, just traces, even though it seems like a big waste of CPU time.
So that's the part that's easy to test: That those messages are gone.

The hard part is: Could this PR cause any other problems in practice? That's why I'd really like someone to review it who is very familiar with the inner workings of Parity and Aura, and it's why I also made an upstream PR: openethereum#10529
(But nobody has looked at it yet.)

[varasev]

Ok, I left ping in that PR, hope the guys will help to check the code.

[varasev]

Since Parity approved the same changes for upstream, I'll try to launch the build based on the afck-seals-internally branch and if it's ok, we will merge it.

[varasev]

I tried to launch it and don't see the trace message TRACE engine generate_seal: 0xbbca…4c78 not a proposer for step ... anymore. Also, the CPU time seems to be reduced 👍 So, I think we can merge it.