-
Notifications
You must be signed in to change notification settings - Fork 101
core: add Ethereum-compatible BLS12-381 serializators and operations to CryptoLib #4050
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
base: master
Are you sure you want to change the base?
Conversation
5d040cd to
11020de
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4050 +/- ##
==========================================
+ Coverage 83.45% 83.49% +0.04%
==========================================
Files 352 352
Lines 42754 43158 +404
==========================================
+ Hits 35679 36036 +357
- Misses 5323 5366 +43
- Partials 1752 1756 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It's not about aliases now, it's just a new function. |
AnnaShaleva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workflows are failing.
Also, need some time to check the main math logic.
go.mod
Outdated
| google.golang.org/protobuf v1.36.9 // indirect | ||
| ) | ||
|
|
||
| replace github.com/nspcc-dev/neo-go/pkg/interop => ./pkg/interop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it once PR is finilized.
11020de to
5a566d4
Compare
0d9946b to
531864f
Compare
008e092 to
db38756
Compare
3aabc28 to
40f06e3
Compare
14c0d14 to
22bdc25
Compare
444f75c to
84e7b36
Compare
84e7b36 to
c0711db
Compare
e438231 to
f99c5a9
Compare
|
|
||
| func (p *blsPoint) FromBytesEth(buf []byte) error { | ||
| switch l := len(buf); l { | ||
| case bls12G1EncodedLength: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add default case, return an appropriate error.
| } | ||
| } | ||
|
|
||
| func (p blsPoint) BytesEth() []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported method needs a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto everywhere.
f99c5a9 to
88984f8
Compare
Close #4041. Signed-off-by: Tural Devrishev <[email protected]>
88984f8 to
09d273a
Compare
|
@txhsl could you please review the implementation one more time? This CryptoLib API is the final suggestion from our side. Our primary concern was neo-project/neo#4186 (comment), so we believe it's solved in this PR. |
txhsl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our primary concern was neo-project/neo#4186 (comment), so we believe it's solved in this PR.
Yes, I see the difference you've made, and the implementation is quite clear to me.
Maybe it's time for @Jim8y to have a look on this, he's a bit confused about the idea at the beginning.
| res = append(res, p.BytesEth()...) | ||
| scalarBE := scalar.Bytes() | ||
| scalarBytes := make([]byte, bls12ScalarLength) | ||
| copy(scalarBytes[bls12ScalarLength-len(scalarBE):], scalarBE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a length/border check first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will panic with slice bounds out of range if scalarBE doesn't fit into bls12ScalarLength.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will panic with
slice bounds out of rangeifscalarBEdoesn't fit intobls12ScalarLength.
Oh, yes.
| const ( | ||
| g1Hex = "97f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb" | ||
| g2Hex = "93e02b6052719f607dacd3a088274f65596bd0d09920b61ab5da61bbdc7f5049334cf11213945d57e5ac7d055d042b7e024aa2b2f08f0a91260805272dc51051c6e47ad4fa403b02b4510b647ae3d1770bac0326a805bbefd48056c8c121bdb8" | ||
| ethG1MultiExpSingleInputHex = "0000000000000000000000000000000017f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb0000000000000000000000000000000008b3f481e3aaa0f1a09e30ed741d8ae4fcf5e095d5d00af600db18cb2c04b3edd03cc744a2888ae40caa232946c5e7e10000000000000000000000000000000000000000000000000000000000000011" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can have more EVM test cases for operations other than MultiExp, although IMO they have already provided enough guarantees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, at least we need to add:
- Compatibility tests for
bls12381PairingListwith some hex-encoded raw data that may be ported to C#. - Bi-directional serializators tests for all added APIs: ensure that result of
serialize(deserialize(input))matches theinputexactly.
| return stackitem.NewByteArray(res) | ||
| } | ||
|
|
||
| func (c *Crypto) bls12381SerializeEthList(_ *interop.Context, args []stackitem.Item) stackitem.Item { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every introduced method needs a comment explaining which type of conversion it's performing, like it's done for bls12381SerializeList.
| res = append(res, p.BytesEth()...) | ||
| scalarBE := scalar.Bytes() | ||
| scalarBytes := make([]byte, bls12ScalarLength) | ||
| copy(scalarBytes[bls12ScalarLength-len(scalarBE):], scalarBE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will panic with slice bounds out of range if scalarBE doesn't fit into bls12ScalarLength.
| const ( | ||
| g1Hex = "97f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb" | ||
| g2Hex = "93e02b6052719f607dacd3a088274f65596bd0d09920b61ab5da61bbdc7f5049334cf11213945d57e5ac7d055d042b7e024aa2b2f08f0a91260805272dc51051c6e47ad4fa403b02b4510b647ae3d1770bac0326a805bbefd48056c8c121bdb8" | ||
| ethG1MultiExpSingleInputHex = "0000000000000000000000000000000017f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb0000000000000000000000000000000008b3f481e3aaa0f1a09e30ed741d8ae4fcf5e095d5d00af600db18cb2c04b3edd03cc744a2888ae40caa232946c5e7e10000000000000000000000000000000000000000000000000000000000000011" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, at least we need to add:
- Compatibility tests for
bls12381PairingListwith some hex-encoded raw data that may be ported to C#. - Bi-directional serializators tests for all added APIs: ensure that result of
serialize(deserialize(input))matches theinputexactly.
Close #4041.