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

Spend down the block budget limit by x% every block #5450

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Nov 11, 2024

@hstove did substantial work on this PR as well so will need review from two others.
Closes #5433

@jferrant jferrant requested a review from a team as a code owner November 11, 2024 20:33
@jferrant
Copy link
Collaborator Author

@obycode also recommends making this a configurable, but also to make it a soft limit and to apply it after all other checks. Will move it to try_mine_tx_with_len in NakamotoBlockBuilder.

…ly it to Nakamoto tenures

Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant requested a review from a team as a code owner November 11, 2024 23:50
@jferrant
Copy link
Collaborator Author

jferrant commented Nov 11, 2024

@obycode also recommends making this a configurable, but also to make it a soft limit and to apply it after all other checks. Will move it to try_mine_tx_with_len in NakamotoBlockBuilder.

I tried to make this a soft limit but it seemed to require some pretty ugly messing around with the way we call try_mine_tx_with_len in select_and_apply_transactions and how we use that result before exiting our loop. Still working on fixing this. EDIT: think I got it working. Gotta fix CI and Tests.

Signed-off-by: Jacinta Ferrant <[email protected]>
jcnelson
jcnelson previously approved these changes Nov 12, 2024
@jferrant jferrant requested a review from hstove November 13, 2024 00:04
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant marked this pull request as draft November 13, 2024 23:30
@jferrant jferrant changed the title Spend down the block budget limit by 25% every block Spend down the block budget limit by x% every block Nov 13, 2024
clarity/src/vm/costs/mod.rs Outdated Show resolved Hide resolved
@jferrant jferrant marked this pull request as ready for review November 14, 2024 18:56
Signed-off-by: Jacinta Ferrant <[email protected]>
stackslib/src/chainstate/stacks/miner.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/tests/nakamoto_integrations.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/tests/signer/v0.rs Outdated Show resolved Hide resolved
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
@@ -536,7 +536,7 @@ impl NakamotoBlockBuilder {
.mempool_settings
.tenure_cost_limit_per_block_percentage
{
if percentage < 100 {
if (1..100).contains(&percentage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could avoid this check altogether if a percentage of 100 in the config becomes a None instead of Some(100).

Copy link
Collaborator Author

@jferrant jferrant Nov 15, 2024

Choose a reason for hiding this comment

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

True, but i kept this additional check in case someone wrote code directly instead of using the raw config file. i.e. we shouldn't attempt to increase the amount of block space nor set it to zero no matter what, I had assumed. But prob better to not fail silently anyway.. maybe convert this to an assert here in case there is a poorly written test? I.e. assert!((1..100).contains(percentage), "BUG: Invalid block budget percentage {percentage}") ?

testnet/stacks-node/src/config.rs Outdated Show resolved Hide resolved
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants