.signers StackerDB contract and PoX 4 signer key changes

[MarvinJanssen]

Description

Now rebased onto #4269.

Work in progress that adds:

• PoX4 changes, takes signer key (buff 33) parameter where needed, converts and stores them as principal.
• The .signers StackerDB contract.
• Uses Nakamoto: Add stacker/signer bit vector and event #4269 Nakamoto make_reward_set() to calculate the signer slots.
• Fills in the mocks/todos in Nakamoto: Add stacker/signer bit vector and event #4269 pertaining to the signers.
• Writes the signer set to .signers.
• Basic unit tests that Stacks using two different signer keys and checks they exist in the signers StackerDB contract.

As for the coupling of PoX reward slots and signer keys we have some open questions. (#4247 and others.)

This PR should unblock @friedger's work on the .voting boot contract.

Applicable issues

Addresses:

• [stacks-core] Update the stacker-db contract from the list of registered signers #4058
• stacks-node must write the new set of stackers to the .pox-4 contract #3980
• [clarity] signer StackerDB contract for pox-4 #3957

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

[kantai]

With just a quick skim, I have some high-level feedback about the approach in this PR.

This PR sets the signing-key via a separate list (actually data-map index) from the reward set list. But unless I'm misunderstanding how signing keys and the PoX reward set should operate, these should be the same list: any entry in the PoX reward set should have a signing-key and any signing-key should be associated with a reward-set entry.

This also impacts the signing-set calculation method, because really this should be calculated via the same get_reward_set method that the PoX reward set construction uses (because the signer set and the reward set are more than related: they should have an exact correspondence). This is top of mind right now to me because the signer bitvec depends on this work, and so I implemented an initial version using mocked data inputs:

https://github.com/stacks-network/stacks-core/blob/feat/stacker-bitvec/stackslib/src/chainstate/stacks/boot/mod.rs#L612

See the note here:

https://github.com/stacks-network/stacks-core/blob/feat/stacker-bitvec/stackslib/src/chainstate/nakamoto/coordinator/mod.rs#L69

[MarvinJanssen]

Thanks @kantai I appreciate you giving the background and linking those files. For this PR the scope was to introduce .signers and to be able to do a round trip of Stacking -> signer keys to PoX -> have the node write signer keys to .signers -> ability to pull and check it in unit tests.

As for the coupling of PoX reward slots and signer keys we have some open questions. (#4247 and others.) With the state of the PR it will be easy to move the signer keys elsewhere / handle them differently in the PoX contract.

I'll update the OP to reflect the above.

[MarvinJanssen]

Should I rename .signers to .stackers?

[jcnelson]

Pretty sure this is off-by-one.

[MarvinJanssen]

@kantai I rebased this onto your branch and filled in the missing pieces and finished the PoX changes. My basic unit test works. There is a bit of code repetition. Let me know if this is the direction you are thinking of and I can clean it up and add some more tests to get it merged. For now I'm also targeting your branch, if yours gets merged first then I will rebase off next.

[friedger]

Why does the partial stacked state need to know about the signer?

Thinking about pooled stacking, it makes sense to provide the key only on aggregate contract calls. The voting slots are based on aggregate values as well.

[kantai]

Yeah -- I agree with this. Partial stacking state shouldn't have the signer, the signer should be set on aggregate-commit. The stacking-state map also should not have a signer-key field.

So I'd recommend:

1. Removing signer-key from stacking-state
2. Removing signer from the partial-stacked-by-cycle
3. Add signer-key argument to the aggregate-commit method.

Otherwise, the changes in pox-4 look good to me.

[setzeus]

Copy. Wrote similar thoughts here #4247 just a few mins ago. Assigning to here / @MarvinJanssen.

[MarvinJanssen]

Yep we were going back and forth on it. Will change it soon. Thanks @friedger

[MarvinJanssen]

Edit: is having the signer key in stacking state not useful to have?

[hstove]

So is the plan to use (signer principal) or (signer-key (buff 33))?

[jcnelson]

The arguments to stacking are (signer-key (buff 33)), but the internal data maps store principal. The stacks functions will need to compute the principal from the signer key.

[jcnelson]

Yeah -- I agree with this. Partial stacking state shouldn't have the signer, the signer should be set on aggregate-commit. The stacking-state map also should not have a signer-key field.

So I'd recommend:

1. Removing signer-key from stacking-state
2. Removing signer from the partial-stacked-by-cycle
3. Add signer-key argument to the aggregate-commit method.

Otherwise, the changes in pox-4 look good to me.

@kantai Are you going to make this happen?

[jcnelson]

@kantai @MarvinJanssen @hstove per @jferrant, we actually need to store the signing keys to the stacker map after all.

[kantai]

Okay, my last push contained a handful of things that I'd appreciate some feedback on (some of these need @jcnelson's eyes):

1. The choice of when to apply the .signers change is really important. This has to be strictly defined, and it can't be a single burn height. My recent commit sets it as "if we're in a prepare phase, and the update hasn't been applied for the target reward cycle, apply it". That means it is the "first block of the prepare phase", but it might be the second burn block of the prepare phase if the first burn block didn't contain a sortition.
2. Because of that constraint, the PoX anchor block selection in Nakamoto needs to be the same block or later. My commit updates the PoX anchor block selection to be the first block evaluated in a prepare phase.
3. In testing this PR, it surfaced a bug in the nakamoto miner: it was loading miner_tenure_info from the SortitionDB's canonical chain tip, but setting header.consensus_hash to be equal to a passed-in value. This PR changes that behavior so that miner_tenure_info is loading information based on that consensus_hash.

Once I've got an okay on the above, I'll wrap this PR up by applying the last pox-4 changes discussed above (i.e., removing the signing-key from the stacker-state because its duplicated information from the reward set list). I'd like to leave out a TODO item from this PR:

    pub fn get_reward_set_nakamoto(
        &self,
        cycle_start_burn_height: u64,
        chainstate: &mut StacksChainState,
        burnchain: &Burnchain,
        sortdb: &SortitionDB,
        block_id: &StacksBlockId,
    ) -> Result<RewardSet, Error> {
        // TODO: this method should read the .signers contract to get the reward set entries.
        //   they will have been set via `NakamotoChainState::check_and_handle_prepare_phase_start()`.
        let cycle = burnchain
            .block_height_to_reward_cycle(cycle_start_burn_height)
            .expect("FATAL: no reward cycle for burn height");

Because implementing that will take a bit of time, and its not needed for unblocking the signer-miner block proposal work. So, I'd rather take care of it in a separate PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/stacker-bitvec@0d8d736). Click here to learn what that means.

❗ Current head 8056694 differs from pull request most recent head e9fd4a9. Consider uploading reports for the commit e9fd4a9 to get more accurate results

Additional details and impacted files

@@                  Coverage Diff                   @@
##             feat/stacker-bitvec    #4263   +/-   ##
======================================================
  Coverage                       ?   28.62%           
======================================================
  Files                          ?      434           
  Lines                          ?   307604           
  Branches                       ?        0           
======================================================
  Hits                           ?    88050           
  Misses                         ?   219554           
  Partials                       ?        0           

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

[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.