-
Notifications
You must be signed in to change notification settings - Fork 661
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
Nakamoto: Add stacker/signer bit vector and event #4269
Conversation
…ently failing: need to update to set for all pox-4 cycles
chainstate.get_reward_addresses_in_cycle(burnchain, sortdb, cycle, block_id)?; | ||
|
||
// TODO (pox-4-workstream): the pox-4 contract must be able to return signing keys |
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.
Is there an issue for this? Because this seems to be pretty high-priority!
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.
There's a lot of ongoing discussion of this in the open .signers PR, I'm not sure that it has its own issue yet, but its definitely top of mind there.
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.
Opened an issue: #4290
let reward_set = | ||
StacksChainState::make_reward_set(threshold, registered_addrs, cur_epoch.epoch_id); | ||
if reward_set.signers.is_none() { | ||
error!("FATAL: PoX reward set did not specify signer set in Nakamoto"); |
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.
Is this truly fatal? If so, then should it panic?
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 its as fatal as non-participation, so it should be treated the same.
@@ -313,6 +316,8 @@ pub struct NakamotoBlockHeader { | |||
pub miner_signature: MessageSignature, | |||
/// Schnorr signature over the block header from the signer set active during the tenure. | |||
pub signer_signature: ThresholdSignature, | |||
/// A bitvec which represents the signers that participated in this block signature. | |||
pub signer_bitvec: BitVec, |
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'm a little skeptical that we should be using the generic BitVec
here. Yes, we need a bitmap of some kind, but I think we also need a hard bound on how big this is allowed to be (e.g. there can be no more than 4000 bits). And, I think that bound needs to be expressed in the data type itself, so it's impossible to represent a block header that would exceed this bound (and thus it's impossible to both serialize and deserialize such a block).
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 added a generic const to the BitVec
struct for MAX_SIZE
. The field in the block header sets this to 4000.
let signing_key = if let Some(PrincipalData::Standard(s)) = entry.signing_key.clone() { | ||
StacksAddress::from(s) | ||
} else { | ||
// TODO: should figure out if in mainnet? |
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
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'd argue this is purely a code-aesthetic choice, really. This branch is only reached if the signing_principal
set in the pox-4 contract is a contract principal, and the important thing is just that the address is unusable, which the burn address will be regardless of version bit.
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 is actually moot now that we know from @jferrant that we need the keys themselves.
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 this is on the right track. Just a few comments.
…al, fix test to prepare for merge with #4269
…al, fix test to prepare for merge with #4269
…al, fix test to prepare for merge with #4269
…s, update end to end test
…pdate anchor block selection, fix miner issue
serialize_with = "addr_serialize", | ||
deserialize_with = "addr_deserialize" | ||
)] | ||
pub signing_address: StacksAddress, |
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.
Per @jferrant, it turns out that this actually needs to be a public key -- DKG needs the public keys before it can begin.
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.
My subsequent review is blocked on the need for the signer set to track public keys instead of principals.
Okay -- this PR has now been updated with the work from #4295: the reward set is written to The signer keys are stored in the reward set (not the stacking-state map) as These signer keys have a many-to-many relationship with PoX addresses. Each reward set entry points to one signing key, but a PoX address can appear multiple times in the reward set entries, and different entries can point to the same signing key. The |
@@ -2408,6 +2409,10 @@ impl EventKeyType { | |||
return Some(EventKeyType::BlockProposal); | |||
} | |||
|
|||
if raw_key == "stacker_set" { |
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 see we have the means to subscribe to stacker sets, but this doesn't help a signer that spins up after stackers have registered. Orthogonal to this PR...Should I create a ticket to add this as an rpc endpoint as we will need to configure the signers with it?
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, please open a ticket -- it should be simple enough to add in a follow-up PR.
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.
Done #4298
Aside from the failed build due to a missing bitvec in the one file, LGTM |
@@ -103,6 +105,15 @@ pub struct MinedNakamotoBlockEvent { | |||
pub tx_events: Vec<TransactionEvent>, | |||
} | |||
|
|||
#[derive(Clone, Debug, Serialize, Deserialize)] | |||
pub struct PoxAnchorBlockEvent { |
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.
@kantai did you intend to include this in the event payload? It looks to be unused right now
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.
Ah -- no, at one point I was going to, but it would have required some cloning of sizable structs (the reward set). Removed in 99730e0.
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.
That data looked really helpful though! Right now the stacker list data in the event is just this this right?
pub struct RewardSet {
pub rewarded_addresses: Vec<PoxAddress>,
pub start_cycle_state: PoxStartCycleInfo,
}
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'll also contain:
// only generated for nakamoto reward sets
pub signers: Option<Vec<NakamotoSignerEntry>>,
Which will contain signing keys, stacked amount for each signing key, and the number of signing slots.
Description
This PR implements a handful of things:
BitVec
class to stacks-common. This PR is at least the third implementation of a bit vector in the codebase, so it seemed like adding a simple data struct would be useful here.BitVec
field to theNakamotoBlockHeader
, this is included in the consensus serialization and the stacker/signer signature hash. It is not included in the miner's signature hash.new_pox_set
endpoint for the event dispatcher. This endpoint announces when a reward set is calculated in a PoX anchor block. It relays information about the reward set:make_signer_set
, which is invoked bymake_reward_set
to translate the list ofRawRewardEntry
s into signing set entries. Currently, it does this by summing all entries with the same public key and assigning slots based on the slot thresholds.nakamoto_get_reward_set()
nakamoto
reward set fetching and2.x
: any reward set that is used by Nakamoto should be fetched via thenakamoto
reward set endpoint. The first nakamoto reward set will occur in epoch 2.5.correct_burn_outs
-- this integration test wasn't asserting that the sortition db advanced, so it missed the fact that the recent update innext
to require signing keys (and unique ones) broke the test. The test now asserts the sortition db advances and it works.correct_burn_outs
test to check the results of thenew_pox_set
event observer API are correct.Still needs test coverage
make_signer_set
needs unit testing. This is easy enough to add and I will add it before this merges, but before I did that, I wanted to get feedback on this PR because ifmake_signer_set
needs to change, it seems like a waste to write the unit tests now.Relevant issues
#4241 - this is the
stacks-node
side of tracking. this PR doesn't implement the punishment for non-participation, though.#4256 - this PR emits this list
#4255 - summing over the signer set entries in this PR gives the amount of active lockup. the amount of total lockup (which could include people who miss the threshold) would need to be added as another field in the return. This should be simple to add and I can do it if its useful (@zone117x).