-
Notifications
You must be signed in to change notification settings - Fork 666
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.
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
PoX-4 Signing Keys Stacking-State #4092
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## next #4092 +/- ##
===========================================
- Coverage 85.08% 64.10% -20.99%
===========================================
Files 429 429
Lines 302009 302143 +134
===========================================
- Hits 256976 193700 -63276
- Misses 45033 108443 +63410 ☔ View full report in Codecov by Sentry. |
Additional context re: using 'delegate-stack-stx' vs. 'stack-aggregation-commit-index' to submit a pool's signing-key for delegates that registered without a signing-key here: |
Note: per Clarity WG morning discussion it's been noted that our preferred workflow here is to actively work on a version of PoX-4 that exists in /contrib per #4094 & then periodically copy that over to the more hardened PoX-4 that'll live in /boot. Once #4094 is merged in I'll make these adjust only to the PoX-4 version in /contrib for now. (unless there's reasonable push back on this workflow) |
delegated-to: (optional principal) | ||
delegated-to: (optional principal), | ||
;; signing key for Nakamoto+, only 'none' when delegated & before delegate calls 'stack-aggregation-commit-indexed | ||
signing-key: (optional (buff 33)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this could just be (buff 33)
rather than an (optional (buff 33))
: the delegate sets this value in delegate-stack-stx
, which is also the first time the stacker-state is instantiated for the delegator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to allow delegators enforce a signing key? If yes, should it be stored in a separate map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to allow delegators enforce a signing key? If yes, should it be stored in a separate map?
Why would the delegator call delegate-stack-stx
if the user supplied the wrong key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the user/pool member. My question does not make sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that stack-aggregation-commit can't change the signing key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing Jude's comments, and just had a few more
@setzeus -- this patch adds a unit test which would check that the contract instantiates: diff --git a/stackslib/src/chainstate/stacks/boot/contract_tests.rs b/stackslib/src/chainstate/stacks/boot/contract_tests.rs
index bd7941f73..d8450f0e7 100644
--- a/stackslib/src/chainstate/stacks/boot/contract_tests.rs
+++ b/stackslib/src/chainstate/stacks/boot/contract_tests.rs
@@ -60,6 +60,8 @@ lazy_static! {
pub static ref POX_CONTRACT_TESTNET: QualifiedContractIdentifier = boot_code_id("pox", false);
pub static ref POX_2_CONTRACT_TESTNET: QualifiedContractIdentifier =
boot_code_id("pox-2", false);
+ pub static ref POX_4_CONTRACT_TESTNET: QualifiedContractIdentifier =
+ boot_code_id("pox-4", false);
pub static ref COST_VOTING_CONTRACT_TESTNET: QualifiedContractIdentifier =
boot_code_id("cost-voting", false);
pub static ref USER_KEYS: Vec<StacksPrivateKey> =
@@ -559,6 +561,31 @@ impl HeadersDB for TestSimHeadersDB {
}
}
+#[test]
+fn pox_4_instantiates() {
+ let mut sim = ClarityTestSim::new();
+ sim.epoch_bounds = vec![0, 1, 2];
+ let delegator = StacksPrivateKey::new();
+
+ let expected_unlock_height = POX_TESTNET_CYCLE_LENGTH * 4;
+
+ // execute past 2.1 epoch initialization
+ sim.execute_next_block(|_env| {});
+ sim.execute_next_block(|_env| {});
+ sim.execute_next_block(|_env| {});
+
+ sim.execute_next_block(|env| {
+ env.initialize_versioned_contract(
+ POX_4_CONTRACT_TESTNET.clone(),
+ ClarityVersion::Clarity2,
+ &boot::POX_4_TESTNET_CODE,
+ None,
+ ASTRules::PrecheckSize,
+ )
+ .unwrap()
+ });
+}
+
#[test]
fn pox_2_contract_caller_units() {
let mut sim = ClarityTestSim::new(); The test passes with the most recent changes:
It also (correctly) fails if applied to the commit 6771f81. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to understand whether the same logic for the pox-address should be applied for the signing key for pools: If the pool operator decides to use a different signer key, the pool members rewards are not really at risk. However, would pool members like to decide whether the pool operator can delegate to one or the other signer? I think yes, therefore delegate-stx
should also contain a signing key in the delegation-state
@setzeus There at least one rust functions that create stack-stx transactions with 4 instead of 5 arguments that makes the unit tests fail. from the logs of the test:
|
Ahhh okay, thx @friedger, so updating PoX-4 stacking parameters broke existing tests somewhere else* I follow. Looking into it. |
Great to see the changes approved here! 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating the stacking interfaces with these new methods! Before merging, I must insist that you add unit tests for them. It's not enough to see that PoX-4 instantiates; you must also verify that the public keys get added to the stacking state. Thanks!
@@ -283,8 +283,9 @@ fn test_simple_nakamoto_coordinator_bootup() { | |||
#[test] | |||
fn test_simple_nakamoto_coordinator_1_tenure_10_blocks() { | |||
let mut test_signers = TestSigners::default(); | |||
let mut peer = boot_nakamoto(function_name!(), vec![], test_signers.aggregate_public_key); | |||
let private_key = peer.config.private_key.clone(); | |||
//let mut peer = boot_nakamoto(function_name!(), vec![], test_signers.aggregate_public_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove dead code
let private_key = peer.config.private_key.clone(); | ||
//let mut peer = boot_nakamoto(function_name!(), vec![], test_signers.aggregate_public_key); | ||
//let private_key = peer.config.private_key.clone(); | ||
let private_key = StacksPrivateKey::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a new key? Can you continue to please use the same key so we get consistent output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation from @kantai to pass/debug those unit tests, reverting back to commented out above I get in both places where I switched out peer.config for a new private key:
let private_key = peer.config.private_key.clone();
| ^^^^ not found in this scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is because you've commented out the code that instantiates peer
An update from the testing WG. This PR is blocked on getting the |
// (pox-addr (tuple (version (buff 1)) (hashbytes (buff 32)))) | ||
// (burn-height uint) | ||
// (lock-period uint)) | ||
let public_key = StacksPublicKey::from_private(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key
is the key used to sign the transaction, not the DKG signing key. There needs to be a separate argument to make_pox_4_lockup
for that signing key.
@@ -117,7 +118,9 @@ | |||
;; previous stack-stx calls, or prior to an extend) | |||
reward-set-indexes: (list 12 uint), | |||
;; principal of the delegate, if stacker has delegated | |||
delegated-to: (optional principal) | |||
delegated-to: (optional principal), | |||
;; signing key for Nakamoto, only 'none' when delegated & before delegate calls stack-aggregation-commit-indexed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is ever none
if we're setting the signing key on delegate-stack-stx
(which IIRC is the plan now)
EDIT: this comment is out of date
@@ -591,7 +605,8 @@ | |||
(define-public (stack-stx (amount-ustx uint) | |||
(pox-addr (tuple (version (buff 1)) (hashbytes (buff 32)))) | |||
(start-burn-ht uint) | |||
(lock-period uint)) | |||
(lock-period uint) | |||
(signing-key (buff 33))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment about signing-key
to the method doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated a sentence & added the following to comment doc above function:
;; ...Stacker is responsible for providing a valid signing key.
...
;; You need to provide a signing-key used to allocate signature-weight to a valid signer
@@ -829,7 +845,8 @@ | |||
(amount-ustx uint) | |||
(pox-addr { version: (buff 1), hashbytes: (buff 32) }) | |||
(start-burn-ht uint) | |||
(lock-period uint)) | |||
(lock-period uint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here -- can you add a comment to the method doc about the signing key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated a sentence & added the following to comment doc above function:
;; * You need to provide a signing-key used to allocate signature-weight to a valid signer
testnet/stacks-node/src/mockamoto.rs
Outdated
@@ -824,6 +824,9 @@ impl MockamotoNode { | |||
Some(AddressHashMode::SerializeP2PKH), | |||
); | |||
|
|||
let public_key = StacksPublicKey::from_private(&self.miner_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there instead needs to be a separate signing_key
field in MockamotoNode
. Same for the Nakamoto node. Perhaps it can be part of the Keychain
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice with some tests to ensure everything works as intended:
-
signing-key
in stacking-state:- Verify that the
signing-key
is correctly set.
- Verify that the
-
Modified Functions (
stack-stx
,delegate-stack-stx
, etc.):- Ensure all functions modified to include the
signing-key
handle it correctly.
- Ensure all functions modified to include the
-
stack-extend
,delegate-stack-extend
:- Test handling of
updated-signing-key
, ensuring proper updates in the stacking state.
- Test handling of
-
get-signer-info-for-cycle
:- Verify retrieval of correct signer info for various inputs.
@jcnelson, @setzeus, could you share your thoughts on these test cases or suggest additional ones that might be relevant?
@@ -27,6 +27,7 @@ | |||
(define-constant ERR_DELEGATION_WRONG_REWARD_SLOT 29) | |||
(define-constant ERR_STACKING_IS_DELEGATED 30) | |||
(define-constant ERR_STACKING_NOT_DELEGATED 31) | |||
(define-constant ERR_STACK_EXTEND_NO_SIGNING_KEY 32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this constant used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused, removed.
214d72d
to
81273b6
Compare
@moodmosaic Yes, please test all of those in |
@@ -98,7 +98,7 @@ Clarinet.test({ | |||
], | |||
cases[0].nameOwner.address), | |||
]); | |||
assertEquals(block.height, 2); | |||
assertEquals(block.height, 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like there was an off-by-one error somewhere, with all these changes. Can you elaborate more on why all of these block heights are now +1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pox-4 is in a different epoch than bns, therefore booting needs one more block
@@ -1798,7 +1794,7 @@ pub mod test { | |||
boot_code_test_addr(), | |||
POX_4_NAME, | |||
"stack-extend", | |||
vec![Value::UInt(lock_period), addr_tuple], | |||
vec![Value::UInt(lock_period), addr_tuple, Value::Optional(OptionalData {data: None})], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't pass None
; instead, make this an argument in Rust which gets passed to stack-extend
.
(define-constant ADDRESS_VERSION_NATIVE_P2WPKH 0x04) | ||
(define-constant ADDRESS_VERSION_NATIVE_P2WSH 0x05) | ||
(define-constant ADDRESS_VERSION_NATIVE_P2TR 0x06) | ||
;; (define-constant ADDRESS_VERSION_NATIVE_P2WPKH 0x04) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't commit commented-out code
|
||
;; PoX disabling threshold (a percent) | ||
(define-constant POX_REJECTION_FRACTION u25) | ||
|
||
;; Valid values for burnchain address versions. | ||
;; These first four correspond to address hash modes in Stacks 2.1, | ||
;; and are defined in pox-mainnet.clar and pox-testnet.clar (so they | ||
;; cannot be defined here again). | ||
;; (define-constant ADDRESS_VERSION_P2PKH 0x00) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're now using is-in-mainnet
instead of a separate pox-4-mainnet.clar
, then where are these constants defined now?
@@ -508,6 +535,14 @@ | |||
(and (>= lock-period MIN_POX_REWARD_CYCLES) | |||
(<= lock-period MAX_POX_REWARD_CYCLES))) | |||
|
|||
;; Is the given burn block height in the prepare phase? | |||
;; This computes `((height - first-burnchain-block-height) + pox-prepare-cycle-length) % pox-reward-cycle-length) < pox-prepare-cycle-length`. | |||
(define-read-only (check-prepare-phase (height uint)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -545,6 +580,10 @@ | |||
(asserts! (check-pox-lock-period num-cycles) | |||
(err ERR_STACKING_INVALID_LOCK_PERIOD)) | |||
|
|||
;; stacking must not happen during prepare phase | |||
(asserts! (not (check-prepare-phase burn-block-height)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this -- it has not been decided yet that stacking will be disabled in the prepare phase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Reverting all the work here to un-do rebasing from #4094.
81273b6
to
72ca439
Compare
;; Once the delegate has stacked > minimum, the delegate should call stack-aggregation-commit | ||
(define-public (delegate-stack-stx (stacker principal) | ||
(amount-ustx uint) | ||
(pox-addr { version: (buff 1), hashbytes: (buff 32) }) | ||
(start-burn-ht uint) | ||
(lock-period uint)) | ||
(lock-period uint) | ||
(signing-key (buff 33))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcnelson, we'd like to get this method (delegate-stack-stx
) under test, with @setzeus. In this diff, there's the signing-key
key that's added.
Looking at PoX-3 tests, some candidates to copy over to PoX-4 are
All of these tests seem to call into delegate-stack-stx
. The question is:
- Are all those tests relevant in the context of PoX-4
- Which one would be the best to start with, so it can be added into this PR.
Work picked up by @MarvinJanssen in #4209 to add required tests in addition to base tests - changing to draft until #4209 is merged in then will close. |
Closing since #4209 is now merged in. |
PoX-4 Signing Key Stacking-State Update
Summary of Changes
This PR is meant to introduce, not complete, the core logic around signing keys in PoX-4 & address #4057 . The focus is specifically on updating the 'stacking-state' map & all the functions that read & write from it. This includes updates to the following public functions:
what this does not include
Getters & setters to the new map 'reward-cycle-signing-keys,' that'll be in another PR. This PR also does not include any logic that'll be used in relevant issues #4058, #4059, & #3970.
Testing
How were these changes tested?
This was only tested locally for syntax errors / clarinet check.
What future testing should occur?
As soon as this is merged I'll open an issue with the user stories that require testing for @moodmosaic to work into pox_4_tests.rs.
Checklist: