refactor(app): Extract payload validation utilities and reduce code duplication#199
refactor(app): Extract payload validation utilities and reduce code duplication#199
Conversation
app/src/state.rs
Outdated
| /// Stores an undecided proposal along with its block data. | ||
| /// | ||
| /// WARN: The order of the two storage operations is important. | ||
| /// TODO: Add more context on why the order is important. |
rnbguy
left a comment
rnbguy
left a comment
There was a problem hiding this comment.
Left few comments. Otherwise, LGTM.
| "💡 Sync block validated at height {} with hash: {}", | ||
| height, new_block_hash | ||
| ); | ||
| debug!(%height, "💡 Sync block validated"); |
There was a problem hiding this comment.
Not really as we no longer have the block hash. We could get it by extracting the execution payload from the block_bytes and then use execution_payload.payload_inner.payload_inner.block_hash, but I don't see the benefit of logging the block hash in a debug message.
app/src/app.rs
Outdated
| // Log stats | ||
| { | ||
| state.txs_count += tx_count as u64; | ||
| state.chain_bytes += block_bytes.len() as u64; | ||
| let elapsed_time = state.start_time.elapsed(); | ||
|
|
||
| state.metrics.tx_stats.add_txs(tx_count as u64); | ||
| state | ||
| .metrics | ||
| .tx_stats | ||
| .add_chain_bytes(block_bytes.len() as u64); | ||
| state | ||
| .metrics | ||
| .tx_stats | ||
| .set_txs_per_second(state.txs_count as f64 / elapsed_time.as_secs_f64()); | ||
| state | ||
| .metrics | ||
| .tx_stats | ||
| .set_bytes_per_second(state.chain_bytes as f64 / elapsed_time.as_secs_f64()); | ||
| state.metrics.tx_stats.set_block_tx_count(tx_count as u64); | ||
| state | ||
| .metrics | ||
| .tx_stats | ||
| .set_block_size(block_bytes.len() as u64); | ||
|
|
||
| // Persist cumulative metrics to database for crash recovery | ||
| state | ||
| .store | ||
| .store_cumulative_metrics(state.txs_count, state.chain_bytes, elapsed_time.as_secs()) | ||
| .await?; | ||
|
|
||
| info!( | ||
| "👉 stats at height {}: #txs={}, txs/s={:.2}, chain_bytes={}, bytes/s={:.2}", | ||
| height, | ||
| state.txs_count, | ||
| state.txs_count as f64 / elapsed_time.as_secs_f64(), | ||
| state.chain_bytes, | ||
| state.chain_bytes as f64 / elapsed_time.as_secs_f64(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The stats are not removed, just moved to a separate function, see log_block_stats.
| Validity::Invalid | ||
| }; | ||
|
|
||
| cache.insert(block_hash, validity); |
There was a problem hiding this comment.
we call validate_execution_payload inside on_decided. do we need this caching even inside on_decided ?
I would prefer to manage cache outside this method.
There was a problem hiding this comment.
I think this is just an optimization that would avoid caching the validation result for the proposer. We could add a param to indicate if the validation result should be cached, but I think it just adds complexity for very little benefit.
…ms/emerald into marius/proposal-validation
2 participants
Summary
payload.rsmoduleChanges
process_complete_proposal_partsto consolidate proposal validation logic used in bothon_received_proposal_partandon_started_roundpayload.rsmodule with:ValidatedPayloadCache- moved from state.rsvalidate_execution_payload- consolidated validation with cachingextract_block_headerandreconstruct_execution_payload- moved from state.rsstore_undecided_valueto combinestore_undecided_block_dataandstore_undecided_proposalcallsstore_undecided_block_datawrapper from Stateon_decidedandon_process_synced_valueto usevalidate_execution_payloadvalidate_payloadas it was only used internally byvalidate_execution_payload