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.

Sign up for GitHub

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

Jump to bottom

[pox-4] Stack Aggregation Increase Update #4644

Merged
merged 17 commits into from
Apr 9, 2024
Merged

[pox-4] Stack Aggregation Increase Update #4644

merged 17 commits into from
Apr 9, 2024

Conversation

setzeus
Copy link
Collaborator

@setzeus setzeus commented Apr 4, 2024

Description

Initial draft of a solution to address #4643 found by @janniks. Keeping the logic in parallel with solo 'stack-increase', we opted to adding a required signature | set approval to the existing 'stack-aggregation-increase' function.

This PR also includes minimal testing setup seen in #4586 as well (which will likely be rebased once this is first merged in). Also includes some comment clean up.

Applicable issues

#4643

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

@setzeus setzeus changed the title [pox-4] [pox-4] Stack Aggregation Increase Update Apr 4, 2024
kantai
kantai reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM -- just caught a different issue in the function that should be handled as well.

friedger
friedger reviewed Apr 4, 2024
View reviewed changes
Copy link
Collaborator

@friedger friedger 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

stackslib/src/chainstate/stacks/boot/pox-4.clar Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Apr 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.31236% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 77.94%. Comparing base (7cd05b8) to head (e63366c).

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4644      +/-   ##
===========================================
- Coverage    83.40%   77.94%   -5.46%     
===========================================
  Files          470      470              
  Lines       333125   333577     +452     
  Branches       317      322       +5     
===========================================
- Hits        277842   260017   -17825     
- Misses       55275    73552   +18277     
  Partials         8        8
Files Coverage Δ
stackslib/src/chainstate/stacks/boot/mod.rs 80.54% <100.00%> (-14.21%) ⬇️
...tackslib/src/chainstate/stacks/boot/pox_4_tests.rs 83.40% <100.00%> (-16.18%) ⬇️
stackslib/src/util_lib/signed_structured_data.rs 97.84% <ø> (ø)
stackslib/src/chainstate/stacks/boot/pox-4.clar 14.12% <0.00%> (-0.22%) ⬇️

... and 195 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 7cd05b8...e63366c. Read the comment docs.

@saralab saralab requested a review from jcnelson April 5, 2024 13:48
@setzeus
Copy link
Collaborator Author

setzeus commented Apr 5, 2024

Confirmed change: update confusing (let ...) variable names.

@setzeus setzeus force-pushed the bug/update-aggregation-increase branch from 981780f to 48442cc Compare April 5, 2024 16:55
@setzeus setzeus marked this pull request as ready for review April 5, 2024 16:55
kantai
kantai previously approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This LGTM

jcnelson
jcnelson requested changes Apr 5, 2024
View reviewed changes
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.

I think you still have one more bug.

kantai
kantai previously approved these changes Apr 6, 2024
View reviewed changes
obycode
obycode previously approved these changes Apr 6, 2024
View reviewed changes
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.

🎉

@obycode obycode mentioned this pull request Apr 8, 2024
Closed
jcnelson
jcnelson reviewed Apr 9, 2024
View reviewed changes
stackslib/src/chainstate/stacks/boot/pox-4.clar
@@ -1433,6 +1437,9 @@
(max-amount uint)
(auth-id uint))
(begin
;; must be called directly by the tx-sender or by an allowed contract-caller
(asserts! (check-caller-allowed)
Copy link
Member

Choose a reason for hiding this comment

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

Great catch!

@obycode obycode merged commit b5a25f0 into develop Apr 9, 2024
1 of 2 checks passed
@hstove hstove deleted the bug/update-aggregation-increase branch April 9, 2024 13:32
moodmosaic added a commit that referenced this pull request Apr 12, 2024
@moodmosaic
fix(pox-4-tests): update @hirosystems/clarinet-sdk-wasm to 2.4.2 for …
5185e58 
…bug fixes

This includes the fixes in
- #4644
- #4670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@friedger friedger friedger left review comments

@jcnelson jcnelson jcnelson approved these changes

@kantai kantai kantai approved these changes

@obycode obycode obycode approved these changes

@hstove hstove Awaiting requested review from hstove

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@setzeus @jcnelson @kantai @friedger @obycode