Swap to ThreadPoolExecutor and use chia_rs version of validate_clvm_and_signature() #18213

matt-o-how · 2024-06-20T15:34:14Z

Purpose:

Migrate to using the chia_rs version of validate_clvm_and_signature and use ThreadPools for faster concurrent execution.
Until chia-rs is bumped to include the pyfunctions get_name_puzzle_conditions and validate_clvm_and_signature the tests for this will fail.

Current Behavior:

We currently use the Python version of validate_clvm_and_signature which forces us to use ProcessPools

New Behavior:

Use the chia_rs version of validate_clvm_and_signature and use ThreadPools for faster concurrent execution.

github-actions · 2024-07-16T12:25:41Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-07-16T15:03:01Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2024-08-08T07:59:08Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-08-08T14:20:20Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

arvidn · 2024-08-21T06:05:11Z

chia/_tests/core/mempool/test_mempool_manager.py

@@ -507,7 +507,7 @@ async def test_duplicate_output() -> None:
 async def test_block_cost_exceeds_max() -> None:
    mempool_manager = await instantiate_mempool_manager(zero_calls_get_coin_records)
    conditions = []
-    for i in range(2400):
+    for i in range(3800):


the main reason some numbers change in this test, as I understand it, is because we no longer turn transactions into "simple generators" before running them. This is more efficient and also more accurate now that (with the hard fork) we no longer pay cost for the generator ROM. So:

transactions are cheaper on the chain since the hard fork

the new logic reflects this cheaper reality

the test therefore will fit more transactions in a block

Hmm that doesn't seem to be correct.

With the inclusion of byte cost in Chia-Network/chia_rs#679 we now have a higher cost than what's currently in main, not lower.

This highlighted line for example goes down from needing 2400 items in main, to needing just 2389 (not 3800), due to cost becoming higher, and the expected cost for https://github.com/Chia-Network/chia-blockchain/pull/18213/files#diff-6a5a9dba08be65c628eff46226c4f7d83c6dbf4bcb9c15649c228b9d8b244c76R581 as another example goes up from main's 10268283 to 19144088, not down to 6600088.

With the inclusion of byte cost in Chia-Network/chia_rs#679 we now have a higher cost than what's currently in main, not lower.

I suspect this is a bug in the rust implementation. Although, as it turns out, the true reason these numbers had to change (so much) is because of the current issue with the rust implementation (not including size cost)

Sent a fix for that in Chia-Network/chia_rs#680 and adapted to the fix here with #18537.

chia/full_node/mempool_manager.py

chia/types/eligible_coin_spends.py

AmineKhaldi

Looks good. I left some suggestions in addition to Arvid's points.

The core.full_node group seems to have some failed tests as well.

chia/_tests/core/mempool/test_mempool_manager.py

chia/_tests/core/mempool/test_singleton_fast_forward.py

AmineKhaldi · 2024-08-21T10:39:05Z

chia/clvm/spend_sim.py

@@ -268,7 +268,6 @@ async def farm_block(
                    get_unspent_lineage_info_for_puzzle_hash=self.coin_store.get_unspent_lineage_info_for_puzzle_hash,
                    item_inclusion_filter=item_inclusion_filter,
                )
-


This seems to be an unintentional diff.

chia/full_node/mempool.py

chia/types/eligible_coin_spends.py

AmineKhaldi · 2024-08-21T10:46:40Z

chia/types/eligible_coin_spends.py

+        # generator = simple_solution_generator(new_sb)
+        assert mempool_item.conds is not None
+        try:
+            new_sbc_result = get_conditions_from_spendbundle(


I would name this new_conditions or new_conds to fit their use below, better.

AmineKhaldi · 2024-08-21T10:47:42Z

chia/types/eligible_coin_spends.py

@@ -354,4 +354,4 @@ async def process_fast_forward_spends(
        # change. Still, it's good form to update the spend bundle with the
        # new coin spends
        mempool_item.spend_bundle = new_sb
-        mempool_item.conds = new_npc_result.conds
+        mempool_item.conds = new_sbc_result


With a name like new_conditions or new_conds this reads better.

chia/types/eligible_coin_spends.py

arvidn · 2024-08-26T17:08:16Z

@matt-o-how there's a problem with the rust implementation. get_conditions_from_spendbundle() doesn't take the bytes size into account when returning the cost. It needs to do something like this: https://github.com/Chia-Network/chia_rs/blob/main/crates/chia-consensus/src/gen/run_block_generator.rs#L81-L84

    let byte_cost = (puzzle.len() + solution.len()) as u64 * constants.cost_per_byte;
    subtract_cost(a, &mut cost_left, byte_cost)?;

…rom full_node

Co-authored-by: Amine Khaldi <[email protected]>

… overhead into account, creating blocks that exceed the max size.

github-actions · 2024-09-05T14:18:03Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-10-21T11:05:58Z

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

matt-o-how added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jun 20, 2024

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 16, 2024

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 16, 2024

matt-o-how force-pushed the threadpoolexecutor branch from da137fa to 32547fb Compare August 5, 2024 12:38

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 8, 2024

matt-o-how force-pushed the threadpoolexecutor branch from e80e820 to 6f23c03 Compare August 8, 2024 14:20

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 8, 2024

matt-o-how marked this pull request as ready for review August 9, 2024 13:01

matt-o-how requested a review from a team as a code owner August 9, 2024 13:01

matt-o-how requested a review from arvidn August 9, 2024 13:01

matt-o-how force-pushed the threadpoolexecutor branch from 7ad3f4d to 22969f4 Compare August 13, 2024 14:42

arvidn closed this Aug 21, 2024

arvidn reopened this Aug 21, 2024

arvidn reviewed Aug 21, 2024

View reviewed changes

arvidn requested a review from AmineKhaldi August 21, 2024 09:45

AmineKhaldi reviewed Aug 21, 2024

View reviewed changes

arvidn reviewed Aug 21, 2024

View reviewed changes

chia/types/eligible_coin_spends.py Outdated Show resolved Hide resolved

matt-o-how force-pushed the threadpoolexecutor branch from c6407b9 to 73abaa5 Compare August 21, 2024 16:35

matt-o-how force-pushed the threadpoolexecutor branch from ccdbffa to 19f6cd6 Compare August 28, 2024 14:17

AmineKhaldi force-pushed the threadpoolexecutor branch 2 times, most recently from 7835275 to fb71e79 Compare September 3, 2024 18:46

matt-o-how added 4 commits September 4, 2024 17:02

swap to threadpool executor and chia_rs validate_clvm_and_signature

454009b

flake8 and black

43c9f25

isort

0c2c3ae

remove mp_context

35eb491

matt-o-how and others added 25 commits September 4, 2024 17:02

adjust cost so ttest passes

72fe562

fix fastforward with rust get_npc() call

1c5a4b1

remove unused imports

110067a

import Err

a9a7c67

swap get_name_puzzle_conditions to get_conditions_from_spendbundle

f506ac6

Fix for latest rust version

af0f8c0

fix fastforwards

91a0f20

isort

9acc339

no longer pass in new_spend_bytes to pre_validate_spendbundle

d29b162

fix benchmarks for new pre_validate_spendbundle params

44cd65a

checks e.args[0] exists before accessing it

c8479b1

fix truncated comment

cbb3917

lint fixes

c96017a

return Err.Unknown

62e7542

remove multiprocessing_context

9eb640f

remove oversight

7bbf6a4

remove initialisation of multiprocessing_context in mempool_manager f…

a8e771e

…rom full_node

remove unused import

7f109bb

Use underscore for test

71f3852

Co-authored-by: Amine Khaldi <[email protected]>

Use underscore for large test int

7f73e8c

Co-authored-by: Amine Khaldi <[email protected]>

remove commented out code

a47454f

Co-authored-by: Amine Khaldi <[email protected]>

rename new_sbc for readability

1298416

Use more descriptive comment

791bed3

Co-authored-by: Amine Khaldi <[email protected]>

remove imprecise check from test

fd5692d

Co-authored-by: Amine Khaldi <[email protected]>

add temporary work-around for mempool block creation not taking block…

c53d78b

… overhead into account, creating blocks that exceed the max size.

AmineKhaldi force-pushed the threadpoolexecutor branch from fb71e79 to c53d78b Compare September 4, 2024 16:05

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 5, 2024

github-actions bot added the stale-pr Flagged as stale and in need of manual review label Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap to ThreadPoolExecutor and use chia_rs version of validate_clvm_and_signature() #18213

Swap to ThreadPoolExecutor and use chia_rs version of validate_clvm_and_signature() #18213

matt-o-how commented Jun 20, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Aug 8, 2024

github-actions bot commented Aug 8, 2024

arvidn Aug 21, 2024

matt-o-how Aug 21, 2024

AmineKhaldi Aug 28, 2024

arvidn Aug 28, 2024

AmineKhaldi Aug 29, 2024

AmineKhaldi left a comment •

edited

Loading

AmineKhaldi Aug 21, 2024

AmineKhaldi Aug 21, 2024

matt-o-how Aug 21, 2024

AmineKhaldi Aug 21, 2024

matt-o-how Aug 21, 2024

arvidn commented Aug 26, 2024

github-actions bot commented Sep 5, 2024

github-actions bot commented Oct 21, 2024

Swap to ThreadPoolExecutor and use chia_rs version of validate_clvm_and_signature() #18213

Are you sure you want to change the base?

Swap to ThreadPoolExecutor and use chia_rs version of validate_clvm_and_signature() #18213

Conversation

matt-o-how commented Jun 20, 2024 • edited Loading

Purpose:

Current Behavior:

New Behavior:

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Aug 8, 2024

github-actions bot commented Aug 8, 2024

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

AmineKhaldi left a comment • 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

arvidn commented Aug 26, 2024

github-actions bot commented Sep 5, 2024

github-actions bot commented Oct 21, 2024

matt-o-how commented Jun 20, 2024 •

edited

Loading

AmineKhaldi left a comment •

edited

Loading