fix: Verify v2 cert instead of reflexively passing #1648
Conversation
Signed-off-by: litt3 <[email protected]>
| } | ||
|
|
||
| // Convert BlobInclusionInfo from V2 to V3 format | ||
| v3BlobInclusionInfo := certTypesBinding.EigenDATypesV2BlobInclusionInfo{ |
There was a problem hiding this comment.
why there are two binding?
contractEigenDACertVerifierV2 "github.com/Layr-Labs/eigenda/contracts/bindings/EigenDACertVerifierV2"
certTypesBinding "github.com/Layr-Labs/eigenda/contracts/bindings/IEigenDACertTypeBindings"
If they are the same, then I suppose we can do assignment.
There was a problem hiding this comment.
First is the actual contract, the other just contains the router cert types (>= 3) afaik
There was a problem hiding this comment.
@bxue-l2 Basic assignment doesn't work, I think the only way is manual here.
| NonSignerQuorumBitmapIndices: certV2.NonSignerStakesAndSignature.NonSignerQuorumBitmapIndices, | ||
| NonSignerPubkeys: convertV2PubkeysToV3(certV2.NonSignerStakesAndSignature.NonSignerPubkeys), | ||
| QuorumApks: convertV2PubkeysToV3(certV2.NonSignerStakesAndSignature.QuorumApks), | ||
| ApkG2: certTypesBinding.BN254G2Point{ | ||
| X: certV2.NonSignerStakesAndSignature.ApkG2.X, | ||
| Y: certV2.NonSignerStakesAndSignature.ApkG2.Y, | ||
| }, | ||
| Sigma: certTypesBinding.BN254G1Point{ | ||
| X: certV2.NonSignerStakesAndSignature.Sigma.X, |
There was a problem hiding this comment.
I keep getting scared that we'll forget a field when doing these verbose manual conversions. Doesn't seem like go has any other good way to do this though (outside of json conversion which has runtime costs and unsafe pointer assignment whcih is.. unsafe).
Will our current set of linters catch if we forget a field? I don't think so right since we turned that off to allow for default values in a bunch of places... otherwise Claude proposed this one
There was a problem hiding this comment.
I'm not really sure this would provide any additional safety.
If we were constructing this v3 cert in multiple locations, and we wanted to just create a single implementation that are very confident in that could be reused, that would make sense.
But we are only doing this in one place. If we were to create constructors to do this work, then those functions have equivalent bug potential to the existing code, right?
| func convertUint32SliceToRelayKeys(relayKeys []uint32) []coreV2.RelayKey { | ||
| result := make([]coreV2.RelayKey, len(relayKeys)) | ||
| for i, key := range relayKeys { | ||
| result[i] = coreV2.RelayKey(key) | ||
| } | ||
| return result |
There was a problem hiding this comment.
sad that golang forces us to make an allocation for these conversions.. https://go.dev/doc/faq#convert_slice_with_same_underlying_type
It is what it is I guess
| @@ -3,7 +3,7 @@ | |||
| go = "1.21" | |||
|
|
|||
| # Tooling Dependencies | |||
| "go:github.com/golangci/golangci-lint/cmd/golangci-lint" = "1.60.3" | |||
| golangci-lint = "1.60.3" | |||
There was a problem hiding this comment.
good catch for this! Looking at https://mise.jdx.dev/registry.html?filter=golangci#tools, it seems like this will use one of the binary installer backends (aqua, ubi, asdf) and download the binary directly. Whereas the go tool will download the source code and then build the binary.
Honestly I don't understand why you had issues with the go build... but this way should be faster anyways.
Signed-off-by: litt3 <[email protected]>
litt3
dismissed stale reviews from samlaf, bxue-l2, and ethenotethan
via
154c482
Since v2 cert and v3 cert are identical, except for field ordering, we can easily (though verbosely) convert from v2->v3. This allows us to provide correct verification results for v2 certs, rather than reflexively treating them as valid.
Closes DAINT-550 (link)