-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Swap to ThreadPoolExecutor and use chia_rs version of validate_clvm_and_signature() #18213
Open
matt-o-how
wants to merge
32
commits into
main
Choose a base branch
from
threadpoolexecutor
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
454009b
swap to threadpool executor and chia_rs validate_clvm_and_signature
matt-o-how 43c9f25
flake8 and black
matt-o-how 0c2c3ae
isort
matt-o-how 35eb491
remove mp_context
matt-o-how 03318dd
convert TypeError into ValidationError
matt-o-how 7aed44e
increase amount for test of exceed cost limit
matt-o-how 91057d3
change hard coded expected costs
matt-o-how 72fe562
adjust cost so ttest passes
matt-o-how 1c5a4b1
fix fastforward with rust get_npc() call
matt-o-how 110067a
remove unused imports
matt-o-how a9a7c67
import Err
matt-o-how f506ac6
swap get_name_puzzle_conditions to get_conditions_from_spendbundle
matt-o-how af0f8c0
Fix for latest rust version
matt-o-how 91a0f20
fix fastforwards
matt-o-how 9acc339
isort
matt-o-how d29b162
no longer pass in new_spend_bytes to pre_validate_spendbundle
matt-o-how 44cd65a
fix benchmarks for new pre_validate_spendbundle params
matt-o-how c8479b1
checks e.args[0] exists before accessing it
matt-o-how cbb3917
fix truncated comment
matt-o-how c96017a
lint fixes
matt-o-how 62e7542
return Err.Unknown
matt-o-how 9eb640f
remove multiprocessing_context
matt-o-how 7bbf6a4
remove oversight
matt-o-how a8e771e
remove initialisation of multiprocessing_context in mempool_manager f…
matt-o-how 7f109bb
remove unused import
matt-o-how 71f3852
Use underscore for test
matt-o-how 7f73e8c
Use underscore for large test int
matt-o-how a47454f
remove commented out code
matt-o-how 1298416
rename new_sbc for readability
matt-o-how 791bed3
Use more descriptive comment
matt-o-how fd5692d
remove imprecise check from test
matt-o-how c53d78b
add temporary work-around for mempool block creation not taking block…
arvidn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be an unintentional diff. |
||
if result is not None: | ||
bundle, additions = result | ||
generator_bundle = bundle | ||
|
@@ -337,7 +336,7 @@ def __init__(self, service: SpendSim) -> None: | |
async def push_tx(self, spend_bundle: SpendBundle) -> Tuple[MempoolInclusionStatus, Optional[Err]]: | ||
try: | ||
spend_bundle_id = spend_bundle.name() | ||
sbc = await self.service.mempool_manager.pre_validate_spendbundle(spend_bundle, None, spend_bundle_id) | ||
sbc = await self.service.mempool_manager.pre_validate_spendbundle(spend_bundle, spend_bundle_id) | ||
except ValidationError as e: | ||
return MempoolInclusionStatus.FAILED, e.code | ||
assert self.service.mempool_manager.peak is not None | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 just2389
(not3800
), 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's10268283
to19144088
, not down to6600088
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent a fix for that in Chia-Network/chia_rs#680 and adapted to the fix here with #18537.