Skip to content

Conversation

@rdeioris
Copy link
Contributor

@rdeioris rdeioris commented Sep 12, 2025

Description

This patch introduces monitoring and limiting for the size of a whole tenure. Note that tenure extend resets the counter.

For allowing more targeted tests, it introduces a config option for the miner (not exposed to the file based configuration) for enabling logging of skipped transactions (that are not logged or sent to the event system).

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@rdeioris rdeioris marked this pull request as ready for review September 16, 2025 13:11
@rdeioris rdeioris requested review from a team as code owners September 16, 2025 13:11
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Added some comments for improvements.

One thing I think it missing is an update to the code where the miner decides whether or not it should attempt to tenure extend, see stacks-node/src/nakamoto_node/miner.rs:1676

                // Do not extend if we have spent < 50% of the budget, since it is
                // not necessary.
                let usage = self
                    .tenure_budget
                    .proportion_largest_dimension(&self.tenure_cost);
                if usage < self.config.miner.tenure_extend_cost_threshold {
                    return Ok(NakamotoTenureInfo {
                        coinbase_tx: None,
                        tenure_change_tx: None,
                    });
                }

I think it should also attempt to extend if the size is >50% of the budget.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

I'm not seeing the code where the miner checks max_tenure_size to stop mining blocks?

@rdeioris
Copy link
Contributor Author

I'm not seeing the code where the miner checks max_tenure_size to stop mining blocks?

I did it for allowing smaller transactions to be included (given that the check is done at transaction inclusion). The only case I see for completely disabling block mining is if the current tenure size is so near to the limit that it is impossibile for any transaction to be included

@rdeioris
Copy link
Contributor Author

Added some comments for improvements.

One thing I think it missing is an update to the code where the miner decides whether or not it should attempt to tenure extend, see stacks-node/src/nakamoto_node/miner.rs:1676

                // Do not extend if we have spent < 50% of the budget, since it is
                // not necessary.
                let usage = self
                    .tenure_budget
                    .proportion_largest_dimension(&self.tenure_cost);
                if usage < self.config.miner.tenure_extend_cost_threshold {
                    return Ok(NakamotoTenureInfo {
                        coinbase_tx: None,
                        tenure_change_tx: None,
                    });
                }

I think it should also attempt to extend if the size is >50% of the budget.

Love it, working on it

@jcnelson
Copy link
Member

Looks like CI is still failing

@rdeioris
Copy link
Contributor Author

@jcnelson @brice-stacks i have removed the miner logic for automatic tenure extend based on tenure size. Before merging i think it is worthy to be sure that DEFAULT_MAX_TENURE_BYTES (currently 10 MB) is a solid value. (currently it has been choosen fully arbitrary)

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Just need answers to a couple of comments.

brice-stacks
brice-stacks previously approved these changes Oct 22, 2025
Copy link

@brice-stacks brice-stacks left a comment

Choose a reason for hiding this comment

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

LGTM!

auto-merge was automatically disabled October 23, 2025 05:10

Head branch was pushed to by a user without write access

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.

7 participants