Add Supranational BLS implementation #2453

Nashatyrev · 2020-07-27T16:59:57Z

PR Description

Add Supranational BLS implementation based on highly optimized native BLS-12381 library

For now I forked the Blst repo and made some tweaks for the Java SWIG binding works correctly on all platforms. Will be working with Blst team to resolve them and push upstream.

The Java SWIG wrapper generated classes and manually built native libraries (Windows, Linux, MacOS) are now in the separate repository: https://github.com/Nashatyrev/jblst. Gradle bintrayUpload task may be used to build and publish the tech.pegasys:jblst artifact

By default the blst BLS implementation is loaded. If for some reason it fails to load native lib BLS falls back to Mikuli implementation (with a WARN log)

The PR was tested on: Windows, Linux and MacOS

Future tasks to do:

migrate to better C++ SWIG bindings (here for now: https://github.com/dot-asm/blst/blob/python.demo/)
resolve issues from the forked Blst repo and switch to the main Blst repo
Create and setup jblst repo to generate SWIG java wrappers and build native libs from upstream blst repo on CI platforms and build the final artifact
Rollback this commit Temporary handling infinite pubkey/signature as a special case if/when the issue with infinite pubkey/signatures is resolved: Infinity pubkey and signature supranational/blst#11
Replace BLSConstants.VERIFICATION_DISABLED flag with Noop BLS implementation

TODOs

Make common BLS tests cover both implementations
Remove pubkey/signature bytes store/lazy calc as it it now handled in common classes
Move artifact to official Pegasys Bintray org

Import Benchmarks

Before:

Benchmark                              (validatorsCount)  Mode  Cnt  Score   Error  Units
TransitionBenchmark.Block.importBlock              32768    ss  250  0,076 ± 0,002   s/op
TransitionBenchmark.Epoch.importBlock              32768    ss  100  0,458 ± 0,009   s/op

After:

Benchmark                              (validatorsCount)  Mode  Cnt  Score   Error  Units
TransitionBenchmark.Block.importBlock              32768    ss  250  0,021 ± 0,003   s/op
TransitionBenchmark.Epoch.importBlock              32768    ss  100  0,415 ± 0,012   s/op

BLS Benchmarks

Blst is x7-x9 faster than Mikuli:

Test	SigCnt	Mikuli	Blst	Speedup
verifySignatureBatchedNonParallelDoublePairing	1	36,177	318,327	8.8
verifySignatureBatchedNonParallelDoublePairing	2	26,333	206,738	7.9
verifySignatureBatchedNonParallelDoublePairing	4	16,908	117,974	7.0
verifySignatureBatchedNonParallelDoublePairing	8	9,324	66,379	7.1
verifySignatureBatchedNonParallelDoublePairing	16	5,379	35,010	6.5
verifySignatureBatchedNonParallelDoublePairing	32	2,768	18,101	6.5
verifySignatureBatchedNonParallelDoublePairing	64	1,442	8,892	6.2
verifySignatureBatchedNonParallelDoublePairing	128	726	4,654	6.4
verifySignatureBatchedNonParallelSinglePairing	1	36,713	324,845	8.8
verifySignatureBatchedNonParallelSinglePairing	2	25,561	203,667	8.0
verifySignatureBatchedNonParallelSinglePairing	4	16,004	123,699	7.7
verifySignatureBatchedNonParallelSinglePairing	8	8,663	67,762	7.8
verifySignatureBatchedNonParallelSinglePairing	16	4,644	35,188	7.6
verifySignatureBatchedNonParallelSinglePairing	32	2,607	18,015	6.9
verifySignatureBatchedNonParallelSinglePairing	64	1,337	8,908	6.7
verifySignatureBatchedNonParallelSinglePairing	128	677	4,607	6.8
verifySignatureBatchedParallelDoublePairing	1	36,081	311,249	8.6
verifySignatureBatchedParallelDoublePairing	2	26,631	207,796	7.8
verifySignatureBatchedParallelDoublePairing	4	23,819	199,297	8.4
verifySignatureBatchedParallelDoublePairing	8	20,292	151,721	7.5
verifySignatureBatchedParallelDoublePairing	16	14,747	106,536	7.2
verifySignatureBatchedParallelDoublePairing	32	8,419	60,879	7.2
verifySignatureBatchedParallelDoublePairing	64	4,466	33,660	7.5
verifySignatureBatchedParallelDoublePairing	128	2,477	16,651	6.7
verifySignatureBatchedParallelSinglePairing	1	35,797	320,506	9.0
verifySignatureBatchedParallelSinglePairing	2	31,836	300,686	9.4
verifySignatureBatchedParallelSinglePairing	4	27,452	243,981	8.9
verifySignatureBatchedParallelSinglePairing	8	22,340	187,294	8.4
verifySignatureBatchedParallelSinglePairing	16	14,204	106,880	7.5
verifySignatureBatchedParallelSinglePairing	32	8,375	64,067	7.6
verifySignatureBatchedParallelSinglePairing	64	4,551	32,898	7.2
verifySignatureBatchedParallelSinglePairing	128	2,342	16,482	7.0
verifySignatureSimple	1	51,861	357,217	6.9
verifySignatureSimple	2	25,967	174,044	6.7
verifySignatureSimple	4	12,868	87,326	6.8
verifySignatureSimple	8	6,423	44,326	6.9
verifySignatureSimple	16	3,259	22,209	6.8
verifySignatureSimple	32	1,649	10,938	6.6
verifySignatureSimple	64	816	5,581	6.8
verifySignatureSimple	128	400	2,775	6.9

Fixes

Fix #2300

Documentation

I thought about documentation and added the documentation label to this PR if updates are required.

…ecific

…aces # Conflicts: # bls/src/main/java/tech/pegasys/teku/bls/BLS.java # bls/src/main/java/tech/pegasys/teku/bls/BLSPublicKey.java # bls/src/test/java/tech/pegasys/teku/bls/BLSPublicKeyTest.java # data/provider/src/main/java/tech/pegasys/teku/api/schema/BLSPubKey.java

…aces

# Conflicts: # bls/build.gradle # bls/src/test/java/tech/pegasys/teku/bls/impl/mikuli/hash2g2/ReferenceTests.java

ajsutton

I'd be keen for @benjaminion to review the BLS specifics as I'm not too familiar with that. Generally looks good otherwise.

The only concern I have is managing the native memory resources to ensure we aren't leaking them. I wonder if we should create a factory class that is the only place we create instances of the SWIG generated classes and automatically adds them to a Cleaner to ensure they are always cleaned up. Unfortunately you just can't depend on the finalized method being called even when the system is heavily under memory pressure.

bls/src/main/java/tech/pegasys/teku/bls/BLSPublicKey.java

bls/src/main/java/tech/pegasys/teku/bls/BLS.java

bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstBLS12381.java

ajsutton · 2020-07-30T01:38:33Z

bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstBLS12381.java

+    p2Signature.delete();
+    hash.delete();


If it's at all possible to have the SWIG bindings make these classes Autocloseable that would be awesome. In any case we should ensure they are deleted regardless of any potential exceptions or errors that occur so would have to use try/finally blocks. It will get really ugly without Autocloseable.

We're also winding up with a native memory leak because the wrapped BlstSignature returned has a reference to p2SignatureAffine but no way to close it and delete that native object. The finalize method isn't guaranteed to be called in any kind of timely manner so we can't really depend on it.

If we have to depend on automatic cleanup (which may well be the case for things like public keys and signatures that are used outside of the Blst specific code and currently expect to be fully on-heap), we should use java.lang.ref.Cleaner to do it which is the recommended instead of finalize.

Maybe use AutoCloseable if possible for the local variables we need to release and then use Cleaner for the things that are actually returned from this class.

I didn't find Autocloseable feature in SWIG unfortunately. Indeed that would look better.
Revised places with temporary native wrapper instances and add try/catch/finally where appropriate: 913e484
I'll add a general comment later on my thoughts regarding finalize and cleaning native structs

bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstBLS12381.java

bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstPublicKey.java

bls/src/test/java/tech/pegasys/teku/bls/BLSPublicKeyTest.java

bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstPublicKeyTest.java

bls/src/test/java/tech/pegasys/teku/bls/impl/mikuli/MikuliPublicKeyTest.java

ajsutton · 2020-07-30T04:05:42Z

gradle/check-licenses.gradle

@@ -169,6 +169,7 @@ downloadLicenses {
      //JMH-Core is licensed under GPLv2 with the Classpath Exception, which allows us to link it and license the derived work under our license.
      'org.openjdk.jmh:jmh-core:1.21': apache,
      (group('io.libp2p')): apache,
+      (group('tech.pegasys')): apache,


We should probably log a ticket to get the license information included in the blst native jar. I can't remember the magic incantation and its not a priority but something we should do when we setup a full CI for it and then we shouldn't need this.

bls/src/main/java/tech/pegasys/teku/bls/BLS.java

bls/src/main/java/tech/pegasys/teku/bls/BLSSecretKey.java

…re and make them private

…s Optional

…BLSPublicKey fromSSZBytes() and fromBytesCompressed() though they are equivalent with current SSZ implementation

…Compressed() though they are equivalent with the current SSZ implementation

bls/src/test/java/tech/pegasys/teku/bls/BLSPublicKeyTest.java

bls/src/test/java/tech/pegasys/teku/bls/BLSSignatureTest.java

bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstTest.java

validator/client/src/test/java/tech/pegasys/teku/validator/client/signer/SignerTest.java

Nashatyrev · 2020-07-30T17:01:45Z

@ajsutton I don't think that finalize in this concrete case is absolute evil.

Assuming that underlying native structs are of comparable size as their java wrapper objects they are most likely will be GCed at some point. If they are not GCed then it means we are fine with free RAM at the moment and the underlying allocated structures would make huge difference. So this not our case when large native memory chunks are referenced by small Java wrappers and GC is not aware of the real object allocations and don't hurry up collecting them. (as I remember we had the latter issue in Java2D library with direct byte buffers)

Here we have pretty simple logic behind finalize. It doesn't involve any interaction with other components as well it doesn't do any blocking operations as well it doesn't throw exceptions. So hitting any complex issue with blocking finalizer thread or leaving hanging object is highly unlikely.

As far as I understand the phrase 'finalize() is not guaranteed to be invoked' relates to JVM shutdown only. So e.g. correctly closing files is not recommended there. As I know there are also OSes which doesn't release all resources automatically when a process terminates, so releasing OS resources in finalize() is not recommended as well in such environment.

I see the following options at the moment:

finalize()
PhantomReference queue: I'm not sure if this would be somehow different from finalize approach in our simple case.
Manually release resources
Store raw native data in say byte[] and pass it to the native function. Andy from Blst was suggesting something like this. This is an interesting option though this would require additional efforts from both sides

BTW found this issue with similar discussion: google/or-tools#1040 So this Google lib also uses SWIG and finalize() to release native resources.

The above is just my vision and I'm not sure of course that these are all correct statements.

benjaminion

I flagged a few trivia, but on the basis of mine and Adrian's reviews I am happy to approve this. It's really good to have this done!

I built locally, and all was well. Running locally, I was happy to see

2020-07-30 18:09:48.850+01:00 | beaconchain-async-0 | INFO  | BlstBLS12381 | Successfully loaded native BLS library

and my sync speed was about 3x better than with the JVM BLS lib. 🙌

(Caveat: I'm leaving it to @Nashatyrev and @ajsutton to decide whether the finalize() things is settled. It looks like it has been thought-through in any case.)

…S.aggregateSignatures() -> BLS.aggregate()

ajsutton

I'm still nervous about finalisers but agree that the implementations aren't going to cause deadlocks or other things - just likely that the native memory won't be released as fast as it probably should. As you say they're quite small amounts of memory so shouldn't be an issue.

So I'm happy to merge this.

Nashatyrev added 30 commits June 25, 2020 14:36

Draft initial experiments

fc11c9f

Merge remote-tracking branch 'pegasys/master' into experimental-supra

1532577

Add JNA library draft

745ca3a

Move SWIG wrappers to a separate package

f442a69

Updated SWIG wrappers

986c4a8

Initial BLS Blst classes. Tests updated

3eff96d

Merge remote-tracking branch 'pegasys/master' into experimental-supra

7413682

Update the draft

5cb89b4

Merge remote-tracking branch 'pegasys/master' into experimental-supra

54f7a01

temp commit

9256c93

Upgrade blst classes

67b1a9d

Update tests

f8da728

Update dll

a7439d5

Temp disable json-simple lib

939e301

Initial draft for BLS interfaces

7fd531b

exclude outdated junit transitive dependency from json-simple module

69300d6

Rename test file to Win compatible

8724e7f

Merge branch 'fix-bls-tests-1' into feature-bls-interfaces

84691cd

Another portion of draft changes

a8bafde

Adjust bls tests by separating them to abstract and implementation sp…

b5762f4

…ecific

Add NoopBLS12381 implementation

27aace7

Fix warnings

5f93ac4

Fix more warns

7c4faf6

Resolve merge conflicts

8c41c8d

Fix the test

aab091f

Fix the test

2040313

Merge remote-tracking branch 'pegasys/master' into feature-bls-interf…

69313d7

…aces

Merge branch 'feature-bls-interfaces' into experimental-supra

59e59c1

# Conflicts: # bls/build.gradle # bls/src/test/java/tech/pegasys/teku/bls/impl/mikuli/hash2g2/ReferenceTests.java

Remove JNA binding

5b48be7

ajsutton reviewed Jul 30, 2020

View reviewed changes

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/main/java/tech/pegasys/teku/bls/BLS.java Outdated Show resolved Hide resolved

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/main/java/tech/pegasys/teku/bls/BLS.java Outdated Show resolved Hide resolved

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/main/java/tech/pegasys/teku/bls/BLSSecretKey.java Outdated Show resolved Hide resolved

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/main/java/tech/pegasys/teku/bls/BLSSecretKey.java Outdated Show resolved Hide resolved

Nashatyrev added 11 commits July 30, 2020 16:33

Remove obsolete comment

f9577a0

Moved BlstSignature specific methods from BlstBLS12381 to BlstSignatu…

0c248d9

…re and make them private

Fix batch verify case with invalid infinity signature

7447b6f

Add lost test case

fd219cc

Left comment for disabled test

654e2fa

Add explicit assertion to the test

ea92796

Apply spotless

d903419

Make BlstBLS12381.INSTANCE initialization more reliable and make it a…

d17224f

…s Optional

Move BLS constants to BLSConstants class. Make a clear separation of …

25fb802

…BLSPublicKey fromSSZBytes() and fromBytesCompressed() though they are equivalent with current SSZ implementation

Make a distinction of BLSSignature from/toSSZBytes() and from/toBytes…

5dca043

…Compressed() though they are equivalent with the current SSZ implementation

Add try/catch/finally to explicitly delete native structures

913e484

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/test/java/tech/pegasys/teku/bls/BLSPublicKeyTest.java Outdated Show resolved Hide resolved

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/test/java/tech/pegasys/teku/bls/BLSSignatureTest.java Outdated Show resolved Hide resolved

benjaminion reviewed Jul 30, 2020

View reviewed changes

bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstTest.java Outdated Show resolved Hide resolved

benjaminion reviewed Jul 30, 2020

View reviewed changes

validator/client/src/test/java/tech/pegasys/teku/validator/client/signer/SignerTest.java Show resolved Hide resolved

benjaminion approved these changes Jul 30, 2020

View reviewed changes

Nashatyrev added 2 commits July 30, 2020 20:21

Minor BLS tests cleanups

7ade0ab

Move BLS.aggregatePublicKeys() -> BLSPublicKey.aggregate(), rename BL…

b60a046

…S.aggregateSignatures() -> BLS.aggregate()

ajsutton approved these changes Jul 30, 2020

View reviewed changes

Nashatyrev added 2 commits July 31, 2020 02:03

Merge branch 'master' into feature-blst

377eaf1

Merge branch 'master' into feature-blst

d7559ca

Nashatyrev merged commit b3d2e07 into Consensys:master Jul 31, 2020

Nashatyrev mentioned this pull request Jul 31, 2020

BLS tests: cover both implementations #2490

Merged

1 task

Nashatyrev deleted the feature-blst branch September 8, 2020 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Supranational BLS implementation #2453

Add Supranational BLS implementation #2453

Nashatyrev commented Jul 27, 2020 •

edited

ajsutton left a comment

ajsutton Jul 30, 2020

Nashatyrev Jul 30, 2020 •

edited

ajsutton Jul 30, 2020

Nashatyrev commented Jul 30, 2020 •

edited

benjaminion left a comment •

edited

ajsutton left a comment

Add Supranational BLS implementation #2453

Add Supranational BLS implementation #2453

Conversation

Nashatyrev commented Jul 27, 2020 • edited

PR Description

TODOs

Import Benchmarks

BLS Benchmarks

Fixes

Documentation

ajsutton left a comment

Choose a reason for hiding this comment

ajsutton Jul 30, 2020

Choose a reason for hiding this comment

Nashatyrev Jul 30, 2020 • edited

Choose a reason for hiding this comment

ajsutton Jul 30, 2020

Choose a reason for hiding this comment

Nashatyrev commented Jul 30, 2020 • edited

benjaminion left a comment • edited

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Nashatyrev commented Jul 27, 2020 •

edited

Nashatyrev Jul 30, 2020 •

edited

Nashatyrev commented Jul 30, 2020 •

edited

benjaminion left a comment •

edited