Accept out-of-order votes #4327

friedger · 2024-02-01T15:57:34Z

Description

Currently, .signer-voting rejects votes that are not for the latest round.

This PR removes this restriction. Votes out of round are accepted.

Applicable issues

fixes [clarity] .signer-voting need to accept out of round votes #4322

Additional info (benefits, drawbacks, caveats)

Checklist

Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs

setzeus

Logic change that removes asserting that rounds-check / out-of-order-votes LGTM.

Only have one NIT re: error messages.

Note that we have an consistency in formatting. In pox-4 the error constants are stored as an Int, in this contract, the error constants are stored as (err uint).

No preference for either format but preferable we stick to a single syntax.

MarvinJanssen

Looks fine to me. The spec requires out-of-order votes?

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs

codecov · 2024-02-02T22:53:03Z

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (2d4d9bd) 72.17% compared to head (1dfc1da) 54.77%.
Report is 3 commits behind head on next.

Files	Patch %	Lines
...src/chainstate/stacks/boot/signers_voting_tests.rs	0.00%	34 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             next    #4327       +/-   ##
===========================================
- Coverage   72.17%   54.77%   -17.40%     
===========================================
  Files         445      445               
  Lines      317208   317198       -10     
===========================================
- Hits       228945   173757    -55188     
- Misses      88263   143441    +55178

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

friedger · 2024-02-05T08:50:12Z

@MarvinJanssen Yes, as discussed with @kantai in https://nakamotoslack.slack.com/archives/C067BHSN1SL/p1706799771752509?thread_ts=1706690349.397339&cid=C067BHSN1SL

Are you read to approve this PR?

stackslib/src/chainstate/stacks/boot/mod.rs

jcnelson

Putting a hold on this for the time being; it's not ready to merge. Will update with more details later.

…rk/stacks-core into fix/pox-4-vote-out-of-order

hstove

Other than a few nits, I think this test should evaluate the contract state after the TX's are confirmed (via read-only calls)

hstove · 2024-02-07T15:00:46Z

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs


    // first vote should succeed
-    let tx1 = &receipts[receipts.len() - 2];
+    let tx1 = &receipts[receipts.len() - 3];


IMO it's better to use explicit nonces to avoid these kind of off-by-one errors

hstove · 2024-02-07T15:02:23Z

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs

@@ -350,7 +359,7 @@ fn vote_for_aggregate_public_key_in_last_block() {
    );

    // second vote should fail with duplicate vote error
-    let tx2 = &receipts[receipts.len() - 1];
+    let tx2 = &receipts[receipts.len() - 2];


Also better to use explicit names for the tx instead of tx2. I'd have to read too much into the test just to find out what tx this is testing

hstove · 2024-02-07T15:03:15Z

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs

+        Value::Response(ResponseData {
+            committed: true,
+            data: Box::new(Value::Bool(true))
+        })


Suggested change

Value::Response(ResponseData {

committed: true,

data: Box::new(Value::Bool(true))

})

Value::okay_true()

Also it's not this PR's code but Value::error(Value::UInt(10002)).unwrap() for errors

…rk/stacks-core into fix/pox-4-vote-out-of-order

obycode · 2024-02-12T15:12:48Z

These changes are now included in #4360.

blockstack-devops · 2024-11-05T00:22:06Z

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.

fix: accept votes for out of round

37d1751

friedger changed the base branch from master to next February 1, 2024 15:57

friedger requested review from jferrant, setzeus and MarvinJanssen February 1, 2024 15:57

friedger commented Feb 1, 2024

View reviewed changes

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs Outdated Show resolved Hide resolved

setzeus approved these changes Feb 1, 2024

View reviewed changes

MarvinJanssen reviewed Feb 2, 2024

View reviewed changes

stackslib/src/chainstate/stacks/boot/signers_voting_tests.rs Outdated Show resolved Hide resolved

saralab requested a review from jcnelson February 2, 2024 14:57

Merge branch 'next' into fix/pox-4-vote-out-of-order

56ce2f7

friedger added 2 commits February 3, 2024 00:01

chore: better public keys, fix typos

5d8309e

Merge branch 'next' into fix/pox-4-vote-out-of-order

f7390ea

jcnelson reviewed Feb 5, 2024

View reviewed changes

stackslib/src/chainstate/stacks/boot/mod.rs Outdated Show resolved Hide resolved

jcnelson self-requested a review February 5, 2024 18:43

jcnelson requested changes Feb 5, 2024

View reviewed changes

friedger added 3 commits February 6, 2024 11:25

fix: use Point for aggregate public key

214be53

Merge branch 'fix/pox-4-vote-out-of-order' of github.com:stacks-netwo…

4d75f91

…rk/stacks-core into fix/pox-4-vote-out-of-order

Merge branch 'next' into fix/pox-4-vote-out-of-order

cd830eb

hstove requested changes Feb 7, 2024

View reviewed changes

friedger added 3 commits February 8, 2024 16:11

chore: improve naming and comments

a70cfa5

Merge branch 'fix/pox-4-vote-out-of-order' of github.com:stacks-netwo…

680b343

…rk/stacks-core into fix/pox-4-vote-out-of-order

Merge branch 'next' into fix/pox-4-vote-out-of-order

1dfc1da

obycode mentioned this pull request Feb 9, 2024

Process successful .signers-voting results #4360

Merged

5 tasks

obycode closed this Feb 12, 2024

blockstack-devops added the locked label Nov 5, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept out-of-order votes #4327

Accept out-of-order votes #4327

friedger commented Feb 1, 2024

setzeus left a comment •

edited

Loading

MarvinJanssen left a comment

codecov bot commented Feb 2, 2024 •

edited

Loading

friedger commented Feb 5, 2024

jcnelson left a comment

hstove left a comment

hstove Feb 7, 2024

hstove Feb 7, 2024

hstove Feb 7, 2024

hstove Feb 7, 2024

obycode commented Feb 12, 2024

blockstack-devops commented Nov 5, 2024

Accept out-of-order votes #4327

Accept out-of-order votes #4327

Conversation

friedger commented Feb 1, 2024

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

setzeus left a comment • edited Loading

Choose a reason for hiding this comment

MarvinJanssen left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2024 • edited Loading

Codecov Report

friedger commented Feb 5, 2024

jcnelson left a comment

Choose a reason for hiding this comment

hstove left a comment

Choose a reason for hiding this comment

hstove Feb 7, 2024

Choose a reason for hiding this comment

hstove Feb 7, 2024

Choose a reason for hiding this comment

hstove Feb 7, 2024

Choose a reason for hiding this comment

hstove Feb 7, 2024

Choose a reason for hiding this comment

obycode commented Feb 12, 2024

blockstack-devops commented Nov 5, 2024

setzeus left a comment •

edited

Loading

codecov bot commented Feb 2, 2024 •

edited

Loading