Core/v2 #806

mooselumph · 2024-10-14T23:33:16Z

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 · 2024-10-16T05:35:00Z

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 · 2024-10-16T05:52:03Z

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.

hopeyen

I only reviewed some files by now, will look at the remaining in another pass

hopeyen · 2024-10-16T23:38:55Z

encoding/kzg/prover/parametrized_prover.go

-	lengthProofChan := make(chan LengthProofResult, 1)
-	commitmentChan := make(chan CommitmentResult, 1)
-	proofChan := make(chan ProofsResult, 1)
+	type commitments struct {


what's the reason to define commitments type within the Encode function?

I think it might be suitable since it's only being used here. I moved it out for consistency!

hopeyen · 2024-10-16T23:49:50Z

encoding/kzg/prover/prover_cpu.go

-	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


should this comment be >=?

The constraint is that params.ChunkLen*params.NumChunks-1 <= g.SRSOrder, so the error case (below) is when the inequality is flipped (>)!

hopeyen · 2024-10-16T23:57:32Z

core/v2/validator.go

+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 {


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?

It looks like both of these checks are done in the next function call. I'll add a comment and remove this check.

hopeyen · 2024-10-17T00:01:54Z

core/v2/core_test.go

+
+}
+
+// prepareBatch takes in multiple blob, encodes them, generates the associated assignments, and the batch header.


prepareBlobs?

hopeyen · 2024-10-17T00:04:26Z

core/v2/core_test.go

+
+	operatorCounts := []uint{4}
+
+	numBlob := 1 // must be greater than 0


could we add more tests such as different number of blobs and blobs that would be considered invalid by the verifier?

ian-shim

@bxue-l2 can you take a look at the encoder changes?

ian-shim · 2024-10-17T04:48:01Z

core/v2/types.go

+type QuorumID = uint8
+
+type OperatorID = [32]byte


Were we going to use those in chainio?

ian-shim · 2024-10-17T04:49:28Z

core/v2/types.go

+
+}
+
+type PaymentHeader struct {


we should probably just use the one in core?

We can do some reconciliation once one or the other of the PRs is merged.

core/v2/types.go

ian-shim · 2024-10-17T04:50:43Z

core/v2/types.go

+	PaymentHeader
+
+	// AuthenticationData is the signature of the blob header by the account ID
+	AuthenticationData []byte `json:"authentication_data"`


authentication data is not part of blob header right?

I think it makes sense to store it in the internal data structure, even it if isn't contained within the hash (BlobKey)?

Hmm does it get used after the initial request validation?

Will be passed onto DA nodes eventually.

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?

ian-shim · 2024-10-17T04:53:42Z

core/v2/types.go

+
+var (
+	ParametersMap = map[uint8]BlobVersionParameters{
+		0: {CodingRate: 8, ReconstructionThreshold: 0.22, NumChunks: 8192},


How is ReconstructionThreshold determined? Does it not depend on onchain parameters?

jianoaix · 2024-10-17T20:02:22Z

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.

hopeyen · 2024-10-17T19:23:37Z

core/v2/assignment.go

+
+func GetAssignment(state *chainio.OperatorState, blobVersion byte, quorum QuorumID, id core.OperatorID) (Assignment, error) {
+
+	assignments, err := GetAssignments(state, blobVersion, quorum)


would it make sense to pass in assignments as a param so GetAssignment can avoid recalculating for each operator id?

I think this is basically a convenience function meant to be called by an operator who only needs the assignemnt for itself.

Maybe we could just remove it.

hopeyen · 2024-10-17T20:15:50Z

core/v2/aggregation.go

+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


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 · 2024-10-17T20:37:44Z

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?

mooselumph requested review from dmanc, cody-littley, hopeyen, bxue-l2, jianoaix and ian-shim October 14, 2024 23:33

mooselumph force-pushed the core/v2 branch from 990f88a to 39b52d2 Compare October 15, 2024 22:52

hopeyen reviewed Oct 17, 2024

View reviewed changes

ian-shim reviewed Oct 17, 2024

View reviewed changes

mooselumph added 4 commits October 17, 2024 16:58

Breakout encoding functions

94c17a3

Add core/v2

ded07b1

Update core types

8cb55fe

Lint fixes

26c6684

mooselumph force-pushed the core/v2 branch from 4887bf1 to 26c6684 Compare October 17, 2024 16:59

hopeyen reviewed Oct 17, 2024

View reviewed changes

ian-shim mentioned this pull request Oct 18, 2024

[v2] Blob Metadata Store #818

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core/v2 #806

Core/v2 #806

mooselumph commented Oct 14, 2024 •

edited

Loading

ian-shim commented Oct 16, 2024

mooselumph commented Oct 16, 2024

hopeyen left a comment

hopeyen Oct 16, 2024

mooselumph Oct 19, 2024

hopeyen Oct 16, 2024

mooselumph Oct 19, 2024

hopeyen Oct 16, 2024

mooselumph Oct 19, 2024

hopeyen Oct 17, 2024

hopeyen Oct 17, 2024

ian-shim left a comment

ian-shim Oct 17, 2024

ian-shim Oct 17, 2024

mooselumph Oct 17, 2024

ian-shim Oct 17, 2024

mooselumph Oct 17, 2024

ian-shim Oct 17, 2024

mooselumph Oct 17, 2024

ian-shim Oct 17, 2024

ian-shim Oct 17, 2024

jianoaix commented Oct 17, 2024

hopeyen Oct 17, 2024

mooselumph Oct 19, 2024

mooselumph Oct 19, 2024

hopeyen Oct 17, 2024

ian-shim commented Oct 17, 2024


		}

		// prepareBatch takes in multiple blob, encodes them, generates the associated assignments, and the batch header.


		operatorCounts := []uint{4}

		numBlob := 1 // must be greater than 0


		func GetAssignment(state *chainio.OperatorState, blobVersion byte, quorum QuorumID, id core.OperatorID) (Assignment, error) {

		assignments, err := GetAssignments(state, blobVersion, quorum)

Core/v2 #806

Are you sure you want to change the base?

Core/v2 #806

Conversation

mooselumph commented Oct 14, 2024 • edited Loading

Why are these changes needed?

Checks

ian-shim commented Oct 16, 2024

mooselumph commented Oct 16, 2024

hopeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianoaix commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim commented Oct 17, 2024

mooselumph commented Oct 14, 2024 •

edited

Loading