PoX-4 Signer Set Structs, Getters & Setters [wip] #4164

setzeus · 2023-12-13T15:02:38Z

PoX-4 Signer Set Structs, Getters, & Setters

Summary of Changes

This PR is WIP addition to #4092 that addresses the PoX-4/Argon epic #3970 with the specific issue #4164. As titled, this specific PR introduces the data structures, getters & setters for the current & next-cycle signer set.

Each cycle signer set can hold up to 4096 signers (identified by a (buff 33)), each with a list of 4096 key-ids (represented as a uint). The following functions were introduced:

Update-current-signer
Update-current-signer-set
Get-signer-set-by-reward-cycle
Get-signer-key-ids-by-reward-cycle

Testing

How were these changes tested?

This was only tested locally for instantiation. Unit tests will be address in #4092 prior to merging into Next.

What future testing should occur?

The logic here should have corresponding unit tests in both Rust & Clarinet/Clarity. I'll be working with @moodmosaic to address these tests correctly.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

…-4-signing-keys

…nlock to /new_block events

…-4-signing-keys

…ing tests

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>

…le file descriptors

- Address type mismatch in POX 4 signer set retrieval - Update variable and function usage for type compatibility

- Address type mismatch in PoX-4 signer set retrieval - Update variable and function usage for type compatibility

jcnelson · 2023-12-19T15:39:46Z

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

+    sim.execute_next_block(|_env| {});
+    sim.execute_next_block(|_env| {});
+
+    sim.execute_next_block(|env| {


Can you verify that this actually catches syntax errors in pox-4? Because, it hadn't in the past.

jcnelson · 2023-12-19T15:39:56Z

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

@@ -34,6 +34,7 @@ use clarity::vm::types::{
 };
 use clarity::vm::{ClarityVersion, Environment, SymbolicExpression};
 use lazy_static::lazy_static;
+use libc::sem_t;


What's this?

jcnelson · 2023-12-19T15:40:29Z

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

@@ -1735,6 +1765,38 @@ pub mod test {
        make_tx(key, nonce, 0, payload)
    }

+    // Get the current signer set


Please don't commit commented-out code

jcnelson · 2023-12-19T15:41:12Z

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

+            vec![
+                Value::UInt(lock_period),
+                addr_tuple,
+                Value::Optional(OptionalData { data: None }),


The make_pox_4_extend() function needs to also take an Option<StacksPublicKey> so it can be added into this call to stack-extend.

Generally speaking, you shouldn't put in default values to PoX-4 callers like make_pox_4_extend. Anything you pass into .pox-4 should be a Rust argument.

jcnelson · 2023-12-19T15:43:05Z

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

@@ -1165,6 +1166,37 @@ impl StacksChainState {
            });
        Ok(aggregate_public_key)
    }
+
+    // Get the current signer set
+    pub fn get_current_signer_set_pox_4(


I don't think you currently have the means of testing this, because there isn't any logic written to write state into reward-cycle-signing-keys. So, I think one of two things need to happen:

You can add the logic to insert state into reward-cycle-signing-keys, e.g. via a private function in pox-4 that the node can call. If you take this route, then you should also add some code to actually populate this data map so you can proceed to test this function in a unit test.

You can delete this function for now, and we'll add it back in once it's possible to populate reward-cycle-signing-keys.

jcnelson · 2023-12-19T15:44:54Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

+;; Map that tracks the signing-key & cycle to a list of key-ids
+(define-map reward-cycle-signing-key-ids 
+    {cycle: uint, signer: (buff 33)}
+    {key-ids: (list 4000 uint)})


Can you add a comment explaining what a key-id is supposed to be, and why it's necessary in addition to the signer-address in reward-cycle-signing-keys and signer in reward-cycle-signing-key-ids? Also, why have both the signing key and the address?

jcnelson · 2023-12-19T15:45:20Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

@@ -576,13 +603,14 @@

 ;; Lock up some uSTX for stacking!  Note that the given amount here is in micro-STX (uSTX).
 ;; The STX will be locked for the given number of reward cycles (lock-period).
-;; This is the self-service interface.  tx-sender will be the Stacker.
+;; This is the self-service interface.  tx-sender will be the Stacker. Stacker is responsible for providing a valid signing key.


Please word-wrap, and also it's The Stacker is responsible ....

jcnelson · 2023-12-19T15:45:36Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

 ;;
 ;; * The given stacker cannot currently be stacking.
 ;; * You will need the minimum uSTX threshold.  This will be determined by (get-stacking-minimum)
 ;; at the time this method is called.
 ;; * You may need to increase the amount of uSTX locked up later, since the minimum uSTX threshold
 ;; may increase between reward cycles.
+;; * You need to provide a signing-key used to allocate signature-weight to a valid signer


signature weight

jcnelson · 2023-12-19T15:47:42Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

+        (asserts! (is-none next-signer-set-map) (err ERR_NEXT_SIGNER_SET_ALREADY_REGISTERED))
+
+        ;; Assert in prepare phase (height equal to / greater than start of prepare phase)
+        (asserts! (>= block-height (- next-reward-cycle-burn-height u100)) (err ERR_NOT_IN_PREPARE_PHASE_EARLY))


This should be a define-read-only function called is-in-prepare-phase. I think one of @friedger's PRs has this?

jcnelson · 2023-12-19T15:48:35Z

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

@@ -67,6 +67,7 @@ use crate::chainstate::stacks::index::marf::MarfConnection;
 use crate::chainstate::stacks::index::MarfTrieId;
 use crate::chainstate::stacks::tests::make_coinbase;
 use crate::chainstate::stacks::*;
+use crate::chainstate::{self};


This is a nonsensical import

jcnelson · 2023-12-19T15:49:16Z

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

+        let reward_cycle = burnchain
+            .block_height_to_reward_cycle(tip.block_height)
+            .unwrap() as u64;
+        peer.chainstate()


This doesn't test anything. There's nothing in this data map, so there's nothing to check. Please actually add data to the data map and verify that you can read it out.

jcnelson · 2023-12-19T15:51:29Z

stackslib/src/chainstate/stacks/boot/pox-4.clar


      ;; return the lock-up information, so the node can actually carry out the lock.
      (ok { stacker: stacker,
            unlock-burn-height: new-unlock-ht }))))

+;; This function allows a node to register a new 'current-signer-set' during the prepare phase
+(define-private (set-current-signer-set (new-signer-set (list 4000 {signer: (buff 33)})))


There is currently no test coverage for this. There should be a unit test in pox_4_tests.rs that calls this function with some synthetic data, and reads it back out again.

Your tests should hit not only the happy path, but also each of the failure paths (and verify that the right error code is returned).

jcnelson · 2023-12-19T15:52:16Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

+
+;; This function allows a node to register a list of key-ids to the current-signer-set
+;; This work is done one-at-a-time by the node & could require up to 4000 individual writes
+(define-private (set-current-signer (signer (buff 33)) (key-ids (list 4000 uint)))


There is currently no test coverage for this. Please add it to pox_4_tests.rs. As before, test each error path as well as the happy path.

jcnelson · 2023-12-19T15:53:57Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

+;; Gets the signer-set for a given reward cycle
+(define-read-only (get-signer-set-by-reward-cycle (cycle uint))
+    (let ((cycle-to-burn-height (reward-cycle-to-burn-height cycle))
+            (burn-height-to-id-header-hash (get-block-info? id-header-hash cycle-to-burn-height)))


This doesn't type-check, since get-block-info? returns an (optional ..). Again, please type-check your code before asking for a review. Use clarity-cli check or have your unit tests run an analysis pass.

jcnelson

Hey @setzeus, there's still lots of work to do here. In particular, it seems that your unit tests don't verify that pox-4 even type-checks. Please fix this, and please add the unit tests described in the above comments. Thanks!

friedger · 2024-01-10T10:15:20Z

Should be re-based to next or closed.

setzeus · 2024-01-10T11:52:25Z

@friedger waiting for @MarvinJanssen to complete his .Signers work to see which of the maps created in this PR will stay here (aka will re-base off of his branch once published or will close).

setzeus and others added 30 commits November 23, 2023 13:37

updated stacking-state & added reward-cycle-signing-keys

b6d447b

updated delegate-stack-stx, stack-extend & delegate-stack-extend

8014575

updated stack-stx

6771f81

comment updates

0db0bbf

changed unwrap to unwrap-panic

8de2d15

added simple instantiation test

db323a4

Merge remote-tracking branch 'refs/remotes/origin/next' into feat/pox…

0d2cffa

…-4-signing-keys

possible formatting issue CI didn't like

8d2d28a

ran cargo fmt --all for formatting CI issue

0adcd09

stashing demo changes

90eb6f5

mockamoto mining working with merge

d17b257

working integration with chains coordinator

44c6ef7

add test for mockamoto, cleanup implementation

6ee473b

add transfer tx to mockamoto test, address PR feedback

38e871b

add stacker-self-signer to mockamoto, add boot receipts, add pox_v3_u…

b2a371a

…nlock to /new_block events

Merge remote-tracking branch 'refs/remotes/origin/next' into feat/pox…

d641ec5

…-4-signing-keys

updated with getter get-sgner-info-for-cycle

ebb429f

Fixed failing unit tests by updating 'stacks-stx' parameters in exist…

6f56aaf

…ing tests

removed unnecessary signer-key optional wrapper

68fedcd

Merge branch 'next' into feat/pox-4-signing-keys

9e92a6f

fix: Use VRFProof::consensus_serialize() to address issue #4115

87d22b9

Fix unit tests and TransactionPayload::deserialize()

b14642d

fix: RPC endpoints should default to proof=1

960a18b

test: fix httpcore test for defaulting in proof query string

e4c0251

use bitcoin 25.0 for tests

4b99609

Make tx fee a variable in stackerdb dkg test and increase it

4ecf8cc

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>

reduce number of signers and keys so test doesn't consume all availab…

8ea4aa4

…le file descriptors

set test_stackerdb_dkg to ignore

7382c6f

fix: set maxburncap to 1BTC in sendrawtransaction (new in Bitcoin 25.0)

ac51c0f

fix: handle epochs 2.5 and 3.0 correctly

81cbbdc

setzeus added 2 commits December 16, 2023 18:06

simplified get-signer-set-by-reward-cycle

fbff70c

addressed comments

b8d1cd4

friedger mentioned this pull request Dec 18, 2023

Vote for aggregated public key #4116

Closed

5 tasks

setzeus and others added 2 commits December 18, 2023 07:44

format update

ecbd6b0

fix: Correct typecheck in get_current_signer_set test

3d98bfd

- Address type mismatch in POX 4 signer set retrieval - Update variable and function usage for type compatibility

moodmosaic force-pushed the feat/pox-4-signer-set-getter-and-setters branch from a171c84 to 3d98bfd Compare December 18, 2023 16:52

fix: Correct typecheck in get_current_signer_set test

14444af

- Address type mismatch in PoX-4 signer set retrieval - Update variable and function usage for type compatibility

setzeus force-pushed the feat/pox-4-signing-keys branch from 214d72d to 81273b6 Compare December 19, 2023 02:54

jcnelson reviewed Dec 19, 2023

View reviewed changes

jcnelson requested changes Dec 19, 2023

View reviewed changes

jcnelson mentioned this pull request Dec 19, 2023

PoX-4 Signing Keys Stacking-State #4092

Closed

8 tasks

moodmosaic force-pushed the feat/pox-4-signing-keys branch from 81273b6 to 72ca439 Compare December 19, 2023 16:35

setzeus changed the title ~~PoX-4 Signer Set Structs, Getters & Setters~~ PoX-4 Signer Set Structs, Getters & Setters [wip] Dec 21, 2023

friedger marked this pull request as draft January 10, 2024 16:56

saralab closed this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoX-4 Signer Set Structs, Getters & Setters [wip] #4164

PoX-4 Signer Set Structs, Getters & Setters [wip] #4164

setzeus commented Dec 13, 2023 •

edited

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson Dec 19, 2023

jcnelson left a comment

friedger commented Jan 10, 2024

setzeus commented Jan 10, 2024

PoX-4 Signer Set Structs, Getters & Setters [wip] #4164

PoX-4 Signer Set Structs, Getters & Setters [wip] #4164

Conversation

setzeus commented Dec 13, 2023 • edited

PoX-4 Signer Set Structs, Getters, & Setters

Summary of Changes

Testing

How were these changes tested?

What future testing should occur?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson left a comment

Choose a reason for hiding this comment

friedger commented Jan 10, 2024

setzeus commented Jan 10, 2024

setzeus commented Dec 13, 2023 •

edited