PT-168025582 Relax protocol version checks in block serialization #2678
Conversation
jur0
added
the
status/wip
jur0
removed
the
status/wip
jur0
force-pushed
the
PT-168025582-relax-protocol-version-checks-in-block-serialization
branch
2 times, most recently
from
231cac5
to
662d15f
Compare
jur0
force-pushed
the
PT-168025582-relax-protocol-version-checks-in-block-serialization
branch
4 times, most recently
from
c111ffb
to
4eaa99d
Compare
jur0
force-pushed
the
PT-168025582-relax-protocol-version-checks-in-block-serialization
branch
from
4eaa99d
to
7852b40
Compare
| @@ -581,26 +581,21 @@ deserialize_key_from_binary(<<Version:32, | |||
| Time:64, | |||
| Info/binary | |||
| >>) -> | |||
| case aec_hard_forks:protocol_effective_at_height(Height) =:= Version of | |||
There was a problem hiding this comment.
I feel uncomfortable by just removing these version checks... I don't see immediately how this can be exploited, but it needs a lot of reasoning to conclude that nobody in the future will add a block on Lima height with Minerva version...
| @@ -337,24 +337,14 @@ update_micro_candidate(#mic_block{} = Block, TxsRootHash, RootHash, Txs) -> | |||
| serialize_to_binary(#key_block{} = Block) -> | |||
| aec_headers:serialize_to_binary(to_key_header(Block)); | |||
| serialize_to_binary(#mic_block{} = Block) -> | |||
There was a problem hiding this comment.
For avoiding future misuse of this function. Please add a comment:
%% Serialization assumes the protocol version in the header to be valid for the provided height. This should have been validated before calling this function.
| }, | ||
| {ok, H} | ||
| end; | ||
| PowEvidence = deserialize_pow_evidence_from_binary(PowEvidenceBin), |
There was a problem hiding this comment.
Same kind of warning in comment here would be in place
jur0
force-pushed
the
PT-168025582-relax-protocol-version-checks-in-block-serialization
branch
from
f437214
to
b4d2ea7
Compare
| %% The function does not check the validity of the protocol version based on | ||
| %% height. It gets the protocol version from the block header. The protocol | ||
| %% version check based on height is performed before inserting it into the | ||
| %% database (aec_conductor). |
jur0
deleted the
PT-168025582-relax-protocol-version-checks-in-block-serialization
branch
| Err = {error, _} -> | ||
| Err | ||
| end. | ||
| Version = aec_headers:version(Header), |
There was a problem hiding this comment.
The validation height/Vsn is lost here. Perhaps add the comment here as well.
Are you sure this is validated elsewhere, and that we do not rely on these checks?
I am more worried about the deserialization than the serialization.
There was a problem hiding this comment.
The deserialization happens in peer connection which tries to add the deserialized block by calling aec_conductor:post_block and the version check is done there.
PT-168025582
Preparatory PR for fork signalling.
deserialize_[key|micro]_from_binaryis called inaec_blocks:deserialize_from_binarywhich is used in:aeu_import:disklog_fold/3- not used anywhere?!aec_peer_connection:deserialize_block/2- whenaec_peer_connectionwants to insert a new block to the db, it callsaec_conductor:post_block/1where the block is validated (including the version check).serialization_templatecalled inaec_blocks:serialize_to_binaryusedaec_hard_forks:protocol_effective_at_heightwhich is not needed becauseaec_blocks:serialize_to_binaryis called for blocks read from the db, so they were validated for the right protocol version when inserted into the db.