Remove chia-blockchain dependency for most of chia_rs #887

matt-o-how · 2025-01-17T15:04:40Z

This PR removes imports from test_blscache.py and deletes test_program_fidelity.py.

It also removes the python running from block_record.rs by implementing pot_iterations.py inside the class

arvidn · 2025-01-17T15:57:01Z

crates/chia-protocol/src/block_record.rs

this is a really nice change. However, I think it would be good to check the correctness of this against the existing python implementation. One way to do that would be to break this off into a separate PR that still relies on the chia-blockchain dependency to compare the result between the rust and python implementation. Can you think of another way to be confident in the correctness of this port?

coveralls-official · 2025-01-17T16:40:13Z

Pull Request Test Coverage Report for Build 13418635508

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

242 of 288 (84.03%) changed or added relevant lines in 4 files are covered.
333 unchanged lines in 14 files lost coverage.
Overall coverage decreased (-0.3%) to 84.085%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/chia-protocol/src/pot_iterations.rs	209	220	95.0%
crates/chia-protocol/src/block_record.rs	28	40	70.0%
crates/chia-protocol/src/pos_quality.rs	0	23	0.0%

Files with Coverage Reduction	New Missed Lines	%
crates/chia-protocol/src/lazy_node.rs	1	90.91%
crates/chia-puzzle-types/src/puzzles/singleton.rs	3	29.17%
crates/chia-puzzle-types/src/puzzles/standard.rs	3	76.32%
crates/chia-puzzle-types/src/puzzles/offer.rs	4	95.4%
crates/chia_streamable_macro/src/lib.rs	7	95.96%
wheel/src/run_program.rs	7	76.67%
crates/chia-consensus/src/fast_forward.rs	8	97.88%
wheel/src/api.rs	20	85.43%
crates/chia-bls/src/signature.rs	22	94.91%
crates/chia_py_streamable_macro/src/lib.rs	30	92.86%

Totals
Change from base Build 12797692864:	-0.3%
Covered Lines:	13742
Relevant Lines:	16343

💛 - Coveralls

…use new functions

Rigidity · 2025-01-23T21:32:39Z

crates/chia-protocol/src/pos_quality.rs

@@ -4,7 +4,9 @@
 pub const UI_ACTUAL_SPACE_CONSTANT_FACTOR: f32 = 0.78;

 // TODO: Update this when new plot format releases
-pub fn expected_plot_size(k: u32) -> u64 {
+#[cfg(feature = "py-bindings")]
+#[pyo3::pyfunction]


I think you have to do #[cfg_attr(feature = "py-bindings", pyo3::pyfunction)] instead?

arvidn · 2025-02-05T17:15:39Z

crates/chia-protocol/src/block_record.rs

-    fn sp_total_iters_impl(&self, py: Python<'_>, constants: &Bound<'_, PyAny>) -> PyResult<u128> {
-        self.sp_sub_slot_total_iters_impl(py, constants)?
-            .checked_add(self.sp_iters_impl(py, constants)? as u128)
+    fn sp_iters_impl(&self, constants: &Bound<'_, PyAny>) -> PyResult<u64> {


now that these are implemented in rust, I don't think they should be limited to just the python bindings anymore. I imagine you put them here to simplify error handling, so make them return PyResult<>. If you make them part of the rust type and make them return chia_error::Result<> you could map those errors with map_err()

arvidn · 2025-02-05T17:34:59Z

crates/chia-protocol/src/pot_iterations.rs

+
+#[cfg(feature = "py-bindings")]
+#[pyo3::pyfunction]
+pub fn calculate_ip_iters(


do you feel like the unit tests below are sufficient to be confident that this is correct and identical to the python implementation?
I'm especially worried about integer width overflow

arvidn · 2025-02-05T18:48:47Z

wheel/generate_type_stubs.py

+    num_sps_sub_slot: int,
+    num_sp_intervals_extra: int,
+    sub_slot_iters: int,
+    signage_point_index: int,
+    required_iters: int,
+) -> int: ...


are all of these plain int in the python implementation? it seems like it might make sense to make them sized ints

arvidn · 2025-02-05T18:50:52Z

tests/test_pot_iterations.py

+        )
+        assert sp_iters > ip_iters
+
+    # def test_win_percentage(self):


I think this commented-out code can be removed, right?

arvidn · 2025-02-05T18:51:55Z

tests/test_block_record_fidelity.py

+
+    br = get_block_record(rng, ssi=ssi, spi=13, ri=1)
+    res = br.ip_iters_impl(DEFAULT_CONSTANTS)
+    assert res is not None


not None, it seems like the wrong value would also pass here

Made this a specific value

…ython bindings

arvidn

I would suggest you have tests that attempt to overflow integers, and especially look at all as casts and see if there's a way to truncate integers, in tests

arvidn · 2025-02-19T13:33:42Z

crates/chia-protocol/src/pot_iterations.rs

+    num_sps_sub_slot: u32,
+    sub_slot_iters: u64,
+) -> pyo3::PyResult<u64> {
+    calculate_sp_interval_iters(num_sps_sub_slot, sub_slot_iters).map_err(Into::into)


I think you can use ? instead of explicitly mapping the error with Into::into.

i.e.

Ok(calculate_sp_interval_iters(num_sps_sub_slot, sub_slot_iters)?)

arvidn · 2025-02-19T13:34:21Z

crates/chia-protocol/src/pot_iterations.rs

+        return Err(Error::Custom(format!(
+            "Invalid sp iters {sp_iters} for this ssi {sub_slot_iters}"
+        )));


I really think we should have error codes instead of free-form strings. Handling errors like this, programatically, is very brittle.

matt-o-how added 3 commits January 17, 2025 14:10

remove imports from test_blscache and delete test_program_fidelity

fe7819a

remove imports from block_record by implement pot_iterations

315fab0

clippy and fmt

0d406e7

matt-o-how changed the title ~~Remove chia-blockchain dependency in tests~~ Remove chia-blockchain dependency for most of chia_rs Jan 17, 2025

matt-o-how added 2 commits January 17, 2025 15:39

don't export new functions

d27c1a5

black tests

19ca423

arvidn reviewed Jan 17, 2025

View reviewed changes

import PyValueError at top of file

264058d

matt-o-how added 7 commits January 22, 2025 10:58

add pot_iterations and pos_quality modules; refactor block_record to …

719b8a2

…use new functions

fix circular import

74a700e

temp commit

ef8c936

fix build with py-bindings

06cdaad

clippy and fmt

2b9d0f7

black format test

242c993

fix fetching rust values from python

b8c0305

Rigidity reviewed Jan 23, 2025

View reviewed changes

matt-o-how added 13 commits January 24, 2025 13:00

tests run but overflow

7d38538

correct order of blockrecord args

2bb5c1e

add tests to pot_iterations.rs

31bdbc7

fix bug in implementation

a1e9425

fix python test

6251c19

fmt and black

6645f85

add ip_iters pytest

fb5897e

clippy changes

44a04d3

update type stubs for new python functions

d752a28

fix stubs

d0d0883

add expected_plot_size to api.rs

b39c89c

black test

638943b

process signage_point_index as u32 to prevent overflows

4294ff4

matt-o-how added 2 commits January 27, 2025 15:45

pytests passing

68aca23

black test again

9f6f641

arvidn reviewed Feb 5, 2025

View reviewed changes

matt-o-how added 5 commits February 7, 2025 13:04

use sized ints in stub definition

5c1c692

Remove commented-out test

966daa4

make rust Result versions of the functions which are referred to in p…

8ff24bf

…ython bindings

fmt

48c44fe

fix export mod / build issues

b14d77b

arvidn reviewed Feb 19, 2025

View reviewed changes

matt-o-how added 2 commits February 19, 2025 17:22

make specific error type and unwrap with ?

c7cea31

flatten if as they now return the same error code

cace987

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove chia-blockchain dependency for most of chia_rs #887

Remove chia-blockchain dependency for most of chia_rs #887

matt-o-how commented Jan 17, 2025

arvidn Jan 17, 2025

coveralls-official bot commented Jan 17, 2025 •

edited

Loading

Rigidity Jan 23, 2025

arvidn Feb 5, 2025

matt-o-how Feb 17, 2025

arvidn Feb 5, 2025

arvidn Feb 5, 2025

matt-o-how Feb 17, 2025

arvidn Feb 5, 2025

matt-o-how Feb 17, 2025

arvidn Feb 5, 2025

matt-o-how Feb 17, 2025

arvidn left a comment

arvidn Feb 19, 2025

arvidn Feb 19, 2025

Remove chia-blockchain dependency for most of chia_rs #887

Are you sure you want to change the base?

Remove chia-blockchain dependency for most of chia_rs #887

Conversation

matt-o-how commented Jan 17, 2025

Choose a reason for hiding this comment

coveralls-official bot commented Jan 17, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13418635508

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

coveralls-official bot commented Jan 17, 2025 •

edited

Loading