-
Notifications
You must be signed in to change notification settings - Fork 176
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
Core/v2 #806
Core/v2 #806
Conversation
Do you mind commenting on which files are copy paste of existing logic vs. which have logic updates? Maybe we can cover copy pastes in this PR and logic updates in separate PRs along with tests? |
Most of the copy-past is coming from #805 . I could rebase this PR to master to remove all of those delta. The tests will fail for now, but we can fix it later. Would that help? For this PR itself, the main copy-pastes are |
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 only reviewed some files by now, will look at the remaining in another pass
if params.ChunkLength*params.NumChunks >= g.SRSOrder { | ||
// Check that the parameters are valid with respect to the SRS. The precomputed terms of the amortized KZG | ||
// prover use up to order params.ChunkLen*params.NumChunks-1 for the SRS, so we must have | ||
// params.ChunkLen*params.NumChunks-1 <= g.SRSOrder. The condition below could technically |
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.
should this comment be >=
?
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.
The constraint is that params.ChunkLen*params.NumChunks-1 <= g.SRSOrder
, so the error case (below) is when the inequality is flipped (>
)!
func (v *ShardValidator) validateBlobQuorum(quorum QuorumID, blob *BlobShard, operatorState *chainio.OperatorState) ([]*encoding.Frame, *Assignment, error) { | ||
|
||
// Check if the operator is a member of the quorum | ||
if _, ok := operatorState.Operators[quorum]; !ok { |
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.
Should there be a further check on the specific v.operatorID
in the quorum? (operatorState.Operators[quorum][v.operatorID]
?) or is this more just checking a valid quorum ID in OperatorState
?
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 looks like both of these checks are done in the next function call. I'll add a comment and remove this check.
|
||
operatorCounts := []uint{4} | ||
|
||
numBlob := 1 // must be greater than 0 |
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.
could we add more tests such as different number of blobs and blobs that would be considered invalid by the verifier?
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.
@bxue-l2 can you take a look at the encoder changes?
core/v2/types.go
Outdated
type QuorumID = uint8 | ||
|
||
type OperatorID = [32]byte |
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.
Were we going to use those in chainio
?
|
||
} | ||
|
||
type PaymentHeader struct { |
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.
we should probably just use the one in core
?
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.
We can do some reconciliation once one or the other of the PRs is merged.
PaymentHeader | ||
|
||
// AuthenticationData is the signature of the blob header by the account ID | ||
AuthenticationData []byte `json:"authentication_data"` |
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.
authentication data is not part of blob header right?
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 it makes sense to store it in the internal data structure, even it if isn't contained within the hash (BlobKey
)?
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.
Hmm does it get used after the initial request validation?
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.
Will be passed onto DA nodes eventually.
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.
Do DA nodes validate this signature again? Then we need to think about how the interface changes there.
Even when that's the case, I think putting this in a separate data structure like BlobMetadata
would be cleaner. wdyt?
|
||
var ( | ||
ParametersMap = map[uint8]BlobVersionParameters{ | ||
0: {CodingRate: 8, ReconstructionThreshold: 0.22, NumChunks: 8192}, |
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.
How is ReconstructionThreshold
determined? Does it not depend on onchain parameters?
Full copy of code generally worries me. The old code usually lives longer than people would think, and that can be an issue for maintenance. |
|
||
func GetAssignment(state *chainio.OperatorState, blobVersion byte, quorum QuorumID, id core.OperatorID) (Assignment, error) { | ||
|
||
assignments, err := GetAssignments(state, blobVersion, quorum) |
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.
would it make sense to pass in assignments
as a param so GetAssignment
can avoid recalculating for each operator id?
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 basically a convenience function meant to be called by an operator who only needs the assignemnt for itself.
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.
Maybe we could just remove it.
type QuorumAttestation struct { | ||
// QuorumAggPubKeys contains the aggregated public keys for all of the operators each quorum, | ||
// including those that did not sign | ||
QuorumAggPubKey map[QuorumID]*bn254.G1Point |
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.
plural versus singular: QuorumAggPubKeys
?
I'm also thinking if there's better ways to arrange these structs (QuorumResult
, QuorumAttestation
, and SignatureAggregation
). It probably doesn't make material differences though
I tend to agree with this. How about we create new library under |
Why are these changes needed?
Adds a
core/v2
library which implements the core logic of v2 system.Note: PR size will be smaller when #805 is merged.
Checks