Paging in StackerDB and update to .signers #4323
Conversation
* expand e2e correct_burns test to invoke the new RPC endpoint * fix reward-set storage information to write correct coinbase height
Paging supports larger StackerDB instances
* make signer-slots a const in stacks-common * update the .signers maintenance to separate voting share from StackerDB slots * update .signers to use and alternate 2 pages of signers
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## next #4323 +/- ##
==========================================
+ Coverage 76.73% 83.03% +6.29%
==========================================
Files 446 446
Lines 318774 319256 +482
==========================================
+ Hits 244607 265088 +20481
+ Misses 74167 54168 -19999 ☔ View full report in Codecov by Sentry. |
kantai
force-pushed
the
feat/reward-set-endpoint
branch
from
cda02b1 to
c6a54db
Compare
|
This will help me so much for cast aggregate vote PR...have been struggling with making things work the way they were and seeing all these changes makes me realize why lol :P |
| @@ -2511,7 +2511,7 @@ impl NakamotoChainState { | |||
| Self::calculate_matured_miner_rewards( | |||
| &mut clarity_tx, | |||
| sortition_dbconn.sqlite_conn(), | |||
| coinbase_height, | |||
| coinbase_height + 1, | |||
There was a problem hiding this comment.
@kantai Can you explain why this change needed to be made?
| @@ -2511,6 +2511,7 @@ impl NakamotoChainState { | |||
| Self::calculate_matured_miner_rewards( | |||
| &mut clarity_tx, | |||
| sortition_dbconn.sqlite_conn(), | |||
| // coinbase_height + 1, | |||
There was a problem hiding this comment.
@kantai I had to comment this out because it was causing the chain to crash in some of the integration tests. Can you please help me understand why this change was here in the first place?
There was a problem hiding this comment.
There were some tests where the coordinator panicked without this change (I think they were signers tests that used mockamoto, or some configuration where nakamoto was active during very early Stacks block heights). This was because of some assumptions about the block height, maturation windows, and the size of the matured rewards. I don't think +1 here is correct, but I do think that the height used for calculated miner rewards maturation in nakamoto needs another look.
The coinbase_height parameter of NakamotoChainState::setup_block includes the block being set up. The chain_tip parameter of StacksChainState::setup_block, however, is the parent tip, so that the stacks height used in this function is logically -1. This is why StacksChainState uses +1 when invoking handle_prepare_phase_start(). This was discovered in trying to test that method, but I didn't try to completely test all of the behavior around these height parameters. It may be that the first block of a tenure has the parent's height passed in, but subsequent blocks in that tenure have the new coinbase height (where we could see something like #4333).
| @@ -2511,6 +2511,7 @@ impl NakamotoChainState { | |||
| Self::calculate_matured_miner_rewards( | |||
| &mut clarity_tx, | |||
| sortition_dbconn.sqlite_conn(), | |||
| // coinbase_height + 1, | |||
There was a problem hiding this comment.
There were some tests where the coordinator panicked without this change (I think they were signers tests that used mockamoto, or some configuration where nakamoto was active during very early Stacks block heights). This was because of some assumptions about the block height, maturation windows, and the size of the matured rewards. I don't think +1 here is correct, but I do think that the height used for calculated miner rewards maturation in nakamoto needs another look.
The coinbase_height parameter of NakamotoChainState::setup_block includes the block being set up. The chain_tip parameter of StacksChainState::setup_block, however, is the parent tip, so that the stacks height used in this function is logically -1. This is why StacksChainState uses +1 when invoking handle_prepare_phase_start(). This was discovered in trying to test that method, but I didn't try to completely test all of the behavior around these height parameters. It may be that the first block of a tenure has the parent's height passed in, but subsequent blocks in that tenure have the new coinbase height (where we could see something like #4333).
…work/stacks-core into feat/larger-stackerdb
|
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. |
Description
This adds paging to StackerDB (it also converts a lot of
expects()to guarded errors: I'm pretty sure those weren't reachable, but easy enough to be a little paranoid with those).It also updates the.signers(and the downstream.signer-voting) contracts to use paging to store the alternate signers sets. As part of this, the.signerscontract changed some: the StackerDB slots are stored separately from the signer voting weights: the prior implementation incorrectly conflated the two.EDIT (@jcnelson): I'll be taking over this PR. The system will implement multiple StackerDBs for signers -- one per message type -- and will support running two signer-sets at once.
This implements #4307 and #4315