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, and porting the code to a new file called pot_iterations.rs.

There is a corresponding PR to import this code from here in chia-blockchain: Chia-Network/chia-blockchain#19197

crates/chia-protocol/src/block_record.rs

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

Pull Request Test Coverage Report for Build 13772159949

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

287 of 312 (91.99%) changed or added relevant lines in 4 files are covered.
350 unchanged lines in 16 files lost coverage.
Overall coverage increased (+0.02%) to 84.438%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/chia-protocol/src/block_record.rs	28	40	70.0%
crates/chia-protocol/src/pot_iterations.rs	231	244	94.67%

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-ssl/src/ca.rs	3	0.0%
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%
crates/chia-consensus/src/gen/conditions.rs	14	99.48%
wheel/src/api.rs	20	85.43%

Totals
Change from base Build 12797692864:	0.02%
Covered Lines:	14004
Relevant Lines:	16585

💛 - 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?

@matt-o-how what happened with this? Did you try it and it didn't work?

crates/chia-protocol/src/block_record.rs

crates/chia-protocol/src/pot_iterations.rs

wheel/generate_type_stubs.py

tests/test_pot_iterations.py

tests/test_block_record_fidelity.py

…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

crates/chia-protocol/src/pot_iterations.rs

arvidn · 2025-03-10T17:13:16Z

crates/chia-protocol/src/pos_quality.rs

+    // is necessary to store the entries in the plot.
+    // """
+
+    expected_plot_size(k).map_err(Into::into)


I thought clippy suggested Ok(call()?) over this. I think they do the same thing, as the ?-operator also implicitly converts the error type

arvidn · 2025-03-10T17:19:20Z

we should squash this when landing it

arvidn · 2025-03-10T19:56:46Z

crates/chia-protocol/src/pot_iterations.rs

+//         / ((1_u128 << 256) * plot_size as u128);
+
+//     Ok(iters.max(1) as u64)
+// }


what's the story with this? Is the TODO still relevant? If we don't need it, we should probably remove it

arvidn · 2025-03-10T19:58:15Z

tests/test_block_record_fidelity.py

@@ -129,3 +129,28 @@ def wrap_call(expr: str, br: Any) -> str:
        return f"V:{ret}"
    except Exception as e:
        return f"E:{e}"
+
+
+# TODO: more thoroughly check these new functions which use self


I don't quite get this comment. what does self refer to here? the BlockRecord?

arvidn · 2025-03-10T19:58:50Z

tests/test_pot_iterations.py

+
+from run_gen import DEFAULT_CONSTANTS
+
+# from chia.consensus.pos_quality import _expected_plot_size


we can probably remove these commented-out import lines now

arvidn · 2025-03-10T19:58:55Z

tests/test_pot_iterations.py

+    is_overflow_block,
+)
+
+# from chia.util.hash import std_hash


arvidn · 2025-03-10T19:59:42Z

tests/test_pot_iterations.py

are these tests copied from chia-blockchain?

arvidn · 2025-03-10T20:00:50Z

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]


@matt-o-how what happened with this? Did you try it and it didn't work?

arvidn · 2025-03-10T20:03:21Z

crates/chia-protocol/src/block_record.rs

    }

    fn sp_total_iters<'a>(
        &self,
        py: Python<'a>,
        constants: &Bound<'_, PyAny>,
    ) -> PyResult<Bound<'a, PyAny>> {
-        ChiaToPython::to_python(&self.sp_total_iters_impl(py, constants)?, py)
+        ChiaToPython::to_python(&self.sp_total_iters_impl(constants)?, py)


it's not clear to me why we need these extra wrapper functions. I would expect we could have pure rust member functions and (when enabled) python wrappers that call the rust functions. This appears to add 2 layers of python wrapper around the rust functions

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

crates/chia-protocol/src/block_record.rs Show resolved Hide resolved

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

crates/chia-protocol/src/pot_iterations.rs Outdated Show resolved Hide resolved

crates/chia-protocol/src/pot_iterations.rs Outdated Show resolved Hide resolved

matt-o-how added 5 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

specifically catch overflows and underflows in arithmetic

c43fe76

safety check div and mod

9e119c9

remove redundant OK(?) pattern

ff4cc17

arvidn previously approved these changes Mar 10, 2025

View reviewed changes

add pytest for expected_plot_size

14d101b

matt-o-how dismissed arvidn’s stale review via 14d101b March 10, 2025 17:43

matt-o-how added 3 commits March 10, 2025 17:53

revert pybinding setup

52d9069

black test_pot_iterations

7bc02db

use Ok(?) for expected_plot_size

1298d6c

arvidn reviewed Mar 10, 2025

View reviewed changes

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 •

edited

Loading

coveralls-official bot commented Jan 17, 2025 •

edited

Loading

Rigidity Jan 23, 2025

arvidn Mar 10, 2025

arvidn left a comment

arvidn Mar 10, 2025

arvidn commented Mar 10, 2025

arvidn Mar 10, 2025

arvidn Mar 10, 2025

arvidn Mar 10, 2025

arvidn Mar 10, 2025

arvidn Mar 10, 2025

matt-o-how Mar 10, 2025

arvidn Mar 10, 2025

arvidn Mar 10, 2025


		from run_gen import DEFAULT_CONSTANTS

		# from chia.consensus.pos_quality import _expected_plot_size

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 • edited Loading

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

Pull Request Test Coverage Report for Build 13772159949

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

arvidn commented Mar 10, 2025

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

matt-o-how commented Jan 17, 2025 •

edited

Loading

coveralls-official bot commented Jan 17, 2025 •

edited

Loading