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

When there is only 1 signer, stacks-signer crashes due to underflow in WSTS #4415

Closed
hstove opened this issue Feb 22, 2024 · 4 comments · Fixed by #4455
Closed

When there is only 1 signer, stacks-signer crashes due to underflow in WSTS #4415

hstove opened this issue Feb 22, 2024 · 4 comments · Fixed by #4455

Comments

@hstove
Copy link
Contributor

hstove commented Feb 22, 2024

I caught an issue when running the Nakamoto devnet locally. I only have 1 signer, and in that case threshold is set to 0, because 70% of 1 is 0 in integer terms. Same with dkg_threshold. This gets passed into WSTS, which generates a random number between 0 and threshold - 1, which is where the underflow happens.

I'll make an issue in WSTS, but adding here for visibility

Question: we can update WSTS to not underflow, but should threshold be 0 when there is 1 signer? Any other consequences we should think about?

@hstove hstove changed the title When there is only 1 signer, stacks-signer crashes due to underflow in threshold When there is only 1 signer, stacks-signer crashes due to underflow in WSTS Feb 22, 2024
@xoloki
Copy link
Collaborator

xoloki commented Feb 22, 2024

The threshold is a function of the number of reward slots/key_ids, not the number of signers. It's curious that a single signer would only have 1 reward slot.

What is the code that is doing the 70% calculation? It likely should be rounding up not down.

wsts should probably panic if given a threshold of 0, since it doesn't make any sense to continue at that point.

@hstove
Copy link
Contributor Author

hstove commented Feb 22, 2024

The threshold is a function of the number of reward slots/key_ids, not the number of signers. It's curious that a single signer would only have 1 reward slot.

Yeah, I can't say whether this is "correct" or not, but I see 1 slot taken by my 1 signer. For example, here is the result of me fetching /v2/stacker_set/$cycle (see weight = 1):

{"stacker_set":{"rewarded_addresses":[{"Standard":[{"version":26,"bytes":"ee9369fb719c0ba43ddf4d94638a970b84775f47"},"SerializeP2PKH"]}],"start_cycle_state":{"missed_reward_slots":[]},"signers":[{"signing_key":"03cd2cfdbd2ad9332828a7a13ef62cb999e063421c708e863a7ffed71fb61c88c9","stacked_amt":50160000000000,"weight":1}]}}

What is the code that is doing the 70% calculation? It likely should be rounding up not down.

In the top post I linked to a spot in stacks-signer, but the code is

let threshold = num_keys * 7 / 10;
let dkg_threshold = num_keys * 9 / 10;

Also I know that in our Clarity code, the same "round down" math is done to determine when an aggregate key has been approved via enough votes.

@xoloki
Copy link
Collaborator

xoloki commented Feb 22, 2024

We should definitely fix both the stacks-signer and clarity code to round up in those calculations.

@hstove hstove linked a pull request Feb 29, 2024 that will close this issue
@hstove
Copy link
Contributor Author

hstove commented Feb 29, 2024

Closing because #4455 merged

@hstove hstove closed this as completed Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants