Remove cache from validation #646
Conversation
| ) -> Result<(OwnedSpendBundleConditions, Vec<PairingInfo>, Duration), ErrorCode> { | ||
| let start_time = Instant::now(); | ||
| let mut a = make_allocator(LIMIT_HEAP); | ||
| let sbc = get_conditions_from_spendbundle(&mut a, spend_bundle, max_cost, height, constants) | ||
| .map_err(|e| e.1)?; | ||
| let npcresult = OwnedSpendBundleConditions::from(&a, sbc); | ||
| let iter = npcresult.spends.iter().flat_map(|spend| { | ||
| let spends = npcresult.spends.clone(); |
There was a problem hiding this comment.
I don't think you need to clone the spends here
There was a problem hiding this comment.
There's an unfortunate move that happens which means we can't return npcresult unless we clone it earlier
There was a problem hiding this comment.
I would expect you the lambda to take the entry by reference, and return a reference
| let mut hasher = Sha256::new(); | ||
| hasher.update(pk.to_bytes()); | ||
| hasher.update(msg.as_slice()); | ||
| let hash: [u8; 32] = hasher.finalize(); |
There was a problem hiding this comment.
it would probably make sense to factor this out as a separate function, as this code is repeated.
| @@ -46,32 +46,54 @@ pub fn validate_clvm_and_signature( | |||
| .flat_map(move |(condition, items)| { | |||
| let spend_clone = spend.clone(); | |||
| items.iter().map(move |(pk, msg)| { | |||
There was a problem hiding this comment.
is there a good reason to move the spends into this lambda function? That's probably why you have to clone the spends in the first place.
| .agg_sig_unsafe | ||
| .iter() | ||
| .map(|(pk, msg)| (pk, msg.as_slice().to_vec())); | ||
| let unsafes = npcresult.agg_sig_unsafe.clone(); |
There was a problem hiding this comment.
I don't think you should clone this either
| .aggregate_verify(iter, &spend_bundle.aggregated_signature); | ||
| let result = aggregate_verify_gt( | ||
| &spend_bundle.aggregated_signature, | ||
| iter.clone().map(|tuple| tuple.1), |
There was a problem hiding this comment.
Cloning the iterator here probably means you'll perform the work in the lambda twice, each time you iterate over it. It might be a good reason to collect() the GTElements first. Alternatively, build the return vector as part of this lambda function. That would be the most efficient.
| @@ -13,7 +13,7 @@ use std::ops::{Mul, MulAssign}; | |||
| pyo3::pyclass, | |||
| derive(chia_py_streamable_macro::PyStreamable) | |||
| )] | |||
| #[derive(Clone)] | |||
| #[derive(Clone, Copy)] | |||
There was a problem hiding this comment.
I believe the GTElement is more than 500 bytes large. I think it's pretty good to require explicit cloning, to see where we pay that cost.
|
|
||
| // Verify aggregated signature | ||
| let result = aggregate_verify_gt( | ||
| &spend_bundle.aggregated_signature, | ||
| iter.clone().map(|tuple| tuple.1), | ||
| pairs.iter().map(|tuple| tuple.1), |
There was a problem hiding this comment.
| pairs.iter().map(|tuple| tuple.1), | |
| pairs.iter().map(|tuple| &tuple.1), |
does this work?
Creating a new branch and PRing into
validate_clvm_and_sigsas this change is rather big.