fix(app): prune block headers and improve sync value handling - #210
Conversation
| // This indicates a serious issue: 2/3+ validators accepted this value, | ||
| // but our validation failed. This suggests state divergence, execution | ||
| // client problems, or Byzantine behavior. | ||
| return Err(eyre::eyre!( |
There was a problem hiding this comment.
I wonder whether the panic here is an attack vector on a node. Namely, if you send me an invalid proposal, instead of making it invalid, I crash here without holding you accountable.
There was a problem hiding this comment.
Malachite verifies the certificate before passing the proposal to the application. This means that a peer can only send an invalid proposal if it can create a valid certificate, which is only possible if more than 1/3 of the voting power is malicious.
There was a problem hiding this comment.
Ok so indeed this is an invalid scenario, I checked malachite's code to make sure that the certificates are indeed validated before the value is validated (in a previous version, those validations were happening in parallel). So yes, this is a serious problem which should be cause a crash,
Summary
decode_valueon_process_synced_valueby sendingNonereplyon_decided()can retrieve block dataDECIDED_BLOCK_HEADERS_TABLEby pruning it alongside certificatesChanges
Error handling improvements
decode_valuenow returnsResultinstead of unwrappingreplay_heights_to_enginepropagates decode errors with contexton_process_synced_valuesendsNonereply on decode failure instead of panickingSync value handling
Performance
earliest_certificate_heightandearliest_value_heightin StateBug fix
DECIDED_BLOCK_HEADERS_TABLEis now pruned alongside certificates (was growing unbounded)