Replace Harmony BLS forks with official Herumi package - #2
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| func (pub *PublicKey) Sub(rhs *PublicKey) { | ||
| if pub == nil || rhs == nil { | ||
| return | ||
| } | ||
| out := (*herumi.G1)(unsafe.Pointer(&pub.PublicKey)) | ||
| herumi.G1Sub(out, out, (*herumi.G1)(unsafe.Pointer(&rhs.PublicKey))) | ||
| } |
There was a problem hiding this comment.
unsafe.Pointer cast relies on undocumented layout equality
(*herumi.G1)(unsafe.Pointer(&pub.PublicKey)) is valid today because both herumi.PublicKey and herumi.G1 are defined as struct{ v C.blsPublicKey } in herumi v1.37.0. However, there is no compile-time assertion that the two types share identical memory layouts, so any future herumi update that adds a field (e.g., a cached flag or an extra coordinate word) would silently corrupt the aggregated public key during mask operations without a build error or test failure unless the golden vectors happen to exercise Sub. Consider adding a compile-time size/offset guard, or opening an upstream issue asking herumi to expose a PublicKey.Sub method directly.
Greptile SummaryThis PR replaces the Harmony-forked
Confidence Score: 4/5The core cryptographic wiring is validated by a golden-vector test that pins all four output bytes; the change is mechanically straightforward across 81 files with no logic rewrites. The Files Needing Attention: crypto/bls/core/core.go (Sub unsafe cast) and crypto/bls/mask.go (redundant init with dropped error)
|
| Filename | Overview |
|---|---|
| crypto/bls/core/core.go | New wrapper package configuring Herumi BLS for Harmony; contains an unsafe.Pointer cast for Sub and a double-init path when imported from mask.go |
| crypto/bls/core/core_test.go | Golden-vector test verifying secret key, public key, and signature wire compatibility with the historical Harmony BLS layout |
| crypto/bls/mask.go | Adds a defensive buffer copy before passing to CGo in BytesToBLSPublicKey; init() now redundantly re-invokes Init whose error is silently dropped |
| p2p/host.go | Adds nil guard for ConsensusPubKey before SerializeToHexStr() in the log statement, preventing a latent nil-dereference panic |
| go.mod | Replaces github.com/harmony-one/bls v0.0.6 with github.com/herumi/bls-eth-go-binary v1.37.0 |
| scripts/setup_bls_build_flags.sh | Deleted; the CGO_CFLAGS/CGO_LDFLAGS/LD_LIBRARY_PATH setup for the old shared BLS/MCL libs is no longer needed |
| .github/workflows/ci-pr.yaml | Removes harmony-one/mcl and harmony-one/bls checkout steps and their associated make build steps |
| Makefile | Removes all CGO/LD_LIBRARY_PATH exports and mcl/bls build rules; libs target now just runs go mod download |
Sequence Diagram
sequenceDiagram
participant App as Binary startup
participant CoreInit as crypto/bls/core init()
participant MaskInit as crypto/bls init()
participant Herumi as herumi (CGo)
App->>CoreInit: package init (runs first)
CoreInit->>Herumi: herumi.Init(BLS12_381)
CoreInit->>Herumi: SetETHserialization(false)
CoreInit->>Herumi: SetMapToMode(0)
CoreInit->>Herumi: SetETHmode(EthModeOld)
CoreInit->>Herumi: SetGeneratorOfPublicKey(harmonyGenerator)
Herumi-->>CoreInit: ok
CoreInit-->>App: initialized (panic on error)
App->>MaskInit: package init (runs after core)
MaskInit->>CoreInit: bls.Init(BLS12_381) [redundant]
CoreInit->>Herumi: "herumi.Init, Set*, SetGeneratorOfPublicKey (again)"
Herumi-->>CoreInit: error silently dropped
MaskInit-->>App: done
Note over App,Herumi: At runtime: callers use core.SecretKey/PublicKey/Sign wrappers
Note over App,Herumi: PublicKey.Sub uses unsafe.Pointer cast to access herumi.G1Sub
Comments Outside Diff (1)
-
crypto/bls/mask.go, line 18-20 (link)Redundant double-
Initwith silently dropped errorThe
crypto/bls/corepackage's owninit()already callsInit(BLS12_381)and panics on failure, so by the time thisinit()runs the library is fully configured. This second call re-runsherumi.Init,SetETHserialization,SetMapToMode,SetETHmode, andSetGeneratorOfPublicKeyagain, and any error returned is silently discarded. While the calls are idempotent in practice, the swallowed error means a hypothetical failure in the second pass would leave the process in an undefined state without any diagnostic. Theinit()block can simply be removed since thecorepackage dependency already guarantees initialization.
Reviews (1): Last reviewed commit: "Remove hard-coded libcrypto.so.10 md5sum..." | Re-trigger Greptile
Summary
github.com/harmony-one/blswith the officialgithub.com/herumi/bls-eth-go-binarymodulecrypto/bls/coreThe resulting Harmony binary does not require external
libblsorlibmclfiles at runtime.Validation
go test ./crypto/bls/core ./crypto/bls ./crypto/vrf/bls ./consensus/quorum ./consensus/votepower ./staking/slashnode/harmonytestsgo build ./cmd/harmonywithout BLS/MCL CGO flagslibblsorlibmclgo test ./...completed all compile paths; remaining local failures require network or localhost bind access unavailable in the sandbox