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

Round up thresholds #4455

Merged
merged 2 commits into from Feb 29, 2024
Merged

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Feb 29, 2024

@hstove found a bug when operating a single signer with a single key. Essentially the threshold became 0 which causes wsts to panic as it should due to an invalid threshold being provided. @xoloki subsequently pointed out that we actually had a bug in our threshold calculation due to integer arithmatic forcing us to always calculate a threshold "under" our desired amount whenever we had any remainder. This pr forces us to "round up" by adding 0.9 to the result. If any remainder exists, it will force our result over into the next integer amount (note that due to integer math and the nature of dividing by 10, the remainder will always be minimum 0.1 or more hence why adding 0.9 guarantees pushing it over to the next integer if remainder is present).

Due to Joey's suggestion, I have converted all integers involved to floating points instead to use the ceil function.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
xoloki
xoloki previously approved these changes Feb 29, 2024
Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

While I'd prefer a floating point based calculation that explicitly calls ceil or some similar function, this is certainly an improvement on what we currently have.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.42%. Comparing base (d063a74) to head (206eca6).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4455      +/-   ##
==========================================
- Coverage   83.46%   83.42%   -0.04%     
==========================================
  Files         448      448              
  Lines      323930   323930              
==========================================
- Hits       270352   270231     -121     
- Misses      53578    53699     +121     
Files Coverage Δ
stacks-signer/src/signer.rs 83.64% <100.00%> (ø)
testnet/stacks-node/src/nakamoto_node/miner.rs 83.48% <100.00%> (ø)

... and 21 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d063a74...206eca6. Read the comment docs.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@hstove
Copy link
Contributor

hstove commented Feb 29, 2024

My main concern is how this interacts with the Clarity code for voting on the aggregate key:

(if (>= (/ (* new-total u1000) total-weight) threshold-consensus)
;; Save this approved aggregate public key for this reward cycle.

If we round down in Clarity, and up in the signer, what happens?

@jferrant
Copy link
Collaborator Author

jferrant commented Feb 29, 2024

My main concern is how this interacts with the Clarity code for voting on the aggregate key:

(if (>= (/ (* new-total u1000) total-weight) threshold-consensus)
;; Save this approved aggregate public key for this reward cycle.

If we round down in Clarity, and up in the signer, what happens?

Nothing bad happens. It means clarity accepts a certain number of votes less than in the wsts calculation side. However, no one will get a result on the wsts side unless they reach the correct threshold. These are ultimately independent thresholds. Though its not a bad idea to make them consistent, the clarity one is arbitrary.

To make consistent have opened a clarity side PR change. These are indepenent code changes though as one does not effect the other.

#4457

@jferrant jferrant changed the title Round up thresholds using +0.9 Round up thresholds Feb 29, 2024
@jferrant jferrant requested a review from xoloki February 29, 2024 18:21
@hstove
Copy link
Contributor

hstove commented Feb 29, 2024

Ok, I think I follow. So basically, wsts and the signers are using this threshold to compute a specific threshold that relates to generating a valid signature for that public key, and then Clarity is using its own threshold to determine how many votes are needed to activate it as the aggregate key. That makes sense.

I can imagine some weird edge cases but nothing worth blocking this

Copy link
Collaborator

@xoloki xoloki 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 added this pull request to the merge queue Feb 29, 2024
Merged via the queue into next with commit 90e36df Feb 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When there is only 1 signer, stacks-signer crashes due to underflow in WSTS
4 participants