Move approval store into MSM#429
Conversation
This commit moves the approval store into the MSM, instead of being an external dependency. The reason is that the approval store needs to be initialized with the validator set of the *next* epoch and not the *current epoch*, which can only be computed inside the MSM. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
| @@ -27,8 +28,12 @@ type ApprovalStore struct { | |||
| validators NodeBLSMappings | |||
| logger common.Logger | |||
| pkByNodeID map[nodeID][]byte | |||
There was a problem hiding this comment.
unrelated but should we change this to nodeIDToPK. we already have validatorsToPKs map[string][]byte in epoch.go
There was a problem hiding this comment.
not sure I follow, what is the problem with the field name?
| // the block builder reads the accumulated approvals when building a block. | ||
| lock sync.RWMutex | ||
| approvalsByNodes map[nodeID]approvalsByPChainHeightAndAuxInfoDigest | ||
| storedCount int |
There was a problem hiding this comment.
why do we need the storedCount field? can't we just calculate it by summing the lengths in approvalsByNodes?
There was a problem hiding this comment.
nvm i guess it would be inefficient to iterate and find exactly how many entries there are, and probably hard to get an accurate estimate
There was a problem hiding this comment.
It's so we can allocate the exact size in Approvals() and avoid memory copying
There was a problem hiding this comment.
unrelated: can we remove the getPKOfNode() function? it seems unnecessary
| // Snapshot the approvals under our lock, then hand them to the destination | ||
| // store (which takes its own lock). Copying first avoids holding two store | ||
| // locks at once. | ||
| as.lock.Lock() |
There was a problem hiding this comment.
we can just grab the read part of the lock rather than the entire lock
There was a problem hiding this comment.
yes, I can change it accordingly.
Though you know, when this is called, the approval store has a few micro seconds left to live 💀
| // store (which takes its own lock). Copying first avoids holding two store | ||
| // locks at once. | ||
| as.lock.Lock() | ||
| type approvalWithTimestamp struct { |
There was a problem hiding this comment.
why do we create a new struct? can we use approvalAndTimestamp?
| tc.verify(t, dst, src, tc.srcApprovals, tc.dstApprovals) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
should we add one where the timestamp is equal
{
// A source approval with the same timestamp as an existing destination approval at the same
// (NodeID, PChainHeight) does not overwrite it: the >= tie in approvalExistsAndUpToDate goes
// to the incumbent.
name: "equal-timestamp source does not overwrite destination",
srcValidators: 2,
dstValidators: 2,
dstApprovals: []approvalAndTimestamp{
{ValidatorSetApproval{NodeID: makeNodeID(1), PChainHeight: 7, Signature: signApproval(7, [32]byte{})}, 100},
},
srcApprovals: []approvalAndTimestamp{
{ValidatorSetApproval{NodeID: makeNodeID(1), PChainHeight: 7, Signature: signApproval(7, [32]byte{})}, 100},
},
verify: func(t *testing.T, dst, _ *ApprovalStore, _, dstSent []approvalAndTimestamp) {
got := dst.Approvals()
require.Len(t, got, 1)
require.Equal(t, 1, dst.storedCount)
require.Equal(t, dstSent[0].ValidatorSetApproval, got[0], "the destination approval is retained on a timestamp tie")
},
},| errValidatorSetUnchanged, next.NextPChainReferenceHeight, prev.PChainReferenceHeight) | ||
| } | ||
|
|
||
| // Else, ! currentValidatorSet.Equal(newValidatorSet) || next.NextPChainReferenceHeight == 0 |
There was a problem hiding this comment.
| // Else, ! currentValidatorSet.Equal(newValidatorSet) || next.NextPChainReferenceHeight == 0 | |
| // Else, !currentValidatorSet.Equal(newValidatorSet) || next.NextPChainReferenceHeight == 0 |
| if decisionToBuildBlock.transitionEpoch && isSealingBlockFinalized { | ||
| sm.Logger.Debug("Transitioning epoch after building block", zap.Uint64("newPChainRefHeight", decisionToBuildBlock.pChainHeight)) | ||
| newSimplexEpochInfo.NextPChainReferenceHeight = decisionToBuildBlock.pChainHeight | ||
| sm.maybeInitializeApprovalStore(decisionToBuildBlock.validatorSet) |
There was a problem hiding this comment.
but if the sealing block is finalized, we must have already computed an approval store. furthermore, since this is for the next epoch aren't these validators not the right validators anyways? we want the validator set for the next, next epoch which we can't know since we are only transitioning to the next epoch.
There was a problem hiding this comment.
This is the sealing block of the previous epoch, not this epoch.
The sealing block of this epoch hasn't been created yet.
since this is for the next epoch aren't these validators not the right validators anyways?
I changed shouldBuildBlock to return the validator set at decisionToBuildBlock.pChainHeight which is written into newSimplexEpochInfo.NextPChainReferenceHeight.
we want the validator set for the next, next epoch which we can't know since we are only transitioning to the next epoch.
This function determines the P-chain reference height for the next epoch, and that's the validator set we are seeding the approval store with.
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
2 participants
This commit moves the approval store into the MSM, instead of being an external dependency.
The reason is that the approval store needs to be initialized with the validator set of the next epoch and not the current epoch, which can only be computed inside the MSM.