fix NoProof block signature

[winsvega]

Other devs report that It is better if NoProof blocks still have the signature. but the signature is just ignored.
Rather then absence of signature as it is now.

[codecov-io]

Codecov Report

Merging #5219 into master will decrease coverage by 0.08%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #5219      +/-   ##
==========================================
- Coverage   60.84%   60.76%   -0.09%     
==========================================
  Files         338      338              
  Lines       27570    27420     -150     
  Branches     3195     3183      -12     
==========================================
- Hits        16776    16662     -114     
+ Misses       9669     9640      -29     
+ Partials     1125     1118       -7

[chfast]

h256{0}.

[chfast]

Maybe make it null?

[gumb0]

Why do you assign it here if you just checked that it's 0?

[winsvega]

because it returns 0 also when there is no signature set.

[winsvega]

We could implenent a method hasSignature() if you woukd like. That will change libethereum. By default I try not to touch libethereum for test related logic

[gumb0]

I don't get - if there's a change to NoProof blocks format - why only testeth files are changed?
Will test_mineBlocks generate correct blocks with NoProof config?

[winsvega]

It will generate correct blocks but without any signature. no nonce and mixhash in blockheader.

[gumb0]

I think we should make aleth generate the blocks with zero signature in NoProof mode

[gumb0]

Do you mean both blocks without any signature and blocks with zero signature are "correct"?
This doesn't make sense to me, only one format should be valid (the one that other clients would also support)

[winsvega]

Correct in what context?
blocks without signature are incorrect for the test format.
blocks with or without signature could still be invalid or correct depending on a block contents.
blocks without signature or with invalid signature are incorret on Ethash

[winsvega]

There is no consensus on blocks without signature cause NoProof is smth that we use for testing purposes. So we define what a block on NoProof should look like (with zero signature / random signature or no signature at all)
It is also possible to support blocks without signature in test format. It just that other clients devs agreed that it is easier for them to parse blocks with at least some signature.

[gumb0]

So we define what a block on NoProof should look like (with zero signature / random signature or no signature at all)

So I understood that intention of this PR is to establish that NoProof blocks have zero signature.

It just that other clients devs agreed that it is easier for them to parse blocks with at least some signature.

Right, so I'm saying we should generate blocks with signature everywhere.

blocks with or without signature could still be invalid or correct depending on a block contents.

Why should blocks without signature be valid?

[winsvega]

Blocks without signature are valid on NoProof

[gumb0]

But you say they are not valid for other clients

[winsvega]

Other clients parsing state test format expect signature in blocks. Only in test format. They dont have NoProof defenition. We could make it anything.

[gumb0]

We could, but why would we want two different block formats in tests and out of the tests? This will only lead to confusion and possible incompatibilities between parts of the codebase.

[winsvega]

Thats why this pr adding a zero signature to the blocks without signature

[chfast]

Thats why this pr adding a zero signature to the blocks without signature

But you don't change NoProof design, just hack the final test output.

I'm also a bit into the solution to make NoProof more strict and do not allow block headers without signature fields.

[winsvega]

By default I try not to touch core libraries logic for testing purposes.
So If you think that NoProof should be modified on a core level to require signature at the block but not verifying it we could open another PR.

To summarize the discussion:
Is it concluded that NoProof should generate 0 signature at corelib mining method?
Is it concluded that aleth should require signature to be present in a block rlp for any Ethash/NoProof

[chfast]

Yes, if that makes the whole system more consistent and simple to understand.
I also don't see having null signatures to be big performance penalty.

Can someone check if Clique is adding/remove any fields to the block hash?

[gumb0]

@chfast It does not change the format, just changes the meaning of some fields ethereum/EIPs#225

[winsvega]

updatet this PR. now 0 signature is set by default inside sealEngineBase generateSeal method.
that is used when NoProof is enabled.

Obsolate

[gumb0]

bcBasicInsert test is failing

[winsvega]

removed generateSeal from SealEngineBase
moved generateSeal from SealEngineBase to NoProof.
Fixed bcBasicImport by moving the old implementation of generateSeal from SealEngineBase to BasicAuthority generateSeal

There should be an error throwing in case no callback is called from BasicAuthority::generateSeal.
That leads to endless loop.

[gumb0]

Where is the endless loop?

[gumb0]

Maybe move enum { MixHashField = 0, NonceField = 1 }; to SealEngineBase and use it here

[winsvega]

this is a good question. what if with sealEngine = Ethash2 there will be 3 or 1 fields for signature?

[gumb0]

We'll see then, let's not try to guess the future

[winsvega]

Endless loop somewhere where it waits for a callback of m_onSealGenerated
because if you remove this m_onSealGenerated from generateSeal it will mine forever.
but in the code I see if (m_onSealGenerated) which means that might be a scenario when it is not set.

[gumb0]

I'd say it's a problem of the calling code if it would not set this handler and then wait somehow (how?) for it to be called.
So I don't think anything needs to be done here.

[winsvega]

fixed your last comments

[chfast]

This PR looks very nice in the end.