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

Fix validation queue race condition in block approval vs validaiton submission #5497

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

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Nov 22, 2024

Closes #5496

…ew hasn't updated between validation sumbission and approval

Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant requested review from a team as code owners November 22, 2024 19:19
@jferrant jferrant marked this pull request as draft November 23, 2024 03:11
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.

This overall LGTM as long as CI passes.

@jferrant
Copy link
Collaborator Author

jferrant commented Nov 27, 2024

This overall LGTM as long as CI passes.

I decided to do a different approach. So unfortunately we expect signers to sign block proposals as valid that arrive BEFORE the new burn block as valid even if the new sortition view would make them invalid....would get rejected as we would have a new sortition view. I changed it to use a much more simplified height check that is very rudimentary. It should NOT ever be used to replace check_proposal or the validation endpoint but combined wiht the ValidateOk, I think is sufficient...

@jferrant jferrant requested a review from jcnelson November 27, 2024 02:43
@jferrant jferrant marked this pull request as ready for review November 27, 2024 02:43
@jferrant jferrant requested a review from a team as a code owner November 27, 2024 02:43
jcnelson
jcnelson previously approved these changes Dec 3, 2024
@jferrant
Copy link
Collaborator Author

jferrant commented Dec 3, 2024

This overall LGTM as long as CI passes.

I broke bitcoind_forking_test and I think partial_tenure_forking. I am actually wondering if this change has uncovered more easily the race condition from the node and the signer about approving blocks that should not be approved. The miner is consistently pushing blocks to me that are not the expected length. I might wait for the hotfix to go through before I try resolving these CI issues.

obycode
obycode previously approved these changes Dec 3, 2024
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 good, just two very minor comments.

stacks-signer/src/v0/signer.rs Outdated Show resolved Hide resolved
stacks-signer/src/v0/signer.rs Outdated Show resolved Hide resolved
@aldur aldur added this to the 3.1.0.0.2 milestone Dec 11, 2024
@jferrant jferrant dismissed stale reviews from obycode and jcnelson via 7519cdb December 18, 2024 23:54
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
@aldur aldur requested a review from obycode December 19, 2024 15:39
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! Lots of great cleanup in here too! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants