Skip to content
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

CHIA-1198 Add an end to end test for mempool bundle fill rate and the resulting block validation #18509

Merged

Conversation

AmineKhaldi
Copy link
Contributor

We try to create a mempool bundle with the maximum fill rate possible and ensure the resulting block would still pass block validation.

@AmineKhaldi AmineKhaldi added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels Aug 21, 2024
@AmineKhaldi AmineKhaldi self-assigned this Aug 21, 2024
@AmineKhaldi AmineKhaldi marked this pull request as ready for review August 21, 2024 13:23
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner August 21, 2024 13:23
@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch from e30d922 to f98698a Compare August 21, 2024 14:48
@AmineKhaldi AmineKhaldi requested a review from arvidn August 21, 2024 14:50
@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch 3 times, most recently from fb5d26e to 6db8957 Compare August 21, 2024 18:31
@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch from 6db8957 to 787fd39 Compare August 22, 2024 10:15
@AmineKhaldi AmineKhaldi requested a review from arvidn August 22, 2024 10:59
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. would you mind adding a comment to the custom cost limit you set, explaining why you pick that specific number?
Also, is there a way to assert the cost of the block as well? I expect it to be different from the transaction cost, but it would be good to have the test ensure it doesn't change

@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch from 787fd39 to 60b1c06 Compare August 22, 2024 12:26
@AmineKhaldi
Copy link
Contributor Author

looks good. would you mind adding a comment to the custom cost limit you set, explaining why you pick that specific number? Also, is there a way to assert the cost of the block as well? I expect it to be different from the transaction cost, but it would be good to have the test ensure it doesn't change

I added a comment to explain the custom cost limit.

As to getting the block cost, I issued a RequestBlock to get a FullBlock and check the cost in its TransactionsInfo.

@AmineKhaldi AmineKhaldi requested a review from arvidn August 22, 2024 12:28
@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch from 60b1c06 to 50f636b Compare August 22, 2024 13:44
@AmineKhaldi AmineKhaldi requested a review from arvidn August 22, 2024 13:46
@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch from 50f636b to 8f5dce3 Compare August 26, 2024 09:59
@AmineKhaldi AmineKhaldi requested a review from arvidn August 26, 2024 10:11
@AmineKhaldi AmineKhaldi force-pushed the fill_rate_block_validation_test branch from 8f5dce3 to 7664fc9 Compare August 26, 2024 16:28
@AmineKhaldi AmineKhaldi requested a review from arvidn August 26, 2024 16:29
@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Aug 27, 2024
@pmaslana pmaslana merged commit 9610e27 into Chia-Network:main Aug 27, 2024
370 checks passed
@altendky altendky mentioned this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants