Get/Set aggregate public key from Clarity #4165

jcnelson · 2023-12-13T15:13:14Z

This merges #4113 and #4126

Signed-off-by: Jacinta Ferrant <[email protected]>

…p functions Signed-off-by: Jacinta Ferrant <[email protected]>

Signed-off-by: Jacinta Ferrant <[email protected]>

…d of push Signed-off-by: Jacinta Ferrant <[email protected]>

call set-aggregate-public-key during append_block pass reward cycle and agg pubkey as symbolic expressions to private contract call read aggregate public key from parent reward cycle then set it in the following remove TODO comment referencing the issue being fixed use from instead of as for explicit cast fmt fixes

…see if the key is set automatically for a new reward cycle

…k/execute_in_env context chain

Signed-off-by: Jacinta Ferrant <[email protected]>

…p functions Signed-off-by: Jacinta Ferrant <[email protected]>

Signed-off-by: Jacinta Ferrant <[email protected]>

* Refactor some of the reused structs from `neon_node` * Fix a logic-bug in `nakamoto::coordinator`: the first prepare phase information will be a Epoch2x block, so the reward set calculation has to handle that. * Add `nakamoto_node` module based on `neon_node` * Add simple integration test for `nakamoto_node`

…ommits at tenure_id changes, cargo fmt-stacks

…c-key

…set-aggregate-public-key

jcnelson · 2023-12-13T15:13:37Z

Actually, let me rebase this onto feat/naka-neon

…up_block() and call it on every block

codecov · 2023-12-13T15:29:35Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (e71444a) 82.70% compared to head (b63ca21) 82.89%.

Files	Patch %	Lines
stackslib/src/chainstate/nakamoto/mod.rs	91.56%	14 Missing ⚠️
stackslib/src/chainstate/burn/db/sortdb.rs	50.00%	6 Missing ⚠️
testnet/stacks-node/src/mockamoto/tests.rs	96.52%	4 Missing ⚠️
stackslib/src/clarity_vm/clarity.rs	96.72%	2 Missing ⚠️
stackslib/src/net/mod.rs	77.77%	2 Missing ⚠️
testnet/stacks-node/src/nakamoto_node/miner.rs	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4165      +/-   ##
==========================================
+ Coverage   82.70%   82.89%   +0.19%     
==========================================
  Files         429      429              
  Lines      301662   302009     +347     
==========================================
+ Hits       249478   250362     +884     
+ Misses      52184    51647     -537

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

xoloki

There's a stale comment in observe_set_aggregate_key, but ow LGTM. Thanks so much for untangling this @jcnelson

xoloki · 2023-12-13T15:57:58Z

stackslib/src/chainstate/nakamoto/mod.rs

@@ -2484,6 +2493,16 @@ impl NakamotoChainState {
            );
        }

+        if !clarity_tx.config.mainnet {
+            Self::set_aggregate_public_key(


Yeah doing this here instead of append_block seems much more correct.

stackslib/src/chainstate/nakamoto/mod.rs

xoloki · 2023-12-13T16:02:06Z

testnet/stacks-node/src/mockamoto/tests.rs

+        })
+        .expect("FATAL: failed to start mockamoto main thread");
+
+    // complete within 5 seconds or abort (we are only observing one block)


This comment was in the original code, but we're now observing up to 100 blocks in 120 secs. Assuming that this is what we really want to be doing, we should fix this comment.

jferrant · 2023-12-13T16:43:33Z

testnet/stacks-node/src/run_loop/nakamoto.rs

@@ -218,13 +225,49 @@ impl RunLoop {
            .map(|e| (e.address.clone(), e.amount))
            .collect();

-        // TODO (nakamoto-neon): check if we're trying to setup a self-signing network
-        //  and set the right genesis data
+        let agg_pubkey_boot_callback = if let Some(self_signer) = self.config.self_signing() {


Ah this was the key I was needing. Was wondering how to set boot contract state, but couldn't seem to figure it out!

…eward cycles_ because integration tests can run in epoch 2.x for many reward cycles before the epoch 3 transition (and, the neon boot code sets the initial aggregate public key)

… 30s)

stackslib/src/chainstate/nakamoto/mod.rs

kantai · 2023-12-13T19:23:04Z

stackslib/src/chainstate/nakamoto/mod.rs

+        for parent_reward_cycle in (0..=my_reward_cycle).rev() {
+            // carry forward the aggregate public key in the past reward cycle to the current
+            // reward cycle.  It may be several cycles back, such as in integration tests where
+            // nakamoto boots up several reward cycles after the initial aggregate public key was set.
+            // TODO: replace with signer voting
+            debug!(
+                "Try setting aggregate public key in reward cycle {}, parent {}",
+                my_reward_cycle, parent_reward_cycle
+            );
+            // execute `set-aggregate-public-key` using `clarity-tx`
+            let Some(aggregate_public_key) = clarity_tx
+                .connection()
+                .with_readonly_clarity_env(
+                    mainnet,
+                    chain_id,
+                    ClarityVersion::Clarity2,
+                    StacksAddress::burn_address(mainnet).into(),
+                    None,
+                    LimitedCostTracker::Free,
+                    |vm_env| {
+                        vm_env.execute_contract_allow_private(
+                            &boot_code_id(POX_4_NAME, mainnet),
+                            "get-aggregate-public-key",
+                            &vec![SymbolicExpression::atom_value(Value::UInt(u128::from(
+                                parent_reward_cycle,
+                            )))],
+                            true,
+                        )
+                    },
+                )
+                .ok()
+                .map(|agg_key_value| {
+                    let agg_key_opt = agg_key_value.expect_optional().map(|agg_key_buff| {
+                        Value::buff_from(agg_key_buff.expect_buff(33))
+                            .expect("failed to reconstruct buffer")
+                    });
+                    agg_key_opt
+                })
+                .flatten()
+            else {
+                debug!(
+                    "No aggregate public key in parent cycle {}",
+                    parent_reward_cycle
+                );
+                continue;
+            };


Looping over reward cycles should be unnecessary. If an integration test requires it, the integration test itself should be altered, rather than this code.

The problem that this fixes is that integration tests do the following:

Install the aggregate public key in pox-4 for reward cycle 0

Mine 200-ish Bitcoin blocks (which is many reward cycles)

Mine some epoch 2.x blocks and instantiate pox-4 (more reward cycles)

Start epoch 3, and try to build a block

Without searching back to reward cycle 0, this last step would fail with a panic because (1) there's no way to "pull forward" the aggregate public key from reward cycle 0 when the first Stacks block is mined without this search code, and (2) epoch 2.x block-production doesn't use this code.

Remember, this whole code path goes away once aggregate public key voting comes online. Then, we'd have stackers vote for (and thus set) the aggregate public key in pox-4 in the penultimate reward cycle prior to epoch 3.0. This removes the need for this entire method.

What you described sounds incorrect to me— the epoch 3.0 instantiation code sets all pre-pox-4 cycles’ agg key. Then the set-agg-key method is invoked in every block after that. This loop is masking an issue.

Actually, it's the epoch 2.5 instantiation logic that populates the pre-pox-4 reward cycles' aggregate public key. And in the integration tests, epoch 2.5 starts at Bitcoin block height 6, which is well before the first Stacks block is mined.

testnet/stacks-node/src/run_loop/neon.rs

testnet/stacks-node/src/nakamoto_node/miner.rs

kantai · 2023-12-13T21:30:21Z

stackslib/src/chainstate/nakamoto/mod.rs

+                .ok_or(ChainstateError::DBError(DBError::NotFoundError))?;
+        let aggregate_key_block_header =
+            Self::get_canonical_block_header(chainstate.db(), sortdb)?.unwrap();
+


Can you add a todo comment here with a reference to #4109 ? As currently implemented here (which I think is fine for now) the at_block is variable: when a new stacks block is received, get_canonical_block_header() returns a different Stacks block, meaning this get_aggregate_public_key() will return different values for the same block at different times.

I'm actually no longer sure that #4109 is desirable, if we're going to allow stackers to re-try DKG in the middle of the cycle. If that were the case, then we'd want the current behavior.

Maybe so, but that merits discussion and that discussion shouldn't stall this PR. Leaving a pointer to that issue makes it so that it's easy to find the code that assumes one approach or the other.

As written, this function may be somewhat dangerous -- I don't think it's strange for a developer to believe that the expected aggregate key for a given block is always the same, but this function will return different results at different times.

kantai · 2023-12-13T21:31:11Z

stackslib/src/chainstate/nakamoto/mod.rs

@@ -1803,6 +1800,29 @@ impl NakamotoChainState {
        ))
    }

+    /// Get the aggregate public key for a block


Maybe just expand this comment that this is fetching the expected stacker aggregate key for a given block.

…egration test framework has mined some blocks

jferrant and others added 30 commits December 11, 2023 16:55

Create a boot contract to initialize pre-pox-4 aggregate key

f697611

Signed-off-by: Jacinta Ferrant <[email protected]>

Retrieve boot contract init agg key and set all pre-pox-4 cycles to it

8783eab

Signed-off-by: Jacinta Ferrant <[email protected]>

Only use the self_signer aggregate pub key for genesis blocks

24a16cd

Signed-off-by: Jacinta Ferrant <[email protected]>

Set the aggregate public key for the NEXT reward cycle in every block

cad57dc

Signed-off-by: Jacinta Ferrant <[email protected]>

Cleanup mine_stacks_block by pulling transaction construction into se…

76ccefa

…p functions Signed-off-by: Jacinta Ferrant <[email protected]>

CRC: add test to set and get the aggregate public key

03848a3

Signed-off-by: Jacinta Ferrant <[email protected]>

CRC: check that the initial aggregate key was set correctly

90dce75

Signed-off-by: Jacinta Ferrant <[email protected]>

Fix failed rebase by updating mockamoto tests.rs to use insert instea…

b79762a

…d of push Signed-off-by: Jacinta Ferrant <[email protected]>

replace manual set-aggregate-public-key test with one that checks to …

5998f3b

…see if the key is set automatically for a new reward cycle

execute private contract call using as_transaction/with_abort_callbac…

9865cb6

…k/execute_in_env context chain

Create a boot contract to initialize pre-pox-4 aggregate key

18fafdd

Signed-off-by: Jacinta Ferrant <[email protected]>

Retrieve boot contract init agg key and set all pre-pox-4 cycles to it

f2772d2

Signed-off-by: Jacinta Ferrant <[email protected]>

Only use the self_signer aggregate pub key for genesis blocks

949e0fd

Signed-off-by: Jacinta Ferrant <[email protected]>

Set the aggregate public key for the NEXT reward cycle in every block

c2f3c5f

Signed-off-by: Jacinta Ferrant <[email protected]>

Cleanup mine_stacks_block by pulling transaction construction into se…

ef6b7f4

…p functions Signed-off-by: Jacinta Ferrant <[email protected]>

CRC: add test to set and get the aggregate public key

e9f9974

Signed-off-by: Jacinta Ferrant <[email protected]>

CRC: check that the initial aggregate key was set correctly

76eea0a

Signed-off-by: Jacinta Ferrant <[email protected]>

Make sure thread exits before looking for transfer tx

96911d0

Signed-off-by: Jacinta Ferrant <[email protected]>

expand first nakamoto-neon test, update block commit logic to issue c…

7f0e1d4

…ommits at tenure_id changes, cargo fmt-stacks

feat: add boot_nakamoto to wrap the 2.x/3.x node handoff

25d1b52

add copyright headers, some code cleanup

b5bb4ac

chore: comments, cleanup unused functions

b84b483

chore: handle merge/rebase artifacts, address PR feedback

aa5ca43

remove unconfirmed tx handling in nakamoto RelayerThread

fc147df

add epoch-3.0 burnchain configuration assertions

16bb688

Merge branch 'feat/naka-neon' into feat/mockamoto-set-aggregate-publi…

56f5f1e

…c-key

Merge branch 'chore/make-set-agg-pubkey-private' into feat/mockamoto-…

5ed69d8

…set-aggregate-public-key

Merge branch 'next' into feat/mockamoto-set-aggregate-public-key

186f52c

jcnelson requested a review from jferrant December 13, 2023 15:13

jcnelson added 6 commits December 13, 2023 10:17

chore: put set-aggregate-public-key call into NakamotoChainState::set…

81c163f

…up_block() and call it on every block

fix: reward cycle prepare phase length of 3

4ce8f72

fix: get aggregate public key from sortition tip parent height

eb043c6

chore: delete old test -- we can't set the aggregate public key via a tx

bc5f9f0

chore: load aggregate public key from clarity

21aa307

chore: set aggregate public key smart contract on bootup

a4c2a7d

xoloki approved these changes Dec 13, 2023

View reviewed changes

jferrant reviewed Dec 13, 2023

View reviewed changes

jferrant approved these changes Dec 13, 2023

View reviewed changes

This was referenced Dec 13, 2023

Feat/mockamoto genesis block routine #4113

Closed

Make set-aggregate-public-key private #4126

Closed

jcnelson added 4 commits December 13, 2023 12:45

fix: when searching for the aggregate public key, search _all prior r…

3785db7

…eward cycles_ because integration tests can run in epoch 2.x for many reward cycles before the epoch 3 transition (and, the neon boot code sets the initial aggregate public key)

fix: install initial aggregate public key to neon, not nakamoto

0a6aa26

fix: search reward cycle 0 as well

e409b0a

fix: expect to wait up to 10 minutes for a block to be processed (not…

7a910e7

… 30s)

kantai reviewed Dec 13, 2023

View reviewed changes

stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved

Merge branch 'next' into feat/get-set-aggregate-public-key

5a29874

kantai reviewed Dec 13, 2023

View reviewed changes

testnet/stacks-node/src/run_loop/neon.rs Outdated Show resolved Hide resolved

kantai reviewed Dec 13, 2023

View reviewed changes

testnet/stacks-node/src/run_loop/neon.rs Outdated Show resolved Hide resolved

kantai reviewed Dec 13, 2023

View reviewed changes

testnet/stacks-node/src/nakamoto_node/miner.rs Outdated Show resolved Hide resolved

jcnelson requested a review from kantai December 13, 2023 20:02

chore: address PR feedback

ea7b165

kantai reviewed Dec 13, 2023

View reviewed changes

chore: address PR feedback and force epoch 2.5 to begin after the int…

b63ca21

…egration test framework has mined some blocks

jcnelson merged commit 8cffc38 into next Dec 14, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get/Set aggregate public key from Clarity #4165

Get/Set aggregate public key from Clarity #4165

jcnelson commented Dec 13, 2023

jcnelson commented Dec 13, 2023

codecov bot commented Dec 13, 2023 •

edited

Loading

xoloki left a comment

xoloki Dec 13, 2023

xoloki Dec 13, 2023

jferrant Dec 13, 2023

kantai Dec 13, 2023

jcnelson Dec 13, 2023 •

edited

Loading

jcnelson Dec 13, 2023

kantai Dec 13, 2023

jcnelson Dec 13, 2023

kantai Dec 13, 2023

jcnelson Dec 13, 2023

kantai Dec 13, 2023

kantai Dec 13, 2023

Get/Set aggregate public key from Clarity #4165

Get/Set aggregate public key from Clarity #4165

Conversation

jcnelson commented Dec 13, 2023

jcnelson commented Dec 13, 2023

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

xoloki left a 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 Dec 13, 2023 • edited Loading

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

codecov bot commented Dec 13, 2023 •

edited

Loading

jcnelson Dec 13, 2023 •

edited

Loading