feat: don't include hashes of preimages in execution witness #14503
Conversation
meyer9
requested review from
joshieDo,
rkrasiuk,
rakita,
shekhirin,
mattsse,
Rjected and
fgimenez
as code owners
| fn witness(&self, input: TrieInput, target: HashedPostState) -> ProviderResult<Vec<Bytes>> { | ||
| TrieWitness::overlay_witness(self.tx(), input, target) | ||
| .map_err(ProviderError::from) | ||
| .map(|hm| hm.values().cloned().collect()) |
There was a problem hiding this comment.
now we have an extra clone here, not sure that's desired due to perf impact. additionally, usually its best practice to let the caller do the cloning instead of hiding costly ops under the hood (can also be written in docs though that this method clones). is it possible instead to return an iterator?
There was a problem hiding this comment.
its best practice to let the caller do the cloning instead of hiding costly ops under the hood
Good callout and good to know for future reference! I tried returning a Box<dyn Iterator<Item = Bytes>>, but there's a macro called delegate_impls_to_as_ref which doesn't correctly handle iterator lifetimes, so I wasn't able to return an iterator here. Other functions in this trait return vecs like proof and multiproof, so I think it's OK for now, although I'd be interested to learn how this can be implemented more efficiently.
Let me know if you have any suggestions on how to make this work!
crates/rpc/rpc/src/debug.rs
Outdated
| @@ -655,7 +655,7 @@ where | |||
| let state = state_provider | |||
| .witness(Default::default(), hashed_state) | |||
| .map_err(EthApiError::from)?; | |||
| Ok(ExecutionWitness { state: state.into_iter().collect(), codes, keys }) | |||
| Ok(ExecutionWitness { state: state.clone(), codes, keys }) | |||
There was a problem hiding this comment.
now we also have this extra clone, same here not sure if we want that. can it be avoided?
mattsse
added
the
S-blocked
Switch execution witness to return vectors instead of hashes. There's no reason to include the hash of the preimage in the response, potentially allowing callers to trust the EL client unnecessarily. No other clients implement this RPC and the only usage currently is in optimism and kona. I'll update these usages once this is merged.
Request from geth team here: ethereum/go-ethereum#30613 (comment)
Once this is complete, I can start implementing
debug_executePayloadmethod in geth and execution-apis.Edit: this is blocked by #14503