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

Feat/block rejections timeout heuristic #5731

Merged
merged 44 commits into from
Feb 3, 2025

Conversation

rdeioris
Copy link
Contributor

@rdeioris rdeioris commented Jan 22, 2025

Description

This patch modifies the logic of miner timeout while waiting for signer confirmations/rejections.

The current behaviour is to wait indefinitely until the threshold is reached (for both rejections and confirmations) or the burnchain tip changes or the block is found in the staging db.

The new behaviour adds heuristic for introducing a timeout (instead of waiting indefinitely and in addition to the threshold checks) that decreases based on the amount of rejections.

Note that if the initial timeout (the one set at the start of the cycle) expires without having rejections, the old behaviour of retrying from scratch is still valid. (eventually the other mechanisms, like the change of the burnchain tip will kick in as before)

The timeout steps can be configured from the config file:

[miner]
block_rejection_timeout_steps = { 0 = 100, 15 = 200 }

That configure the system to wait 100 seconds when there are 0% rejections and 200 when there are 15% rejections. The user is free to specifies more steps:

The default is:

(0, Duration::from_secs(600));
(10, Duration::from_secs(300));
(20, Duration::from_secs(150));
(30, Duration::from_secs(0));

Applicable issues

Additional info (benefits, drawbacks, caveats)

Note that the original code was set to trigger the conditional variable of the waiting cycle only when reaching the threshold (in confirmations or rejections). Now it is triggered at every update to move the logic of timeout in the block proposal final part ( get_block_status() )

The integration test is based on a test only atomic counter (added to the Counters struct) that gets updated with computed timeout value based on rejections.

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 changed the title Feat/block rejections heuristic Feat/block rejections timeout heuristic Jan 23, 2025
@aldur aldur added this to the 3.1.0.0.5 milestone Jan 23, 2025
@aldur aldur requested a review from kantai January 23, 2025 15:45
@rdeioris rdeioris marked this pull request as ready for review January 29, 2025 15:39
@rdeioris rdeioris requested review from a team as code owners January 29, 2025 15:39
@rdeioris
Copy link
Contributor Author

@obycode I have refactored the logic to keep the old behaviour and just check for change in the rejections amount. Combined with timeout checks i got no more stalls while waiting for the condvar

obycode
obycode previously approved these changes Jan 31, 2025
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.

Thanks!

Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Mainly requesting a change to the integration test, because you can simplify the code a lot.

Nice work on this! The miner-related code is very straightforward.

testnet/stacks-node/src/run_loop/neon.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/tests/signer/v0.rs Show resolved Hide resolved
testnet/stacks-node/src/tests/signer/v0.rs Show resolved Hide resolved
obycode
obycode previously approved these changes Jan 31, 2025
@rdeioris rdeioris requested a review from hstove February 1, 2025 10:11
obycode
obycode previously approved these changes Feb 1, 2025
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.

LGTM, just had a minor comment to help remove some type casting.

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.

👍

@kantai kantai added this pull request to the merge queue Feb 3, 2025
Merged via the queue into develop with commit 64bbb91 Feb 3, 2025
182 of 183 checks passed
@kantai kantai deleted the feat/block_rejections_heuristic branch February 3, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants