Implement approval store#384
Conversation
yacovm
force-pushed
the
reconfig-6
branch
3 times, most recently
from
bbd66e3 to
2c54008
Compare
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Also self-sign during aggregation - Introduces ApprovalStore (msm/approvals.go) — an in-memory store of validator approvals for epoch transitions, keyed by (NodeID, PChainHeight). It verifies BLS signatures on ingest, deduplicates by timestamp (newer wins, older is dropped), and prunes per-node entries to at most len(validators) by evicting the oldest timestamp. - Adds Timestamp to ValidatorSetApproval (msm/encoding.go) so the store can order/evict approvals deterministically. - computeNewApprovals (msm/msm.go) now optimistically self-signs the next epoch's P-chain reference height each round and appends its own ValidatorSetApproval to the peer set since the store deduplicates it later. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
|
|
||
| // SignatureVerifier verifies a cryptographic signature against a message and public key. | ||
| // Used to verify Approvals from validators for epoch transitions. | ||
| type SignatureVerifier interface { |
There was a problem hiding this comment.
i just realized we have this interface already defined in simplex.api. thoughts on removing this and just using the simplex interface(we already do this for the Signer interface)?
There was a problem hiding this comment.
But the interface we have there takes in a signer []NodeID where here we take a public key instead.
| WaitForPendingBlock(ctx context.Context) | ||
| } | ||
|
|
||
| type verificationInput struct { |
There was a problem hiding this comment.
| bs := make(blockStore) | ||
| bs[0] = &outerBlock{block: genesisBlock} | ||
|
|
||
| var myNodeID nodeID |
There was a problem hiding this comment.
is there any benefit of making this random?
| var myID nodeID | ||
| copy(myID[:], sm.MyNodeID) | ||
| validators := NodeBLSMappings{ | ||
| {NodeID: myID, BLSKey: []byte{1}, Weight: 1}, |
There was a problem hiding this comment.
nit
validators := NodeBLSMappings{
{NodeID: nodeID(sm.MyNodeID), BLSKey: []byte{1}, Weight: 1}also did github remove the suggestions option on pr review? can't seem to find it
|
|
||
| type approvalsByPChainHeight map[uint64]*ValidatorSetApproval | ||
|
|
||
| type ApprovalStore struct { |
There was a problem hiding this comment.
Is this supposed to be passed into the MSM somewhere? I don't see it connected anywhere outside of tests
There was a problem hiding this comment.
Yeah, this is what implements:
// ApprovalsRetriever retrieves the approvals from validators of the next epoch for the epoch change.
type ApprovalsRetriever interface {
Approvals() ValidatorSetApprovals
}
| } | ||
| } | ||
|
|
||
| func (as *ApprovalStore) checkApprovalSignature(approval *ValidatorSetApproval, pk []byte) error { |
There was a problem hiding this comment.
hmm is it not a problem that we verify the signature is valid over just the pChainHeight here, but in the MSM code we verify it with the context + asn1 encoding? shouldn't there be one canonical way of verifying/signing that is shared for both of them?
There was a problem hiding this comment.
Yeah, because when I implemented this, I still haven't done that domain separation in the MSM.
Will fix 👍
| return approvals | ||
| } | ||
|
|
||
| func (as *ApprovalStore) HandleApproval(approval *ValidatorSetApproval) error { |
There was a problem hiding this comment.
where is HandleApproval going to be called? Do we need to make this handling method thread safe?
There was a problem hiding this comment.
I will connect it somehow once I build the orchestration layer.
For now, I just implemented this logic without paying attention to thread safety as it's not connected to anything yet
| binary.BigEndian.PutUint64(pChainHeightBuff, pChainHeight) | ||
|
|
||
| // We check if the signature is valid before we store the approval. | ||
| return as.signatureVerifier.VerifySignature(approval.Signature, pChainHeightBuff, pk) |
There was a problem hiding this comment.
we dont we also check the signature is correct over the AuxInfoSeqDigest? I guess what is stopping nodes from agreeing on the pchain signature, but having different hashes in AuxInfoSeqDigest
There was a problem hiding this comment.
That's a good question.
So, it's true that in the spec I wrote that we check the signature on both the P-chain height and the AuxInfoDigest.
However, since you asked to remove the code that handles the AuxInfo from the MSM PR and I have yet to introduce it back, I am signing for now only on the P-chain height.
| ) | ||
|
|
||
| func makeNodeID(seed byte) nodeID { | ||
| var n nodeID |
There was a problem hiding this comment.
i'm just realizing this now but nodeID will get very annoying imo. we already have simplex.NodeID and now we are adding nodeID which is different. Maybe in a separate pr can we change nodeID to something like avagoNodeID and merge that pr in before this one?
There was a problem hiding this comment.
Sure we can align the NodeID definitions.
But why do that before merging this PR? I think it'll be easier just doing it afterwards, no?
We anyway will need to do this before the repo is swallowed by avalanchego
| require.Equal(t, 0, as.storedCount) | ||
| } | ||
|
|
||
| func TestApprovalStoreHandleApprovalInvalidSignature(t *testing.T) { |
There was a problem hiding this comment.
thoughts on #392
feel free to close/merge/implement
2 participants
Also self-sign during aggregation
ingest, deduplicates by timestamp (newer wins, older is dropped), and prunes per-node entries to at most len(validators) by evicting the oldest timestamp.