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

AVS DevX: Go SDK bls aggregation service doesn't support >1 quorum #261

Open
samlaf opened this issue Jun 7, 2024 · 8 comments · Fixed by #277
Open

AVS DevX: Go SDK bls aggregation service doesn't support >1 quorum #261

samlaf opened this issue Jun 7, 2024 · 8 comments · Fixed by #277

Comments

@samlaf
Copy link
Collaborator

samlaf commented Jun 7, 2024

Describe the bug

bls aggregation needs to add signatures and g2 pubkeys for every quorum that an operator is part of, but it currently only adds them once: https://github.com/Layr-Labs/eigensdk-go/blob/4a204d0e0c9118babf1d74353f72c2b97009d7db/services/bls_aggregation/blsagg.go#L295C31-L295C46

We have unit tests for >1 quorums (eg here) but those were wrongly implemented, so will need to be fixed. An integration test with the contract would have picked this up.

To Reproduce

Expected behavior

Screenshots

OS details

Additional context

@samlaf samlaf added the bug Something isn't working label Jun 7, 2024
@renlulu
Copy link
Contributor

renlulu commented Jun 10, 2024

Do you mean the operator might use different bls key for different quorum? so we need to track it separately

@samlaf
Copy link
Collaborator Author

samlaf commented Jun 10, 2024

No same key. Just that each quorum needs to be verified, and the way we do this is by combining each quorum's sig in a single pairing check. Basically this means the aggregator needs to add a signature to the global aggregate signature for every quorum an operator is part of.

@renlulu
Copy link
Contributor

renlulu commented Jun 10, 2024

Will that need change following struct? Especially last 8 fields

// BlsAggregationServiceResponse is the response from the bls aggregation service
type BlsAggregationServiceResponse struct {
	Err                error                    // if Err is not nil, the other fields are not valid
	TaskIndex          types.TaskIndex          // unique identifier of the task
	TaskResponse       types.TaskResponse       // the task response that was signed
	TaskResponseDigest types.TaskResponseDigest // digest of the task response that was signed
	// The below 8 fields are the data needed to build the IBLSSignatureChecker.NonSignerStakesAndSignature struct
	// users of this service will need to build the struct themselves by converting the bls points
	// into the BN254.G1/G2Point structs that the IBLSSignatureChecker expects
	// given that those are different for each AVS service manager that individually inherits BLSSignatureChecker
	NonSignersPubkeysG1          []*bls.G1Point
	QuorumApksG1                 []*bls.G1Point
	SignersApkG2                 *bls.G2Point
	SignersAggSigG1              *bls.Signature
	NonSignerQuorumBitmapIndices []uint32
	QuorumApkIndices             []uint32
	TotalStakeIndices            []uint32
	NonSignerStakeIndices        [][]uint32
}

@renlulu
Copy link
Contributor

renlulu commented Jun 10, 2024

Not sure any change needed from contract side too. https://github.com/Layr-Labs/eigenlayer-middleware/blob/dev/src/BLSSignatureChecker.sol#L92

@renlulu
Copy link
Contributor

renlulu commented Jun 10, 2024

Not sure any change needed from contract side too. https://github.com/Layr-Labs/eigenlayer-middleware/blob/dev/src/BLSSignatureChecker.sol#L92

I see, seems we don't have to change contract, but only disperse side need some adjustment, if we change sdk here

@samlaf
Copy link
Collaborator Author

samlaf commented Jun 10, 2024

Correct. I will make the changes. Shouldn’t change any of the structs, just their contents. It’s a pretty minimal change I feel.

@samlaf
Copy link
Collaborator Author

samlaf commented Jun 21, 2024

Created integration test framework in #277 .
Left TODO for this PR:

  1. implement 2 quorum integration test: https://github.com/Layr-Labs/eigensdk-go/pull/277/files#diff-071edecb38c0d57bcdf4ed46768441f2ebff6138344bd39ec8999b5c06511eb2R892 (which will break b/c of this issue's bug)
  2. fix this bug

@samlaf
Copy link
Collaborator Author

samlaf commented Jul 11, 2024

Reopening as this was automatically closed by a PR which didn't actually implement this (only implemented integration test tooling to be able to test this feature once it's implemented). Sorry this is taking longer than planned.. getting sidetracked left and right.

supernovahs added a commit to Layr-Labs/eigensdk-rs that referenced this issue Aug 30, 2024
Implemented integration tests for `bls_aggregation` crate.
Added 5 tests:

- 2 quorums 1 operator (ignored)
- 2 quorums 2 operators shared (ignored)
- 1 quorum 1 operator
- 1 quorum 2 operators 
- 2 quorums 2 operators separated

Two cases are ignored as they are failing due to this bug: [bls
aggregation service doesn't support >1
quorum](Layr-Labs/eigensdk-go#261)

---------

Co-authored-by: supernovahs <[email protected]>
Co-authored-by: Supernovahs.eth <[email protected]>
Co-authored-by: Pablo Deymonnaz <[email protected]>
Co-authored-by: ricomateo <[email protected]>
@linear linear bot changed the title Bug: bls aggregation service doesn't support >1 quorum AVS DevX: Go SDK BUG - bls aggregation service doesn't support >1 quorum Sep 9, 2024
@linear linear bot changed the title AVS DevX: Go SDK BUG - bls aggregation service doesn't support >1 quorum AVS DevX: Go SDK bls aggregation service doesn't support >1 quorum Sep 9, 2024
@linear linear bot removed bug Something isn't working go-sdk labels Sep 9, 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 a pull request may close this issue.

2 participants