Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove cache from validation #646

Open
wants to merge 6 commits into
base: validate_clvm_and_sigs
Choose a base branch
from
Open

Remove cache from validation #646

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34c750a
first step to remove cache
matt-o-how Aug 6, 2024
4a039a5
change api to not use cache
matt-o-how Aug 6, 2024
460f3e3
fmt again
matt-o-how Aug 6, 2024
722d553
fix pytest and unused api import
matt-o-how Aug 6, 2024
5cbed30
hash_pk_and_msg as a factored function
matt-o-how Aug 6, 2024
5d0f4d8
move to for loop and derive Copy for GTElement
matt-o-how Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 42 additions & 30 deletions crates/chia-consensus/src/spendbundle_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ use crate::gen::opcodes::{
use crate::gen::owned_conditions::OwnedSpendBundleConditions;
use crate::gen::validation_error::ErrorCode;
use crate::spendbundle_conditions::get_conditions_from_spendbundle;
use chia_bls::BlsCache;
use chia_bls::PairingInfo;
use chia_bls::{aggregate_verify_gt, hash_to_g2};
use chia_protocol::SpendBundle;
use clvmr::sha2::Sha256;
use clvmr::{ENABLE_BLS_OPS_OUTSIDE_GUARD, ENABLE_FIXED_DIV, LIMIT_HEAP};
use std::sync::{Arc, Mutex};
use std::time::{Duration, Instant};

// currently in mempool_manager.py
Expand All @@ -24,14 +24,14 @@ pub fn validate_clvm_and_signature(
max_cost: u64,
constants: &ConsensusConstants,
height: u32,
cache: &Arc<Mutex<BlsCache>>,
) -> 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to clone the spends here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an unfortunate move that happens which means we can't return npcresult unless we clone it earlier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect you the lambda to take the entry by reference, and return a reference

let iter = spends.iter().flat_map(|spend| {
let condition_items_pairs = [
(AGG_SIG_PARENT, &spend.agg_sig_parent),
(AGG_SIG_PUZZLE, &spend.agg_sig_puzzle),
Expand All @@ -46,32 +46,52 @@ pub fn validate_clvm_and_signature(
.flat_map(move |(condition, items)| {
let spend_clone = spend.clone();
items.iter().map(move |(pk, msg)| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(
pk,
make_aggsig_final_message(
condition,
msg.as_slice(),
&spend_clone,
constants,
),
)
let mut aug_msg = pk.to_bytes().to_vec();
let msg = make_aggsig_final_message(
condition,
msg.as_slice(),
&spend_clone,
constants,
);
aug_msg.extend_from_slice(msg.as_ref());
let aug_hash = hash_to_g2(&aug_msg);
let pairing = aug_hash.pair(pk);

(hash_pk_and_msg(&pk.to_bytes(), &msg), pairing)
})
})
});
let unsafe_items = npcresult
.agg_sig_unsafe
.iter()
.map(|(pk, msg)| (pk, msg.as_slice().to_vec()));
let unsafes = npcresult.agg_sig_unsafe.clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should clone this either

let unsafe_items = unsafes.iter().map(|(pk, msg)| {
let mut aug_msg = pk.to_bytes().to_vec();
aug_msg.extend_from_slice(msg.as_ref());
let aug_hash = hash_to_g2(&aug_msg);
let pairing = aug_hash.pair(pk);
(hash_pk_and_msg(&pk.to_bytes(), msg), pairing)
});
let iter = iter.chain(unsafe_items);

// Verify aggregated signature
let (result, added) = cache
.lock()
.unwrap()
.aggregate_verify(iter, &spend_bundle.aggregated_signature);
let result = aggregate_verify_gt(
&spend_bundle.aggregated_signature,
iter.clone().map(|tuple| tuple.1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
if !result {
return Err(ErrorCode::BadAggregateSignature);
}
Ok((npcresult, added, start_time.elapsed()))
Ok((
npcresult,
iter.map(|tuple| (tuple.0, tuple.1.to_bytes().to_vec()))
.collect(),
start_time.elapsed(),
))
}

fn hash_pk_and_msg(pk: &[u8], msg: &[u8]) -> [u8; 32] {
let mut hasher = Sha256::new();
hasher.update(pk);
hasher.update(msg);
hasher.finalize()
}

pub fn get_flags_for_height_and_constants(height: u32, constants: &ConsensusConstants) -> u32 {
Expand Down Expand Up @@ -161,7 +181,6 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm,
&TEST_CONSTANTS,
236,
&Arc::new(Mutex::new(BlsCache::default())),
)
.expect("SpendBundle should be valid for this test");
}
Expand Down Expand Up @@ -193,7 +212,6 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm,
&TEST_CONSTANTS,
236,
&Arc::new(Mutex::new(BlsCache::default())),
)
.expect("SpendBundle should be valid for this test");
}
Expand Down Expand Up @@ -222,15 +240,13 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm / 2, // same as mempool_manager default
&TEST_CONSTANTS,
236,
&Arc::new(Mutex::new(BlsCache::default())),
);
assert!(matches!(result, Ok(..)));
let result = validate_clvm_and_signature(
&spend_bundle,
TEST_CONSTANTS.max_block_cost_clvm / 3, // lower than mempool_manager default
&TEST_CONSTANTS,
236,
&Arc::new(Mutex::new(BlsCache::default())),
);
assert!(matches!(result, Err(ErrorCode::CostExceeded)));
}
Expand Down Expand Up @@ -271,7 +287,6 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm,
&TEST_CONSTANTS,
1,
&Arc::new(Mutex::new(BlsCache::default())),
)
.expect("SpendBundle should be valid for this test");
}
Expand Down Expand Up @@ -321,7 +336,6 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm,
&TEST_CONSTANTS,
TEST_CONSTANTS.hard_fork_height + 1,
&Arc::new(Mutex::new(BlsCache::default())),
)
.expect("SpendBundle should be valid for this test");
}
Expand Down Expand Up @@ -365,7 +379,6 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm,
&TEST_CONSTANTS,
TEST_CONSTANTS.hard_fork_height + 1,
&Arc::new(Mutex::new(BlsCache::default())),
)
.expect("SpendBundle should be valid for this test");
}
Expand Down Expand Up @@ -409,7 +422,6 @@ ff01\
TEST_CONSTANTS.max_block_cost_clvm,
&TEST_CONSTANTS,
TEST_CONSTANTS.hard_fork_height + 1,
&Arc::new(Mutex::new(BlsCache::default())),
)
.expect("SpendBundle should be valid for this test");
}
Expand Down
1 change: 0 additions & 1 deletion tests/test_blscache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,6 @@ def test_validate_clvm_and_sig():
DEFAULT_CONSTANTS.MAX_BLOCK_COST_CLVM,
DEFAULT_CONSTANTS,
DEFAULT_CONSTANTS.HARD_FORK_HEIGHT + 1,
cache,
)

assert sbc is not None
Expand Down
1 change: 0 additions & 1 deletion wheel/generate_type_stubs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@ def validate_clvm_and_signature(
max_cost: int,
constants: ConsensusConstants,
peak_height: int,
cache: Optional[BLSCache],
) -> Tuple[SpendBundleConditions, List[Tuple[SpendBundleConditions, List[Tuple[bytes32, bytes]]]], float]: ...

def get_conditions_from_spendbundle(
Expand Down
1 change: 0 additions & 1 deletion wheel/python/chia_rs/chia_rs.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ def validate_clvm_and_signature(
max_cost: int,
constants: ConsensusConstants,
peak_height: int,
cache: Optional[BLSCache],
) -> Tuple[SpendBundleConditions, List[Tuple[SpendBundleConditions, List[Tuple[bytes32, bytes]]]], float]: ...

def get_conditions_from_spendbundle(
Expand Down
19 changes: 5 additions & 14 deletions wheel/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ use pyo3::types::PyList;
use pyo3::types::PyTuple;
use pyo3::wrap_pyfunction;
use std::iter::zip;
use std::sync::{Arc, Mutex};

use crate::run_program::{run_chia_program, serialized_length};

Expand Down Expand Up @@ -368,20 +367,12 @@ pub fn py_validate_clvm_and_signature(
max_cost: u64,
constants: &ConsensusConstants,
peak_height: u32,
cache: Option<BlsCache>,
) -> PyResult<(OwnedSpendBundleConditions, Vec<PairingInfo>, f32)> {
let real_cache = cache.unwrap_or_default();
let (owned_conditions, additions, duration) = validate_clvm_and_signature(
new_spend,
max_cost,
constants,
peak_height,
&Arc::new(Mutex::new(real_cache)), // TODO: use cache properly
)
.map_err(|e| {
let error_code: u32 = e.into();
PyErr::new::<PyTypeError, _>(error_code)
})?; // cast validation error to int
let (owned_conditions, additions, duration) =
validate_clvm_and_signature(new_spend, max_cost, constants, peak_height).map_err(|e| {
let error_code: u32 = e.into();
PyErr::new::<PyTypeError, _>(error_code)
})?; // cast validation error to int
Ok((owned_conditions, additions, duration.as_secs_f32()))
}

Expand Down
Loading