Skip to content

Replace Harmony BLS forks with official Herumi package - #2

Open
Frozen wants to merge 7 commits into
devfrom
update/bls-go-binary
Open

Replace Harmony BLS forks with official Herumi package#2
Frozen wants to merge 7 commits into
devfrom
update/bls-go-binary

Conversation

@Frozen

@Frozen Frozen commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • replace github.com/harmony-one/bls with the official github.com/herumi/bls-eth-go-binary module
  • remove separate Harmony BLS/MCL checkout, build flags, shared-library packaging, and runtime library paths
  • centralize Harmony consensus configuration in crypto/bls/core
  • preserve the historical generator, serialization, group layout, hash-to-curve mode, address derivation, and public-key subtraction
  • add golden wire-compatibility vectors for secret keys, public keys, message signatures, and hash signatures
  • update CI, Docker, Makefile, scripts, and documentation for the single static Go dependency

The resulting Harmony binary does not require external libbls or libmcl files at runtime.

Validation

  • go test ./crypto/bls/core ./crypto/bls ./crypto/vrf/bls ./consensus/quorum ./consensus/votepower ./staking/slash
  • compile node/harmony tests
  • direct go build ./cmd/harmony without BLS/MCL CGO flags
  • verified on macOS arm64 that the resulting binary has no dynamic dependency on libbls or libmcl
  • full go test ./... completed all compile paths; remaining local failures require network or localhost bind access unavailable in the sandbox

Comment thread crypto/bls/core/core.go
Comment on lines +172 to +178
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)))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 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-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces the Harmony-forked github.com/harmony-one/bls CGo library (which required separate mcl and bls source tree checkouts, compile-time BLS_SWAP_G=1 flags, and bundled shared libraries) with the official github.com/herumi/bls-eth-go-binary static module. A new crypto/bls/core wrapper package reproduces all historical Harmony consensus parameters at runtime — the custom G1 generator, disabled ETH serialization, EthModeOld hash-to-curve mode — and adds nil-safe wrappers that copy slice arguments before crossing the CGo boundary.

  • crypto/bls/core/core.go: New central wrapper; configures Herumi with Harmony's generator via SetGeneratorOfPublicKey, wraps SecretKey/PublicKey/Sign with nil-safe methods, and implements PublicKey.Sub via an unsafe.Pointer cast to herumi.G1.
  • crypto/bls/core/core_test.go: Golden wire-compatibility vectors for sk=1 assert that the public key, message signature, and hash signature bytes match the historical Harmony BLS output.
  • All 81 files: Import paths uniformly updated from github.com/harmony-one/bls/ffi/go/bls to github.com/harmony-one/harmony/crypto/bls/core; CI, Dockerfile, Makefile, and scripts drop all external BLS/MCL build steps.

Confidence Score: 4/5

The 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 unsafe.Pointer cast in PublicKey.Sub works today because herumi.PublicKey and herumi.G1 have identical single-field layouts, but there is no compile-time guarantee; a future herumi update could silently corrupt aggregated keys used in mask operations. Separately, mask.go's init() re-invokes the full Init sequence and discards any returned error, which is harmless in practice but would hide a failure in edge cases. Neither issue affects the happy path covered by the existing test suite.

Files Needing Attention: crypto/bls/core/core.go (Sub unsafe cast) and crypto/bls/mask.go (redundant init with dropped error)

Important Files Changed

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
Loading

Comments Outside Diff (1)

  1. crypto/bls/mask.go, line 18-20 (link)

    P2 Redundant double-Init with silently dropped error

    The crypto/bls/core package's own init() already calls Init(BLS12_381) and panics on failure, so by the time this init() runs the library is fully configured. This second call re-runs herumi.Init, SetETHserialization, SetMapToMode, SetETHmode, and SetGeneratorOfPublicKey again, 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. The init() block can simply be removed since the core package dependency already guarantees initialization.

Reviews (1): Last reviewed commit: "Remove hard-coded libcrypto.so.10 md5sum..." | Re-trigger Greptile

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.

2 participants