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: prevent multiple block proposal evals #5453

Merged
merged 28 commits into from
Jan 15, 2025

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Nov 12, 2024

This PR adds a new table to SignerDB, which is used to track block proposal validation submissions that received a 429 (which happens when the node is already evaluating a proposal).

When a block validation response is received, the signer checks if there are any pending block proposals. If so, it submits the proposal for validation.

In a "one-node-many-signers" scenario, such as many of our integration tests, this doesn't result in the same proposal being submitted many times. This is because, when a block validation response is received by the signer, the signer checks if that block was marked as "pending" and removes it from the DB.

Note: opening in draft while I write an integration test

Copy link
Collaborator

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

Just the one comment really.

Also I apologize in advance as this will be not fun to test..

@aldur aldur added this to the 3.1.0.0.2 milestone Dec 11, 2024
@hstove hstove marked this pull request as ready for review December 18, 2024 17:41
@hstove hstove requested review from a team as code owners December 18, 2024 17:41
@hstove hstove requested review from jferrant and obycode December 18, 2024 18:04
@hstove hstove requested a review from kantai December 20, 2024 15:50
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.

Looks great! Just noticed one potential optimization. Nice test!

jferrant
jferrant previously approved these changes Jan 13, 2025
Copy link
Collaborator

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

I think there are a couple of unused functions but LGTM

@hstove
Copy link
Contributor Author

hstove commented Jan 14, 2025

I'm going to try out this new integration test in develop and see if it does replicate the empty tenure behavior.

@hstove hstove requested a review from jferrant January 14, 2025 15:42
obycode
obycode previously approved these changes Jan 14, 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!

jferrant
jferrant previously approved these changes Jan 14, 2025
@hstove
Copy link
Contributor Author

hstove commented Jan 14, 2025

Good news - I've copy/pasted new_tenure_while_validating_previous_scenario into develop, and it fails as we'd expect it to! The failure happens from the test expecting signers to explicitly reject a block, which doesn't happen in develop.

@hstove hstove dismissed stale reviews from jferrant and obycode via 4d44a6d January 14, 2025 17:59
@hstove hstove requested a review from a team as a code owner January 14, 2025 17:59
@hstove hstove requested review from jferrant and obycode January 14, 2025 18:10
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.

🙌

@hstove hstove dismissed aldur’s stale review January 14, 2025 18:23

Integration test added

@hstove hstove added this pull request to the merge queue Jan 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 14, 2025
@obycode
Copy link
Contributor

obycode commented Jan 14, 2025

@wileyj I've seen this ☝️ on a couple different PRs today. Any idea why that's happening?

@jferrant jferrant added this pull request to the merge queue Jan 15, 2025
Merged via the queue into develop with commit 08f65e1 Jan 15, 2025
163 checks passed
@hstove hstove deleted the feat/retry-pending-block-proposals branch January 15, 2025 04:49
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Jan 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
7 participants