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

[Nakamoto] getsortition.rs try_handle_request does not actually attempt to return the last winning sortition correctly #4938

Closed
jferrant opened this issue Jul 1, 2024 · 5 comments · Fixed by #4939

Comments

@jferrant
Copy link
Collaborator

jferrant commented Jul 1, 2024

This function should be returning the last sortition with the winning commit. However, note the if statement returns none for the last_sortition_ch when the fetched block snapshot does not contain a winning sortition. Later in the signer we attempt to use this non existent consensus hash to find the last winning soritition which will of course always fail. Update getsortition.rs to properly fetch the last winning sortition. This can either be fixed inside getsortition.rs or in the chainstate.
See the following relevant sections:

let (miner_pk_hash160, stacks_parent_ch, committed_block_hash, last_sortition_ch) = if !sortition_sn.sortition {

let last_sortition_ch = latest_state

@github-project-automation github-project-automation bot moved this to Status: 🆕 New in Stacks Core Eng Jul 1, 2024
@jferrant jferrant moved this from Status: 🆕 New to Status: 📋 Backlog in Stacks Core Eng Jul 1, 2024
@jcnelson
Copy link
Member

jcnelson commented Jul 1, 2024

I can take this

@jcnelson
Copy link
Member

jcnelson commented Jul 1, 2024

So, just a heads up, this is not what the node's API contract of this endpoint is. The API contract is to return data about a specific sortition, given one of a set of possible identifiers for it. If that sortition happens to be empty, then no data will be returned.

But, I can change the node to always return the consensus hash of the last sortition with a winner

jcnelson added a commit that referenced this issue Jul 1, 2024
@jcnelson jcnelson mentioned this issue Jul 1, 2024
@jcnelson
Copy link
Member

jcnelson commented Jul 1, 2024

Here you go. #4939

Can you add test coverage to this and verify that it does what the signer expects it to do?

jcnelson added a commit that referenced this issue Jul 1, 2024
@jferrant jferrant self-assigned this Jul 2, 2024
@jferrant
Copy link
Collaborator Author

jferrant commented Jul 2, 2024

Here you go. #4939

Can you add test coverage to this and verify that it does what the signer expects it to do?

Will do!

@jferrant jferrant moved this from Status: 📋 Backlog to Status: 💻 In Progress in Stacks Core Eng Jul 2, 2024
@saralab saralab moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Jul 2, 2024
@jferrant jferrant closed this as completed Jul 9, 2024
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: ✅ Done in Stacks Core Eng Jul 9, 2024
jcnelson added a commit that referenced this issue Jul 12, 2024
@blockstack-devops
Copy link
Contributor

This issue 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 Oct 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants