Extend the BLSCache to add in a rust Pybinding for validate_clvm_and_signature for use in the mempool #637

matt-o-how · 2024-07-31T14:56:09Z

No description provided.

coveralls-official · 2024-07-31T15:27:39Z

Pull Request Test Coverage Report for Build 10267356686

Details

411 of 443 (92.78%) changed or added relevant lines in 4 files are covered.
288 unchanged lines in 11 files lost coverage.
Overall coverage decreased (-1.6%) to 81.361%

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	322	323	99.69%
crates/chia-bls/src/bls_cache.rs	15	23	65.22%
wheel/src/api.rs	1	23	4.35%

Files with Coverage Reduction	New Missed Lines	%
crates/chia-protocol/src/classgroup.rs	1	0.0%
crates/chia-bls/src/error.rs	4	0.0%
crates/chia-bls/src/parse_hex.rs	6	70.0%
crates/chia-traits/src/streamable.rs	9	92.54%
crates/chia-bls/src/secret_key.rs	11	88.22%
crates/chia-bls/src/signature.rs	19	93.94%
crates/chia-bls/src/public_key.rs	19	89.9%
wheel/src/api.rs	44	60.37%
crates/chia-bls/src/gtelement.rs	46	25.93%
crates/chia-bls/src/bls_cache.rs	46	71.88%

Totals
Change from base Build 10265410447:	-1.6%
Covered Lines:	11908
Relevant Lines:	14636

💛 - Coveralls

arvidn · 2024-08-02T09:35:12Z

crates/chia-bls/src/bls_cache.rs

@@ -122,9 +129,9 @@ impl BlsCache {
        let msgs = msgs
            .iter()?
            .map(|item| item?.extract())
-            .collect::<PyResult<Vec<PyBackedBytes>>>()?;


what's the rationale for making this change?

I've reverted it back now.

arvidn · 2024-08-02T09:37:32Z

crates/chia-bls/src/bls_cache.rs

-                PyBytes::new_bound(py, key),
-                PyBytes::new_bound(py, &value.to_bytes()),
-            ))?;
+            ret.append((PyBytes::new_bound(py, key), value.clone().into_py(py)))?;


we talked about going back to using plan bytes here, since GTElement doesn't support pickle. (maybe it should in the future, but let's make that a separate PR)

arvidn · 2024-08-02T09:40:07Z

crates/chia-bls/src/bls_cache.rs

@@ -64,7 +71,7 @@ impl BlsCache {

            // Otherwise, we need to calculate the pairing and add it to the cache.
            let mut aug_msg = pk.borrow().to_bytes().to_vec();
-            aug_msg.extend_from_slice(msg.as_ref());


I would expect extend_from_slice() to be more efficient. Is there a good reason to make this change?

arvidn · 2024-08-02T09:45:22Z

wheel/generate_type_stubs.py

+    max_cost: int,
+    constants: ConsensusConstants,
+    peak_height: int,
+    cache: Option[BLSCache],


I don't think you should pass in the cache here. If we want to make that change, I think it should be considered independently of porting this to rust

arvidn · 2024-08-02T09:46:39Z

wheel/generate_type_stubs.py

+def get_name_puzzle_conditions(
+    spend_bundle: SpendBundle,
+    max_cost: int,
+    constants: ConsensusConstants,
+    height: int,
+) -> SpendBundleConditions: ...


It doesn't look like this function is defined in this PR. Did you mean get_conditions_from_spendbundle()?
It would be good to understand why this wasn't caught by CI as well. Perhaps there's no python test for this function, or we don't run mypy

Fixed the immediate problem, but I agree the testing for the pyi file is lacking

Manually generating this file is I hope only a temporary solution

Pyo3 is working on automatic type stub generation (though I don't know how we'll do our custom integer types)

we do run mypy, so I would have expected this to be caught if there was a test attempting to use this function (under either this name or the true name)

arvidn · 2024-08-02T09:50:33Z

crates/chia-consensus/src/spendbundle_validation.rs

+    max_cost: u64,
+    constants: &ConsensusConstants,
+    height: u32,
+    cache: &Arc<Mutex<BlsCache>>,


I don't think you should pass in the cache here

arvidn · 2024-08-02T09:57:14Z

tests/test_blscache.py

    for key, value in cached_bls.items():
        assert isinstance(key, bytes)
-        assert isinstance(value, bytes)
+        assert isinstance(value, GTElement)


we agreed to these as bytes, right?

crates/chia-consensus/src/gen/condition_tools.rs

arvidn · 2024-08-02T10:44:30Z

crates/chia-consensus/src/spendbundle_validation.rs

+    use std::sync::Arc;
+
+    #[test]
+    fn test_validate_no_pks() {


there seems to be a lot of repetitions across these tests. did you try to factor something out or make it a parameterized test?

crates/chia-consensus/src/spendbundle_validation.rs

wheel/src/api.rs

arvidn · 2024-08-02T11:25:11Z

crates/chia-consensus/src/gen/condition_tools.rs

+    result
+}
+
+pub fn u64_to_bytes(val: u64) -> Bytes {


this seems to be an internal function that probably shouldn't be public/exported

It's used in spendbundle_validation.rs's tests and I suspect will be useful in more places as the Rust ports continue

it should probably be pub(crate) then

arvidn

I think you should revert crates/chia-bls/src/bls_cache.rs, crates/chia-bls/benches/cache.rs, crates/chia-bls/src/lib.rs.

You can define PairingInfo in wheel/src/api.rs if you like a name for it.

arvidn · 2024-08-05T15:06:54Z

crates/chia-consensus/src/spendbundle_validation.rs

+    if !result {
+        return Err(ErrorCode::BadAggregateSignature);
+    }
+    Ok((npcresult, added, start_time.elapsed()))


Suggested change

Ok((npcresult, added, start_time.elapsed()))

Ok((npcresult, cache.items(), start_time.elapsed()))

This change requires you to add the items() function to the cache (which currently only exists in the python binding). Alternatively (but maybe a bigger change) would be to replace the call to aggregate_verify() with just a loop over computing the pairings, and return those. There's already an aggregate_verify_gt() for the signature validation

arvidn · 2024-08-06T08:34:58Z

crates/chia-consensus/src/gen/condition_tools.rs

+        #[case] parent_id: Vec<u8>,
+        #[case] puzzle_hash: Vec<u8>,


every test case uses the same values for parent_id and puzzle_hash. I don't think you need to make these parameters to the test.

I think you could make msg a constant in the tests as well, since you don't need to test different values of it (I think).

arvidn · 2024-08-06T08:36:01Z

crates/chia-consensus/src/gen/condition_tools.rs

+    result
+}
+
+pub(crate) fn u64_to_bytes(val: u64) -> Bytes {


we already have two copies of this exact function (with this name) in the repository. The other copies are only available to tests however. So I still think it would make sense to make this private (not public) and use one of the existing copies for the test.

#645

arvidn · 2024-08-06T08:42:00Z

crates/chia-consensus/src/gen/condition_tools.rs

+        match opcode {
+            AGG_SIG_PARENT => {
+                expected_result.extend(parent_id.as_slice());
+                expected_result.extend(TEST_CONSTANTS.agg_sig_parent_additional_data.as_slice());
+            }
+            AGG_SIG_PUZZLE => {
+                expected_result.extend(puzzle_hash.as_slice());
+                expected_result.extend(TEST_CONSTANTS.agg_sig_puzzle_additional_data.as_slice());
+            }
+            AGG_SIG_AMOUNT => {
+                expected_result.extend(u64_to_bytes(coin_amount).as_slice());
+                expected_result.extend(TEST_CONSTANTS.agg_sig_amount_additional_data.as_slice());
+            }
+            AGG_SIG_PUZZLE_AMOUNT => {
+                expected_result.extend(puzzle_hash.as_slice());
+                expected_result.extend(u64_to_bytes(coin_amount).as_slice());
+                expected_result.extend(
+                    TEST_CONSTANTS
+                        .agg_sig_puzzle_amount_additional_data
+                        .as_slice(),
+                );
+            }
+            AGG_SIG_PARENT_AMOUNT => {
+                expected_result.extend(parent_id.as_slice());
+                expected_result.extend(u64_to_bytes(coin_amount).as_slice());
+                expected_result.extend(
+                    TEST_CONSTANTS
+                        .agg_sig_parent_amount_additional_data
+                        .as_slice(),
+                );
+            }
+            AGG_SIG_PARENT_PUZZLE => {
+                expected_result.extend(parent_id.as_slice());
+                expected_result.extend(puzzle_hash.as_slice());
+                expected_result.extend(
+                    TEST_CONSTANTS
+                        .agg_sig_parent_puzzle_additional_data
+                        .as_slice(),
+                );
+            }
+            AGG_SIG_ME => {
+                expected_result.extend(coin.coin_id().as_slice());
+                expected_result.extend(TEST_CONSTANTS.agg_sig_me_additional_data.as_slice());
+            }
+            _ => {}
+        };


rather than computing the expected value programatically, I think it would be easier to read this test (and be confident it's correct) if the expected output was a parameter instead. Since these are binary strings, it could be simplified by either altering the test constants to make the additional data ascii, or to change the messages to be hex and then specify the expected output as hex. Given that the amount will be binary either way, that might be the better option

…Conditions (to enable reusing existing test facilities). Extend the tests to cover the block-generator test cases

matt-o-how requested a review from arvidn August 1, 2024 14:33

arvidn requested changes Aug 2, 2024

View reviewed changes

arvidn reviewed Aug 2, 2024

View reviewed changes

wheel/src/api.rs Outdated Show resolved Hide resolved

arvidn reviewed Aug 2, 2024

View reviewed changes

matt-o-how force-pushed the validate_clvm_and_sigs branch 2 times, most recently from ef93e62 to ba7da79 Compare August 5, 2024 14:28

arvidn reviewed Aug 5, 2024

View reviewed changes

matt-o-how force-pushed the validate_clvm_and_sigs branch from 5893e16 to a81e3d0 Compare August 5, 2024 15:13

arvidn reviewed Aug 6, 2024

View reviewed changes

matt-o-how and others added 18 commits August 6, 2024 12:16

use uncached tree_hash()

fcb3288

fmt

0a986d0

add test for get_conditions_from_spendbundle()

279f60e

make get_conditions_from_spendbundle() return a non-owned SpendBundle…

81a3ca6

…Conditions (to enable reusing existing test facilities). Extend the tests to cover the block-generator test cases

intitial commit

a8638bb

cargo fmt

4e1cd22

add a pairinginfo type to make clippy happy

696da41

fmt

da62f2a

black tests

1efd030

use pairinginfo type in pybinding

75f7d96

pybindings clippy changes

575ff63

clippy cache improvements

e0ae829

fmt again

9904e06

blscache.items() returns bytes, blscache.update() takes bytes

8a3c9d5

fix pystubs

ed15954

rename comment in api

b7220d7

undo unjustified changes

327df7c

move PyBackedBytes import

363f506

matt-o-how added 5 commits August 6, 2024 12:20

fix rebase detritus

b2aa033

fmt

e85505e

convert newly returned SBC to OSBC

79f73d7

change u64_to_bytes to pub(crate)

1fdc4d1

rebase cleanup

ffb68ce

matt-o-how force-pushed the validate_clvm_and_sigs branch from 39d2d85 to ffb68ce Compare August 6, 2024 11:31

matt-o-how added 2 commits August 6, 2024 13:27

fmt again

88e4af3

remove parent, puzzle, and msg as defined cases in aggsig test

faa0f58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the BLSCache to add in a rust Pybinding for validate_clvm_and_signature for use in the mempool #637

Extend the BLSCache to add in a rust Pybinding for validate_clvm_and_signature for use in the mempool #637

matt-o-how commented Jul 31, 2024

coveralls-official bot commented Jul 31, 2024 •

edited

Loading

arvidn Aug 2, 2024

matt-o-how Aug 2, 2024 •

edited

Loading

arvidn Aug 2, 2024

matt-o-how Aug 2, 2024

arvidn Aug 2, 2024

matt-o-how Aug 2, 2024

arvidn Aug 2, 2024

arvidn Aug 2, 2024

matt-o-how Aug 2, 2024

Rigidity Aug 2, 2024

arvidn Aug 2, 2024

arvidn Aug 2, 2024

arvidn Aug 2, 2024

matt-o-how Aug 2, 2024

arvidn Aug 2, 2024

arvidn Aug 2, 2024

matt-o-how Aug 2, 2024

arvidn Aug 5, 2024

matt-o-how Aug 5, 2024

arvidn left a comment

arvidn Aug 5, 2024

arvidn Aug 6, 2024

matt-o-how Aug 6, 2024

arvidn Aug 6, 2024

arvidn Aug 6, 2024

	Ok((npcresult, added, start_time.elapsed()))
	Ok((npcresult, cache.items(), start_time.elapsed()))

Extend the BLSCache to add in a rust Pybinding for validate_clvm_and_signature for use in the mempool #637

Are you sure you want to change the base?

Extend the BLSCache to add in a rust Pybinding for validate_clvm_and_signature for use in the mempool #637

Conversation

matt-o-how commented Jul 31, 2024

coveralls-official bot commented Jul 31, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10267356686

Details

💛 - Coveralls

Choose a reason for hiding this comment

matt-o-how Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls-official bot commented Jul 31, 2024 •

edited

Loading

matt-o-how Aug 2, 2024 •

edited

Loading