Skip to content

refactor: reorder types in multiproof.rs #19182

@yongkangc

Description

@yongkangc

Describe the feature

Currently its hard to navigate. We should reorder the file in

consts
types
pub structs
fns

the main type should always be top of file. the main type should always be top of file
just trips me up if I navigate to a file proof_task and then the first types are some private internal types and I have to navigate up and down

Additional comments to address:

https://github.com/paradigmxyz/reth/pull/19203/files#r2456286060

we can maybe separate the handling of messages into separate methods so it's easier to navigate, but not a blocker
#19203 (comment)

#19203 (comment)
yeah this feels a bit weird because we first clone stuff from self into the
StorageMultiproofInput only to drop it here
imo this entire type StorageMultiproofInput is redundant

To do that, we convert it back to the storage proof only

// Extract storage proof from the multiproof
let storage_proof = multiproof.storages.remove(&hashed_address).ok_or_else(|| {
ParallelStateRootError::StorageRoot(StorageRootError::Database(DatabaseError::Other(
format!("storage proof not found in multiproof for {hashed_address}"),
)))
})?;

feels like a massive footgun, but Im lacking context

Previously we did this https://github.com/paradigmxyz/reth/blob/4a24cb3b499b997f93de9b5a01bfd0ecfe8339c1/crates/engine/tree/src/tree/payload_processor/multiproof.rs#L[…]71, in order to send the result of storage proof calculation back to the multiproof manager. This is basically constructing a multiproof that has just the storage proof.
When receiving the ProofCalculated in multiproof manager, we didn't distinguish between account or storage proofs, it's just a multiproof.
Now, we do the same here

// Convert storage proof to account multiproof format
let result_msg = match result {
Ok(storage_proof) => {
let multiproof = reth_trie::DecodedMultiProof::from_storage_proof(
hashed_address,
storage_proof,
);
let stats = crate::stats::ParallelTrieTracker::default().finish();
Ok((multiproof, stats))
}
Err(e) => Err(e),
};
, but this codepath is also used in ParallelProof::storage_proof, and it should return only the storage proof as-is, not the multiproof with the storage proof in it.
To do that, we convert it back to the storage proof only
// Extract storage proof from the multiproof
let storage_proof = multiproof.storages.remove(&hashed_address).ok_or_else(|| {
ParallelStateRootError::StorageRoot(StorageRootError::Database(DatabaseError::Other(
format!("storage proof not found in multiproof for {hashed_address}"),
)))
})?;

we definitely should clean this up before releasing, because now we do storages.remove() in the hot path of calculating an account multiproof, basically allocating a whole DecodedMultiProof and then deallocating it for nothing

Metadata

Metadata

Assignees

Labels

C-debtA clean up/refactor of existing codeC-enhancementNew feature or requestS-needs-triageThis issue needs to be labelled

Type

No type

Projects

Status

Backlog

Relationships

None yet

Development

No branches or pull requests

Issue actions