Add functions for multithreaded validation to the Rust side.

[matt-o-how]

[DRAFT PR]

[coveralls-official]

Pull Request Test Coverage Report for Build 10164470436

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 522 of 551 (94.74%) changed or added relevant lines in 6 files are covered.
• 81 unchanged lines in 9 files lost coverage.
• Overall coverage increased (+0.5%) to 83.158%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/chia-consensus/src/gen/condition_tools.rs	73	74	98.65%
crates/chia-consensus/src/spendbundle_validation.rs	344	346	99.42%
wheel/src/api.rs	25	51	49.02%

Files with Coverage Reduction	New Missed Lines	%
crates/clvm-traits/src/match_byte.rs	2	92.5%
crates/clvm-traits/src/to_clvm.rs	3	98.45%
crates/chia-protocol/src/program.rs	4	71.86%
crates/clvm-derive/src/from_clvm.rs	4	98.77%
crates/clvm-derive/src/to_clvm.rs	5	98.1%
crates/clvm-traits/src/clvm_decoder.rs	9	75.0%
crates/clvm-traits/src/clvm_encoder.rs	15	57.5%
crates/clvm-traits/src/from_clvm.rs	19	90.87%
crates/chia-protocol/src/bytes.rs	20	85.51%

Totals
Change from base Build 10066449910:	0.5%
Covered Lines:	12156
Relevant Lines:	14618

---

💛 - Coveralls

[arvidn]

this function is key, and should probably be pulled out first, as a separate commit. It's also sensitive as it's the one defining the hard fork and soft-forks. There are currently some PRs in-flight modifying it. So it will need to be updated after those land.

[Rigidity]

Imo we should name this something more clear, like spend_bundle_validation. We aren't using multi-processing anymore.

[matt-o-how]

Done

[Rigidity]

Is this going to be uncommented/removed later?

[matt-o-how]

Done

[Rigidity]

nit: you could conditionally assign these functions to a variable then call the variable as a function instead of duplicating this

[arvidn]

I don't think you can port this function to rust (yet). This is the python function that posts validate_clvm_and_signature to a ProcessPoolExecutor, which we'll change to a ThreadPoolExecutor once we complete this project.

[matt-o-how]

Removed

[arvidn]

I think you should make this a specialized function for validating mempool transactions. Maybe it could be called something like mempool_validate_spendbundle().
You wouldn't need to take the mempool parameter, and you could set the MEMPOOL_MODE flag unconditionalle.

[matt-o-how]

Eventually we'll want to use it for other purposes though right? The mempool param isn't taking up a lot of overhead

[arvidn]

Most of the suggestions I've made to simplify this was based on the assumption that we would use a separate function for full blocks.

The cost of making this function work both for mempool (i.e. single spend bundles) and full blocks is:

1. We would need to turn the spend bundle into a block before calling this function (in order to have a uniform interface). This function would then need to run said block generator function to get back to the individual spends. As it is now, we can skip that part.
2. We would need to support both pre- and post- hard fork (since blocks need to validate with the pre-hard fork rules). Transactions going into the mempool only need to be validated with the post-hard fork rules however.

[arvidn]

this should have a unit test. I would recommend rstest so you can parameterize.

[matt-o-how]

Done

[arvidn]

Most of the suggestions I've made to simplify this was based on the assumption that we would use a separate function for full blocks.

The cost of making this function work both for mempool (i.e. single spend bundles) and full blocks is:

1. We would need to turn the spend bundle into a block before calling this function (in order to have a uniform interface). This function would then need to run said block generator function to get back to the individual spends. As it is now, we can skip that part.
2. We would need to support both pre- and post- hard fork (since blocks need to validate with the pre-hard fork rules). Transactions going into the mempool only need to be validated with the post-hard fork rules however.

[arvidn]

I think there are two separate changes that could be pulled out of this PR, as independent ones.

• porting get_flags_for_height_and_constants() to rust and exposing it to python. This is in itself a pretty siginifcant change as it moves the logic that's used to define soft-fork activation.
• Updating the API to BLS cache to take an iterator of pairs rather than two separate iterators

[arvidn]

Ideally, you would not collect() the items here. I think you can use .chain() to "append" the AGG_SIG_UNSAFE conditions to the end of the iterator

[matt-o-how]

Done

[arvidn]

I'm pretty sure you can leave this as a &[u8] and convert it directly to a Program below, with the into() conversion.

Suggested change

	let solution = Bytes::new(
	hex!(
	"ff\
	ff33\
	ffa02222222222222222222222222222222222222222222222222222222222222222\
	ff01\
	80\
	80"
	)
	.to_vec(),
	);
	let solution = hex!(
	"ff\
	ff33\
	ffa02222222222222222222222222222222222222222222222222222222222222222\
	ff01\
	80\
	80"
	);

[matt-o-how]

It needs the to_vec, but we can save the Bytes::new() with .into()

[arvidn]

I could be wrong, but it looks like this would be simpler to just:

Suggested change

	let result = validate_clvm_and_signature(
	&spend_bundle,
	TEST_CONSTANTS.max_block_cost_clvm,
	&TEST_CONSTANTS,
	TEST_CONSTANTS.hard_fork_height - 1,
	Arc::new(Mutex::new(BlsCache::default())),
	);
	if let Ok(_) = result {
	panic!("height too low!")
	};
	assert!_eq(validate_clvm_and_signature(
	&spend_bundle,
	TEST_CONSTANTS.max_block_cost_clvm,
	&TEST_CONSTANTS,
	TEST_CONSTANTS.hard_fork_height - 1,
	Arc::new(Mutex::new(BlsCache::default())),
	).unwrap_err(), <expected_error>);

[matt-o-how]

This test has been removed now anyway as we removed the fork height

[arvidn]

I think it's a little bit odd to make the aggregate_verify() member function return the cache misses.
On the python side, we construct an empty BLSCache run aggregate_verify() and then pick out the cached items. This is not the most efficient way of doing it, but it's simple and reuses the BLS cache as the "local stash" of the pairings.

Your approach is more efficient. For example, we know that every single key will be a cache miss, so there's not point in looking it up in the cache. However, when validating blocks, we don't really want to do the extra work of returning the cache misses. So your approach would work best if we decide to use different functions for validating SpendBundles and validating blocks. You could go the route of not using the BLS cache at all, in your patch. Instead, just implement a free function aggregate_verify() that doesn't use a cache but returns all the pairings it computed as a list.

This would improve performance some, and maybe simplify the two different code paths as well, since they don't need to share as much code.

In the long run, however, perhaps we would like to share the cache as well. I'm not sure though, maybe it's reasonable to aim for what's simplest now and figure out how to port the whole thread pool to rust when we get there/

[arvidn]

this is a very long line. Maybe this should be included from a binary file (or at least a separate file) instead. see !include_str() and !include_bytes() macros. I would like a comment to explain what it is also.

[matt-o-how]

Done

[arvidn]

this parameter should be removed. You're only dealing with transactions, i.e. mempool mode

[matt-o-how]

Done

[arvidn]

you need to export the get_flags_for_height_and_constants() as well. We need that function on the python side.

[matt-o-how]

Done

[arvidn]

I don't think this change would work. We still pass this type across the python process boundary, so it needs to be pickled.

[matt-o-how]

GTElement is a Rust pybinding in Python right? So passing it across should be straightforward. Or am I missing something?

[arvidn]

the way current python code is written, suggests that we have to manually convert objects into something that supports pickle.
looking at crates/chia_py_streamable_macro/src/lib.rs, it seems like we implement the pickle protocol in our bindings though. So, as far as I can tell, this ends up serializing the GTElement into bytes.
I don't believe we have any test coverage or round-trips of the pickle support though

[arvidn]

do we need to call this from python?
It would also need to be updated in chia_rs.pyi

[matt-o-how]

Yes. It is required to fix a bug with the fast-forwarding code

[arvidn]

if you combined these into a single expression, flags wouldn't have to be mut

[matt-o-how]

Done

[arvidn]

this file, npc_result.rs should probably be renamed. I believe the term "npc" is outdated

[matt-o-how]

Done

[arvidn]

something strange happened here. spend_bundle_validation.rs was removed and a brand new files spendbundle_validation.rs was added. It looks like you didn't use git mv to rename this file, or something else went wrong with git. It's complicated to review that this file was unchanged like this and I think the git-history of the file breaks as well.

[matt-o-how]

Should be fixed now