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

Add BLS tests #15

Merged
merged 8 commits into from Sep 20, 2023
Merged

Add BLS tests #15

merged 8 commits into from Sep 20, 2023

Conversation

catch-21
Copy link
Contributor

@catch-21 catch-21 commented Jun 1, 2023

Will not compile until we depend on cardano-node that uses the BLS supporting plutus version (after IntersectMBO/plutus#5231 is merged)

@catch-21 catch-21 force-pushed the bls branch 4 times, most recently from 8cc6dc8 to 8b386e2 Compare June 2, 2023 10:07
mkAggregateSigG1 :: BlsParamsWithPubKey -> sc -> Bool
mkAggregateSigG1 BlsParamsWithPubKey{..} _sc = do
let
hashedMsgs = mapWithConstSecondArg BI.bls12_381_G2_hashToGroup blsSigBls12381G2XmdSha256SswuRoNul messageElements
Copy link

Choose a reason for hiding this comment

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

blsSigBls12381G2XmdSha256SswuRoNul

Excellent variable name! (I understand why it's like that: it's a pity it's not a bit easier to convert from strings to bytestrings. I wonder if we can use pack here to do the conversion off-chain and then lift it into the Plutus code?)

Copy link
Contributor Author

@catch-21 catch-21 Jun 6, 2023

Choose a reason for hiding this comment

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

I can do this off chain

blsSigBls12381G2XmdSha256SswuRoNul :: P.BuiltinByteString
blsSigBls12381G2XmdSha256SswuRoNul = BI.toBuiltin $ Char8.pack "BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_"

But what's the benefit of lifting rather than simply referencing the variable as I am currently?

Copy link

Choose a reason for hiding this comment

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

But what's the benefit of lifting rather than simply referencing the variable as I am currently?

I forgot about this and now I'm not sure what I was talking about. I think maybe I was alluding to the fact that you need this BuiltinByteString "424c535f5349475f424c53313233383147325f584d443a5348412d3235365f535357555f524f5f4e554c5f" that's supposed to represent a DST containing the ascii string "BLS_blahblahblah" and it's kind of error-prone to have the big hex string rather than a readable string. I think I was trying to suggest that we could just write the readable string in the source and do the hard work of converting it to a bytestring off the chain (we might be able to convert it on-chain too, but that would surely be too expensive),

Copy link

Choose a reason for hiding this comment

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

Oh wait! I always get horribly confused about his, but isn't OverloadedStrings going to make "424c535f..." into the bytestring whose bytes are the ascii codes of the characters '4', '2', '4', 'c', ...? Don't we in fact want to call some function like bytesFromHex as in the Plutus examples? Maybe that's fixed now though.

Copy link

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

I'll comment now but come back and re-review some of it when I understand a bit better what's going on. One thing that strikes me is that it might be tricky to keep the code synchronised with the versions in plutus, but there's probably no easy way to fix that.

mkG2Element :: [Word8] -> CompressedG2Element
mkG2Element = CompressedG2Element . toBuiltin . BS.pack

groth16Scalar :: Integer
Copy link

Choose a reason for hiding this comment

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

I guess you should update this to match the refactored version that I included in the PR for plutus-benchmarks.


simpleVerifyBls12381SigTestInfo = TestInfo {
testName = "VerifyBls12381SigTest",
testDescription = "Uses three scripts with BLS functions." ++
Copy link

Choose a reason for hiding this comment

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

Outdated?

mkAggregateSigG1 :: BlsParamsWithPubKey -> sc -> Bool
mkAggregateSigG1 BlsParamsWithPubKey{..} _sc = do
let
hashedMsgs = mapWithConstSecondArg BI.bls12_381_G2_hashToGroup blsSigBls12381G2XmdSha256SswuRoNul messageElements
Copy link

Choose a reason for hiding this comment

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

But what's the benefit of lifting rather than simply referencing the variable as I am currently?

I forgot about this and now I'm not sure what I was talking about. I think maybe I was alluding to the fact that you need this BuiltinByteString "424c535f5349475f424c53313233383147325f584d443a5348412d3235365f535357555f524f5f4e554c5f" that's supposed to represent a DST containing the ascii string "BLS_blahblahblah" and it's kind of error-prone to have the big hex string rather than a readable string. I think I was trying to suggest that we could just write the readable string in the source and do the hard work of converting it to a bytestring off the chain (we might be able to convert it on-chain too, but that would surely be too expensive),

mkAggregateSigG1 :: BlsParamsWithPubKey -> sc -> Bool
mkAggregateSigG1 BlsParamsWithPubKey{..} _sc = do
let
hashedMsgs = mapWithConstSecondArg BI.bls12_381_G2_hashToGroup blsSigBls12381G2XmdSha256SswuRoNul messageElements
Copy link

Choose a reason for hiding this comment

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

Oh wait! I always get horribly confused about his, but isn't OverloadedStrings going to make "424c535f..." into the bytestring whose bytes are the ascii codes of the characters '4', '2', '4', 'c', ...? Don't we in fact want to call some function like bytesFromHex as in the Plutus examples? Maybe that's fixed now though.

@kwxm kwxm self-requested a review June 27, 2023 08:14
Copy link

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

OK, I think I understand what's going on now. I look forward to seeing this working!

… the blocking protocol version issue. All tests passing.
@catch-21 catch-21 merged commit a6f9dc7 into main Sep 20, 2023
4 checks passed
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.

None yet

2 participants