Feat/exit at reward cycle contract #3035

pavitthrap · 2022-02-04T20:28:33Z

This PR implements vote to exit network contract & related logic. It is built off of the ideas in this discussion: #2845

Overview of protocol

Stackers vote for a reward cycle at which the node should next exit.
For a vote to gain consensus in a particular cycle:
- PoX must be active
- 50% of stackers must have voted in the exit contract
- Voting is aggregated as follows:
  - Before an exit height is chosen, a vote for RC N is interpreted as voting to exit at RC N or higher. If there is an equal number of votes for RC 10, 11, and 20, RC 11 will be selected as the exit height (those who voted for RC 10 will have their votes counted towards RC 11 in this example).
  - After some exit height H is chosen, only votes for block heights higher than H will be counted as valid in the election.
Once there is consensus for some reward cycle, the cycle after consensus was reached is a "veto" cycle.
Miners call the veto function to signal that they want to reject a particular proposal. 80% of the blocks in the veto reward cycle must contain a veto for the veto to pass.

Feedback

In process_ready_blocks and process_exit_reward_cycle, I currently do nothing if the node is unable to find the exit cycle info for a particular Stacks block. This should only happen for the first Stacks block. What should this behavior look like?

Future steps

Need to deploy the exit contract, and once I do so, I must "refer" to the contract differently within the code (issue Update contract identifier for exit contract #3034).
Need to think about whether miners will have difficulty getting their veto transactions in the current blocks. Might need to prioritize miner veto transactions?
Update SIP-015 draft to mention the contract

Testing

new tests in contract tests, chains coordinator tests, and added a neon_integrations test

Added contracts wip fixed veto process error in chainstate Integration test passing

… bug in db

gregorycoppola · 2022-02-09T16:04:28Z

Thanks for important change.

What about these test failures? Are they flakes or real?

https://github.com/stacks-network/stacks-blockchain/runs/5072277005?check_suite_focus=true

gregorycoppola

Lots of good code! Still reading. Had some interim comments.

src/burnchains/mod.rs

gregorycoppola · 2022-02-10T01:23:04Z

src/burnchains/mod.rs

@@ -320,6 +321,41 @@ pub struct PoxConstants {
    _shadow: PhantomData<()>,
 }

+#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)]


can comment explain which part of the code will use this?

use what? The struct or these derived functions?

src/chainstate/coordinator/mod.rs

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

src/chainstate/burn/db/sortdb.rs

jcnelson · 2022-02-11T19:45:48Z

src/chainstate/burn/db/sortdb.rs

+        self.execute(sql, args)?;
+
+        if let Some(exit_proposal) = exit_at_reward_cycle_info.curr_exit_proposal {
+            let sql =


Should this verify that exactly one row was set? Is it possible that this UPDATE touches zero rows?

The logic right before this inserts a row with that block_id, and it should fail if the insert failed?

Do you think an assertion is still necessary here?

src/chainstate/coordinator/mod.rs

jcnelson · 2022-02-14T02:44:49Z

src/chainstate/coordinator/mod.rs

+        if stacks_block_height_in_cycle == 2 {
+            // We want to process the exit-at-rc contract reward cycle for the reward cycle the
+            // current block's parent's parent belongs to since the height (in number of Stacks
+            // blocks) of the current block in its reward cycle is 2.


It's not clear to me why this is? Why not process the votes in the first block of a reward cycle, or even the last block?

Aaron felt it was a better idea to stall exit contract processing until after the first block of a reward cycle (just to avoid doing more computation in the first block of a reward cycle).

I don't do it in the last block of the reward cycle since that would invalidate the last veto.

Aaron felt it was a better idea to stall exit contract processing until after the first block of a reward cycle (just to avoid doing more computation in the first block of a reward cycle).

I'm not sure the trade-off is worth it here, though (cc @kantai). If you process the exit-at-rc logic at Stacks block height mod 2, then you have to introduce and test corner cases where you have a reward cycle with only one block. If we can instead make the PoX reward set calculation faster (something that's useful on its own anyway, and something that we could reasonably do), then the concern about doing it in the first reward cycle block goes away, and no corner cases are needed. This is all consensus-critical code, so if we can avoid less code even if it means a modest performance hit (or a need to spend some cycles optimizing code), then I think it's worth it. But would love to hear a strong counter-argument.

Let's discuss this in the architecture meeting

I can't come this week :/ Please let me know what y'all talk about

jcnelson · 2022-02-14T02:45:42Z

src/chainstate/coordinator/mod.rs

+        {
+            // This is an edge case in which the previous reward cycle contains only a single Stacks block.
+            // It is time to process the exit-at-rc contract for the reward cycle the current block's
+            // parent's parent belongs to.


If there's a reward cycle that contains only a single Stacks block, it might simplify things to simply declare that a vote tally will not occur. In such conditions, the network is already well and truly dead anyway.

The processing occurs for the parent cycle (which could be any length) so not sure if we should introduce that rule.

src/chainstate/coordinator/mod.rs

jcnelson · 2022-02-14T02:52:09Z

src/chainstate/stacks/boot/exit-at-rc.clar

+(define-constant MINIMUM_RC_BUFFER_FROM_PRESENT u6)
+
+;; Data vars
+(define-data-var pox-reward-cycle-length uint POX_REWARD_CYCLE_LENGTH)


Instead of duplicating this here, would it be better if the contract simply pulled this information from .pox via get-pox-info?

This also applies to first-burnchain-block-height below.

It would also be more efficient to store all of these vars as a single tuple data variable, since then reading it would incur only one MARF read. The initial values for these variables could be instantiated directly from contract-call?'s to .pox.

I avoided using get-pox-info since that function is defined as define-read-only (and is not callable).
I made them data vars to mimic the pattern in pox.clar - but I can change them to a tuple.

I think it could make sense to leave them separate since they are few in number, I would need a double get to get the value I require, and because I use them in different functions.

read-only methods are also callable by other contracts, so the initial values for these variables should be settable by doing something like (contract-call? .pox get-pox-info)

If these are settable via the pox get-pox-info call, then you should be able to switch them to constants (rather than data vars), which would simplify some of the contract code.

src/burnchains/mod.rs

src/chainstate/burn/db/sortdb.rs

jcnelson · 2022-04-13T19:05:03Z

src/chainstate/coordinator/mod.rs

+        let mut total_votes = 0;
+        let invalid_reward_cycles: HashSet<u64> =
+            HashSet::from_iter(invalid_reward_cycles.into_iter());
+        let is_mainnet = self.burnchain.is_mainnet();


This method is broken, but we can't fix it because it's consensus-critical. Burnchain::is_mainnet() always returns false, since it compares the Stacks network ID to the Bitcoin network ID (oops).

Can you add a docstring to Burnchain::is_mainnet() to say something to the effect of "BROKEN! DO NOT USE IN NEW PLACES!"?

Should I make a new is_mainnet_function (like is_mainnet_v2) and use that instead? In that function I would compare the Stacks network ID against BITCOIN_NETWORK_ID_MAINNET (aka BITCOIN_MAINNET).

It doesn't look like this value is ever used in this method -- I think just remove this variable.

src/chainstate/coordinator/mod.rs

jcnelson · 2022-04-13T19:21:37Z

src/chainstate/coordinator/mod.rs

+                }
+            }
+        } else {
+            warn!(


This is bound to happen at least once, right? When this PR goes live on existing chainstate, no block is going to have any reward cycle exit info.

Yes - let's discuss this in the arch meeting

src/chainstate/coordinator/mod.rs

…tinction between veto/vote.

…stead of per block; fixed some tests

jcnelson · 2022-04-15T18:26:59Z

Todo from architecture meeting:

Add to testnet checklist to instantiate this contract on testnet, and make sure the address is correct in the code
Consider deleting Burnchain::is_mainnet() (in a separate PR)
Need a good solution for deducing the exit reward cycle when starting a node from existing chainstate in the middle of a vote (in a separate PR)

kantai · 2022-05-06T16:25:10Z

src/chainstate/coordinator/mod.rs

+            .burnchain
+            .exit_contract_constants
+            .rejection_confirmation_percent;
+        let is_mainnet = self.burnchain.is_mainnet();


It doesn't look like is_mainnet is ever used in this method -- can be removed.

kantai · 2022-05-06T16:30:32Z

This looks good to me, I think the only todos are a few superficial changes I noted, and then a larger change to make it so that the contract isn't a boot contract in testnet. That will require changing some of the tests, but that should be doable by just:

Adding a contract publish transaction to the tests from some address that has funds in the tests.
Removing the contract from the testnet burnchain config.

I don't think we need to have a contract address for the testnet burnchain config in this PR: a contract can be launched to testnet after this PR merges, and a subsequent PR would set that contract in the config.

kantai · 2022-05-06T16:34:45Z

src/chainstate/coordinator/mod.rs

+                                .expect("BUG: failed to construct simple tuple"),
+                            ),
+                        )
+                        .expect("BUG: Failed querying rc-proposal-rejections")


Hmm. What happens if this is evaluated before the contract is launched? Does it panic?

jcnelson · 2023-02-07T16:25:03Z

We'll make the call on the future of this PR at the next architecture meeting.

blockstack-devops · 2024-11-14T00:20:13Z

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.

pavitthrap added 14 commits February 3, 2022 11:30

WIP- need to get sortition DB to open wo error; pox active when expected

462e207

Added contracts wip fixed veto process error in chainstate Integration test passing

update test: WIP

ef6ea4d

Test: fixed issue with test_make_boot_contracts_reference

82f4925

updated test to properly exit if node reaches exit reward cycle

777db95

Docs: added docs to exit at rc contract'

537251e

feat: made absolute minimum reward cycle for exit configurable; fixed…

29ba1b9

… bug in db

nit: altered comments

61afe33

Added delay to rc processing; fork test is wip

52592e6

test: cc test wip, and is passing

7046941

voting working in test

2c73a96

Finished forking test

d8220a8

Chore: updated tests

90b3dc9

Clean up code

3a55c23

Clean up integration test

3bf9efe

pavitthrap requested review from jcnelson, kantai and gregorycoppola February 4, 2022 20:28

gregorycoppola reviewed Feb 10, 2022

View reviewed changes

jcnelson reviewed Feb 11, 2022

View reviewed changes

src/chainstate/burn/db/sortdb.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 11, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 11, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 14, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 14, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 14, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 14, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Feb 14, 2022

View reviewed changes

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/burnchains/mod.rs Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/burnchains/mod.rs Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/burn/db/sortdb.rs Outdated Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Show resolved Hide resolved

jcnelson reviewed Apr 13, 2022

View reviewed changes

src/chainstate/coordinator/mod.rs Outdated Show resolved Hide resolved

pavitthrap added 3 commits April 13, 2022 17:17

Minor change: renamed veto to reject in order to create more of a dis…

7fbd17e

…tinction between veto/vote.

Changed exit block info such that one is created per reward cycle, in…

5e36a72

…stead of per block; fixed some tests

Added comments

bb75e63

kantai self-requested a review May 6, 2022 15:27

kantai reviewed May 6, 2022

View reviewed changes

This was referenced Jun 10, 2022

Feat/pox getters #3166

Merged

PoX Updates: .pox-info contract for native information getters #2538

Closed

jcnelson added stacks-future frozen PRs that are on hold and removed stacks-2.1 labels Jul 11, 2022

jcnelson closed this Feb 7, 2023

blockstack-devops added the locked label Nov 14, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/exit at reward cycle contract #3035

Feat/exit at reward cycle contract #3035

pavitthrap commented Feb 4, 2022 •

edited

Loading

gregorycoppola commented Feb 9, 2022

gregorycoppola left a comment

gregorycoppola Feb 10, 2022

pavitthrap Mar 14, 2022

jcnelson Feb 11, 2022

pavitthrap Mar 17, 2022

jcnelson Feb 14, 2022

pavitthrap Mar 14, 2022

jcnelson Mar 16, 2022

pavitthrap Mar 17, 2022

jcnelson Mar 17, 2022

jcnelson Feb 14, 2022

pavitthrap Mar 14, 2022

jcnelson Feb 14, 2022

jcnelson Feb 14, 2022

jcnelson Feb 14, 2022

pavitthrap Mar 15, 2022 •

edited

Loading

kantai Apr 13, 2022

kantai Apr 13, 2022

jcnelson Apr 13, 2022

jcnelson Apr 13, 2022 •

edited

Loading

pavitthrap Apr 13, 2022 •

edited

Loading

kantai May 6, 2022

jcnelson Apr 13, 2022

pavitthrap Apr 14, 2022

jcnelson commented Apr 15, 2022

kantai May 6, 2022

kantai commented May 6, 2022

kantai May 6, 2022

jcnelson commented Feb 7, 2023

blockstack-devops commented Nov 14, 2024

Feat/exit at reward cycle contract #3035

Feat/exit at reward cycle contract #3035

Conversation

pavitthrap commented Feb 4, 2022 • edited Loading

Overview of protocol

Feedback

Future steps

Testing

gregorycoppola commented Feb 9, 2022

gregorycoppola 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

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

pavitthrap Mar 15, 2022 • 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

jcnelson Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

pavitthrap Apr 13, 2022 • 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

jcnelson commented Apr 15, 2022

Choose a reason for hiding this comment

kantai commented May 6, 2022

Choose a reason for hiding this comment

jcnelson commented Feb 7, 2023

blockstack-devops commented Nov 14, 2024

pavitthrap commented Feb 4, 2022 •

edited

Loading

pavitthrap Mar 15, 2022 •

edited

Loading

jcnelson Apr 13, 2022 •

edited

Loading

pavitthrap Apr 13, 2022 •

edited

Loading