pox-4: disallow stacking during prepare phase

[friedger]

Description

This PR update pox-4 to disallow stacking during the prepare phase

Applicable issues

• fixes Pox: Disallow stacking locks during prepare phase #3775

Additional info (benefits, drawbacks, caveats)

When user stack stx during the prepare phase, their stx is locked, but they do not earn rewards. In addition, pool operators have extra work to remove these pool members.

Checklist

• Test coverage for new or modified code paths
• Changelog is updated

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8cffc38) 85.08% compared to head (f947264) 82.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4094      +/-   ##
==========================================
- Coverage   85.08%   82.89%   -2.20%     
==========================================
  Files         429      429              
  Lines      302009   302009              
==========================================
- Hits       256976   250358    -6618     
- Misses      45033    51651    +6618     

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

[jcnelson]

Hi @friedger, thanks for sending this over!

Regarding the PR organization, we don't use Clarinet to test pox-4 (or any PoX implementation) because PoX is not a smart contract. It's a part of the Clarity VM that just happens to be written in Clarity. Its behavior and implementation is deeply dependent on the ways in which the node interacts with it (e.g. in order to lock/unlock STX, calculate reward sets, and so on). We test PoX in Rust via tests in pox-locking/src and in stackslib/src/chainstate/stacks/boot/.

[jcnelson]

Regarding the PR organization, we don't use Clarinet to test pox-4 (or any PoX implementation) because PoX is not a smart contract

Actually, I want to clarify this. It's totally fine to use Clarinet to test pox-4 (see https://github.com/hirosystems/stacks-e2e-testing); it's just that it shouldn't come as a substitute or at the expense of the Rust tests we have for locking/unlocking.

[friedger]

pox_4.rs and pox_4_tests.rs is not yet in branch next. Therefore, rust tests for pox-4 are out of scope of this PR.

[moodmosaic]

We test PoX in Rust via tests in pox-locking/src and in stackslib/src/chainstate/stacks/boot/.

@jcnelson, this means we need a pox_4.rs similar to https://github.com/stacks-network/stacks-core/blob/next/pox-locking/src/pox_3.rs, right?

[jcnelson]

Yes

…

On Wed, Nov 29, 2023, 7:40 AM Nikos Baxevanis ***@***.***> wrote: We test PoX in Rust via tests in pox-locking/src and in stackslib/src/chainstate/stacks/boot/. @jcnelson <https://github.com/jcnelson>, this means we need a pox_4.rs similar to https://github.com/stacks-network/stacks-core/blob/next/pox-locking/src/pox_3.rs, right? — Reply to this email directly, view it on GitHub <#4094 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADQJKYUQXVW6DOAVRSKGD3YG4UM7AVCNFSM6AAAAAA75YC2PCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRHAZDENZTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[friedger]

checks are green now :-)

[zone117x]

The new event for revoke-delegate-stx looks good. I'm not sure about the implications of the stacking during prepare phase change, so my approval for this PR wouldn't mean much. @janniks any thoughts?

[friedger]

I have added a link to the discussion on discord before this feature request (https://discord.com/channels/621759717756370964/730814571517968436/1112787020117463100) to the corresponding github issue

[friedger]

All comments addressed

[friedger]

Changes related to revoke-delegate-stx have been removed.
This PR should be now much easier to merge.

[setzeus]

Reviewed & all LGTM. Only thoughts are maybe the name registration tests should be extracted / part of another PR?

Edit* Especially since I see more BNS tests in #4100 & #4116 I think it might make sense to make a separate PR strictly for BNS items.

[friedger]

@zone117x Could you please review this again?

[obycode]

Feedback from PR meeting - let's open a discussion. @AshtonStephens

[jcnelson]

Discussion created: #4193

[friedger]

Converted to draft until discussion is concluded

[saralab]

Risky , will reconsider for next HF

[blockstack-devops]

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.