-
Notifications
You must be signed in to change notification settings - Fork 647
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
modify nakamoto block header to use Vec<MessageSignature>
#4781
base: develop
Are you sure you want to change the base?
Conversation
6abb574
to
8c92e2d
Compare
8c92e2d
to
fe3ca6f
Compare
fe3ca6f
to
d14e5b4
Compare
@jcnelson I think that's possibly the right idea, but I'm finding it to be actually pretty reasonable to swap the testing infra without really messing it up for when we go back to threshold sigs. There are a few places (ie I have a number of TODO comments in this PR, which are mainly related to where you'd need to do epoch-gated behavior. I'd love to see what you think about how to proceed given my first attempt at this. |
@jferrant @kantai @obycode tagging you in as I think this PR is in a good place to get a review. I'll want to see a full CI run to see exactly what's still TBD, but a lot of the initial pieces are in place. I've also rebased this on top of #4788, as that includes some type changes that are utilized. |
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 LGTM -- just a few comments. Exciting stuff!
@@ -1696,7 +1784,8 @@ impl NakamotoChainState { | |||
db_handle: &mut SortitionHandleConn, | |||
staging_db_tx: &NakamotoStagingBlocksTx, | |||
headers_conn: &Connection, | |||
aggregate_public_key: &Point, | |||
_aggregate_public_key: &Point, |
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.
You can remove this argument, and just note in the comment below that it will be required in the future.
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 made this an Option
(still unused), as I'm not sure whether this should or shouldn't be a place where we want to refactor later (given how it's used downstream).
); | ||
block.header.signer_signature = ThresholdSignature(signature); | ||
/// Reorder a list of signatures to match the order of the reward set. | ||
pub fn reorder_signatures( |
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 rather confused as to why this method and its caller (sign_block_with_reward_set
) need to exist? If a block is signed by iterating through the reward set's private keys, then it would always be the case that the signatures are in order. No one should be trying to check a block's signatures against a different reward set than the one that signed it, so sign_block_with_reward_set
as written seems buggy. If the test wants to sign a block with a different reward set than the one configured in this TestSigners
instance, then perhaps the test should create a TestSigners
for that reward 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.
Yeah I understand the confusion. For one, this function isn't signing the block using the reward set's keys (the RewardSet
also doesn't have private keys), it's signing the block using TestSigners
's keys and using the reward set to separately:
- (most importantly) not sign the block with any TestSigner keys that aren't in the reward set
- reorder signatures to match the reward set
For a use case, imagine a testing scenario like this:
- Set up your test with
TestSigners(alice, bob)
- Next, stack with just bob
- Sign some blocks
- Later stack with alice
- Sign some blocks
In that cash, TestSigners
would appropriately only sign blocks using the correct signers at that time. You could do this by manually changing test_signers.signer_keys
, too, but TestSigners
is generally just used for one-time test setup and then passed around.
} | ||
|
||
// Sort [`Self::signer_keys`] by their compressed public key | ||
pub fn sorted_signer_keys(&self) -> Vec<Secp256k1PrivateKey> { |
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 does this method need to exist? I thought blocks were signed by their respective signers in reward-set order?
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.
You could do reward-set order (for each in reward set, lookup if test signer has that key, sign), but TestSigners
is kind of doing the opposite (for each key in signer_keys, sign, then reorder/remove based on reward set). I can see how that's messy though, I'll do it the other way. Also, this specific function isn't even used. I probably used it at some point but I'll remove 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.
I removed this and refactored the other methods to do the (for each key in reward_set, sign if TestSigner has that key) approach. LMK if you think this needs more work
…aggregate public keys
…t the block in reward cycle N at reward cycle index 0 was signed by the signers of reward cycle N - 1
…t an extension of a previously-started tenure, or a newly-started tenure), and clarify that the existing API for getting the highest Nakamoto tenure only pertains to the highest *started* tenure (not extended)
true, | ||
self.burnchain.pox_constants.clone(), | ||
) | ||
.expect("FATAL: could not open sortition DB"); |
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 make these fatal. Just return an error; the miner will try again on its own.
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.
&new_block.header.consensus_hash, | ||
) | ||
.expect("FATAL: could not retrieve chain tip") | ||
.expect("FATAL: could not retrieve chain tip"); |
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.
Again, please don't make this fatal.
…ed reward set of the anchor block is not yet known
This builds on top of #4778, to try and reduce duplicate work and conflicts.
The
verify_signer_signatures
function has a few rules:RewardSet
and using 70% as the thresholdTracking the work included in this PR:
signer_signature
field inNakamotoBlockHeader
to beVec<MessageSignature>
verify_signer_signatures
method toNakamotoBlockHeader
with testsaccept_block
TestSigners
andTestSigningChannel
to sign the block using the new formatblind_signer
related code for Nakamoto integrationsWhat I think (?) is out of scope:
TestSigningChannel