AntelopeIO / spring

C++ implementation of the Antelope protocol with Savanna consensus
Other
5 stars 2 forks source link

Verify important block invariants (specifically in finality and QC extensions) for Legacy and Transition blocks #691

Closed arhag closed 2 weeks ago

arhag commented 2 weeks ago

Background and suggest protocol compatible simplifications to verify_qc_claim

verify_qc_claim was designed to work whether no matter whether the block it was called on was a Legacy block, Transition block, or Proper Savanna block.

But it turns out the current implementation only calls verify_qc_claim on Proper Savanna blocks (in fact it, from a type perspective, it cannot even call it on a Legacy block).

https://github.com/AntelopeIO/spring/blob/21cec1fa496587d825e45862ec1d09c895c4362e/libraries/chain/controller.cpp#L4006-L4011

But consider this part of verify_qc_claim: https://github.com/AntelopeIO/spring/blob/21cec1fa496587d825e45862ec1d09c895c4362e/libraries/chain/controller.cpp#L3897-L3916

If verify_qc_claim is only called for Proper Savanna blocks, then that code can be simplified to just an EOS_ASSERT that checks that header_ext is present.

Furthermore, consider this part of verify_qc_claim: https://github.com/AntelopeIO/spring/blob/21cec1fa496587d825e45862ec1d09c895c4362e/libraries/chain/controller.cpp#L3928-L3934

If verify_qc_claim is only called for Proper Savanna blocks, then that code can also be simplfied to just an EOS_ASSERT that checks that prev_finality_ext is present.

The code is still safe for blocks other Proper Savanna blocks, especially when considering there is also the later check that happens in block_header_state::next: https://github.com/AntelopeIO/spring/blob/21cec1fa496587d825e45862ec1d09c895c4362e/libraries/chain/block_header_state.cpp#L442-L443

But the above simplifications are valuable to make the code clearer.

Undesired blockchain validation rules in existing Spring implementation

None of the above so far is an actual bug. However, the fact that verify_qc_claim is not called for Legacy or Transition blocks means that the current implementation of Spring will accept Legacy blocks (i.e. blocks without a finality block header extension) and Transition blocks (i.e. block with a finality block header extension but that has a schedule_version that is not equal to the proper_svnn_schedule_version constant) even if they have a QC block extension present. In fact, there are other block invariants that can be violated for Legacy and Transition blocks that the current Spring implementation does not check for but really should.

The scope of this issue is to ensure that all relevant block invariants for blocks of the different block types (Legacy, Transition, and Proper Savanna) are validated in create_block_state_i prior to constructing the block state.

The relevant block invariants are described below.

Block invariants to validate for the different types of blocks (Legacy, Transition, Proper Savanna)

A Legacy block is defined by not having a finality block header extension.

There are a simple set of checks that are needed for Legacy blocks only:

A Transition block is defined by a block that has a finality block header extension present but does not have a schedule_version set to the proper_svnn_schedule_version constant (i.e. is_proper_svnn_block() returns false).

Then there are a set of checks that need to be done for Transition blocks:

A Proper Savanna block is defined as a block that has a schedule_version set to the proper_svnn_schedule_version constant (i.e. is_proper_svnn_block() returns true). This also necessarily implies that it has a finality block header extension present (assuming it was a valid block).

Then there are a set of basic checks that need to be done for Proper Savanna blocks:

Finally, there is complete QC validation that needs to only be done for Proper Savanna blocks that have a QC block extension present:

Protocol compatibility implications

Technically, to prevent such undesired Legacy and Transition blocks from being accepted would require changes to the implementation which amount to a blockchain validation protocol change compared to Spring 1.0.0-rc1, 1.0.0-rc2, and 1.0.0-rc3. But the reality is that the unmodified nodeos implementation would not produce blocks that violate any of these invariants. And furthermore, to violate almost all of these invariants in a produced block, it would take a producer (a somewhat trusted entity in most realizations of Antelope chains) to run a modified version of the nodeos implementation. The one exception is that a man-in-the-middle could add a superfluous (and incorrect) QC block extension to Legacy and Transition blocks that would be accepted (and propagated) by the current nodeos implementation and potentially be written out to the irreversible block log. But even the block validation protocol was to be made stricter in the future, it would be possible to prune out these undesired superfluous QCs from the irreversible blocks log.

So it is extremely unlikely for the protocol change (which is still allowed in release candidate stage) to cause problems with any existing (test) chains out there that have already deployed prior release candidates of 1.0.0 or even already transition to Savanna.

heifner commented 2 weeks ago

But it turns out the current implementation only calls verify_qc_claim on non-legacy blocks (so IF Transition Blocks and Proper IF Blocks):

It is not called on IF Transition Blocks as IF Transition Blocks are only transmitted as legacy blocks and then transitioned on each node from legacy to savanna. The verify_qc_claim is not called on the IF Transition Blocks as they are created in the node.

The blocks are Transition blocks. I was thinking of block_state. The verify qc claim is not called on block_state of transition blocks. So ignore this comment.