[Blockchain] Simplify block verification

[zivkovicmilos]

Description

This PR aims to separate out block insertion and block verification.
There are 2 kinds of block verifications that can occur:

• Node wants to verify a previously sealed block (from a different node)
• Node wants to verify a proposed block (from a different node)

It also refactors the way the blockchain module is tested.

Changes include

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

• I have assigned this PR to myself
• I have added at least 1 reviewer
• I have added the relevant labels
• I have updated the official documentation
• I have added sufficient documentation in code

Testing

• I have tested this code with the official test suite
• I have tested this code manually

Additional comments

Fixes EDGE-469

[dbrajovic]

LGTM. Great job on breaking up the WriteBlock logic as well as adding test coverage 🙏

[Kourin1996]

Just a few comments

[Kourin1996]

Just a comment. Blockchain struct will be no longer safe because it's possible to insert block with incorrect parent hash due to wrong way to use Blockchain. (Please correct me if I'm misreading)

My suggestion is to add checking ParentHash in WriteBlock at least.

[zivkovicmilos]

@Kourin1996

You're right, but it comes down to the semantics of WriteBlock. Everywhere in the codebase it's been used with the assumption that it will also verify in addition to writing.

After I've decoupled these concepts, I needed to add a VerifySealedBlock from every module that was expecting the verification (I believe only restore and syncer).

@dbrajovic what do you think about @Kourin1996's suggestion?

[Kourin1996]

You're right, but it comes down to the semantics of WriteBlock. Everywhere in the codebase it's been used with the assumption that it will also verify in addition to writing.

@zivkovicmilos I don't think this is the verification. It's like a nil check before inserting.

[0xAleksaOpacic]

LGTM! I have manually tested branch with old chain for backwards compactibility. It works

[zivkovicmilos]

@Kourin1996

I've added a notice to the WriteBlock method:

a74f79a

People who need to call WriteBlock, will not expect verification. If they want verification, they can call the verification method before committing the block locally

[dbrajovic]

cc @zivkovicmilos @Kourin1996

Judging by all the places where WriteBlock() is used, there should be no side effects despite taking the block verification outside of the method. At each call site,I backtracked the stack to see if verification is still ensured after the call to WriteBlock - this does seem to be case.

As for the naming, it is anti-pattern, but so is the blockchain module doing any verification in the first place. This should be tackled in a separate PR, imo.

[Kourin1996]

@zivkovicmilos Could you check whether node check each CommittedSeals signature before inserting block?

[zivkovicmilos]

@Kourin1996

I've gone into a detailed comparison between checks done on develop and checks done on this branch - the only thing that I found missing was the CommittedSeals check before inserting a block, as it was previously contained inside WriteBlock.

I've fixed this in:

92f8113