Conversation
68a788d to
d8290b2
Compare
| ) | ||
|
|
||
| func TestInstanceMixedNodeType(t *testing.T) { | ||
| t.Skip("skipping until test instance refactor") |
There was a problem hiding this comment.
im skipping these because i didn't want to spend time updating the tests since i have them passing here
yacovm
left a comment
There was a problem hiding this comment.
Made a first pass.
The most important comment is regarding resetting the aux info store between epochs.
| // Can be used for backward-compatibility and upgrade purposes. | ||
| Version VersionID `canoto:"uint,1"` | ||
|
|
||
| // Info is opaque bytes that can be used by applications to encode any information that describes |
There was a problem hiding this comment.
Info is opaque bytes that can be used by applications to encode any information that describes the current state for the application.
That's the most ambiguous definition I've read for quite some time, but I can only blame myself.
|
|
||
| // AuxiliaryInfo defines application-specific information for applications that might care about epoch change, | ||
| // such as threshold distributed public key generation. | ||
| type AuxiliaryInfo struct { |
There was a problem hiding this comment.
We should add an epoch field here, otherwise we may get a misunderstanding if a node is lagging and sends information regarding the wrong epoch.
There was a problem hiding this comment.
shouldn't this be handled by Data and the auxiliary info application?
I think the only source of truth for whether AuxiliaryInfo is valid should be the IsLegalAppend function call. It shouldn't be up to the msm/instance to filter aux infos. If the application cared about epochs, they can encode that information in Data or maybe encode a timestamp.
There was a problem hiding this comment.
But there is no point in putting stuff into memory that if that stuff belongs to the wrong epoch.
The framework should do its best to provide reliable data delivery.
If the node is correct but it's just behind, we should exclude its contribution at the protocol layer, not at the app layer.
There was a problem hiding this comment.
ok, will add
i still think a malicious node can bypass this by encoding a different epoch. Therefore, we would still need some sort of check in the application layer
There was a problem hiding this comment.
added the epoch to the struct, but I'll set up blocking old auxiliary info for when I wire it into the Instance since the MSM isn't aware of the current epoch.
There was a problem hiding this comment.
i still think a malicious node can bypass this by encoding a different epoch. Therefore, we would still need some sort of check in the application layer
Yeah but we should offload protection of malicious input to the application, but not let correct but stale input be regarded as malicious
| a.lock.Lock() | ||
| defer a.lock.Unlock() | ||
|
|
||
| // Iterate in node ID order so the returned entries are deterministic. |
There was a problem hiding this comment.
why do we need it to be deterministic? So that anyone verifying this would iterate in the same order when applying IsLegalAppend?
There was a problem hiding this comment.
it's helpful for testing. i don't think it matters for production, because this is only called by the block builder.
| return asn1.Marshal(signedMsg) | ||
| } | ||
|
|
||
| func SignApproval(signer common.Signer, nextPChainReferenceHeight uint64, auxInfoDigest [32]byte) ([]byte, error) { |
There was a problem hiding this comment.
what's the reason this is exported? This was createSelfApproval before.
There was a problem hiding this comment.
it's used in a future PR
There was a problem hiding this comment.
| VerifiedBlockMessage *VerifiedBlockMessage | ||
| VerifiedReplicationResponse *VerifiedReplicationResponse | ||
|
|
||
| // Epoch Transition Messages |
There was a problem hiding this comment.
I'm missing the part where we send these two...?
There was a problem hiding this comment.
also in a future PR 😉
yacovm
left a comment
There was a problem hiding this comment.
Some more comments. Will make another pass after they're addressed.
| } | ||
|
|
||
| if err := a.app.IsLegalAppend(info.Version, validators, legalHistory, info.Data); err != nil { | ||
| // we don't remove this info from the mempool. maybe it can be added in a different block |
There was a problem hiding this comment.
This is a good decision, because DKG material is linear in the size of the validator set, so we may not be able to add more than a single entry per block.
| if auxInfoHistory.OldestVersionID != info.Version { | ||
| return nil, [32]byte{}, false, fmt.Errorf("proposed auxiliary info does not have the proper version %d: %w", auxInfoHistory.OldestVersionID, err) | ||
| } | ||
| if err := sm.AuxiliaryInfoApp.IsLegalAppend(auxInfoHistory.OldestVersionID, validators, auxInfoHistory.Data, info.Data); err != nil { |
There was a problem hiding this comment.
This just checks that adding info is allowed if we just add this info.
But it might be that adding the first info is allowed on its own, adding the second info is allowed on its own, but adding the first info and then the second info is forbidden.
For example, if proposedAuxInfos contains several info that are the same one, we should only be able to add the first one, not the rest.
We need to accumulate the history as we go through the auxiliary info we check are legal appends.
There was a problem hiding this comment.
ah good catch, we need to have some better testing with the auxiliary info stuff. maybe in some future prs or when DKG is added we can create some better aux tests
| if ai.data != nil { | ||
| cloned.data = make([]common.AuxiliaryInfo, len(ai.data)) | ||
| for i, entry := range ai.data { | ||
| entry.Data = slices.Clone(entry.Data) |
There was a problem hiding this comment.
There is no reason to clone the data. We are cloning to avoid cloning the canoto fields.
| } | ||
| if ai.data != nil { | ||
| cloned.data = make([]common.AuxiliaryInfo, len(ai.data)) | ||
| for i, entry := range ai.data { |
There was a problem hiding this comment.
for i, entry := range ai.data { - this is racy.
We should only use i to access otherwise we copy canotoData too.
yacovm
left a comment
There was a problem hiding this comment.
Two important comments and then LGTM
Blocks now embed an AuxiliaryInfoBatch holding one entry per node. The builder collects legal entries from a new auxInfoStore mempool (populated by HandleAuxiliaryInfo) instead of calling Generate itself.
SignApproval is exported so the transition listener (next PR) can sign and broadcast approvals, and GetAuxiliaryHistory exposes the aux history traversal.
In future prs, the
HandleAuxiliaryInfoandHandleApprovalmethods will be called by the instance that feeds in messages.