Skip to content
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

Closed
wants to merge 8 commits into from
Closed

Core/v2 #806

wants to merge 8 commits into from

Conversation

mooselumph
Copy link
Collaborator

@mooselumph mooselumph commented Oct 14, 2024

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

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ian-shim
Copy link
Contributor

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?

@mooselumph
Copy link
Collaborator Author

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 core/v2/aggreation*.go. I would treat everything else in core/v2 as new.

Copy link
Contributor

@hopeyen hopeyen left a 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

encoding/kzg/prover/parametrized_prover.go Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this comment be >=?

Copy link
Collaborator Author

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 {
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

core/v2/core_test.go Outdated Show resolved Hide resolved

operatorCounts := []uint{4}

numBlob := 1 // must be greater than 0
Copy link
Contributor

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?

Copy link
Contributor

@ian-shim ian-shim left a 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
Comment on lines 15 to 17
type QuorumID = uint8

type OperatorID = [32]byte
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

core/v2/types.go Show resolved Hide resolved
PaymentHeader

// AuthenticationData is the signature of the blob header by the account ID
AuthenticationData []byte `json:"authentication_data"`
Copy link
Contributor

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?

Copy link
Collaborator Author

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)?

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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},
Copy link
Contributor

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?

@jianoaix
Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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
Copy link
Contributor

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

@ian-shim
Copy link
Contributor

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.

I tend to agree with this. How about we create new library under core/v2 only for those with updated logic?
@mooselumph I know you preferred not to have any dependencies on core in v2 components, but I think using core libraries with no logic changes is fine, can be helpful for maintenance, and migration remains straight forward. Any thoughts?

@ian-shim ian-shim mentioned this pull request Oct 18, 2024
6 tasks
@mooselumph mooselumph closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants