Skip to content

fix(crypto): avoid BN128 class initialization deadlock - #7099

Open
Federico2014 wants to merge 1 commit into
tronprotocol:release_v4.8.3from
Federico2014:hotfix/bn128_initialization_deadlock
Open

Federico2014 wants to merge 1 commit into
tronprotocol:release_v4.8.3from
Federico2014:hotfix/bn128_initialization_deadlock

Conversation

@Federico2014

@Federico2014 Federico2014 commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Move the inverse-of-two constant from Fp to Params and update its two uses in PairingCheck. This removes the Fp → Params → Fp2 → Fp static initialization cycle while preserving the constant value and pairing arithmetic.

Extend the existing FpTest with one regression test that initializes Fp, Fp2, Fp6, and Fp12 in fresh class loaders that reject Params.

Why are these changes required?

Concurrent first calls to BN128 addition/multiplication and pairing can initialize different classes in the cycle and wait indefinitely for each other. Later calls that require those classes also block. Field initialization can now complete without initializing the curve parameters.

This PR has been tested by:

  • Independently compiled the affected package and ran FpTest and Fp12Test with JDK 8 and JUnit 4.13.2: all 13 tests passed.
  • Ran the initialization regression test against the unchanged base implementation: it failed on the dependency on Params. The same test passes with this patch.
  • Diff whitespace checks and the Math usage check passed.
  • Gradle build, focused test, and required Checkstyle commands were attempted offline but could not resolve uncached dependencies, including commons-lang3:3.20.0. Those tasks have not passed locally.

Follow up

Complete Gradle and platform-matrix validation in CI.

Extra details

The change preserves BN128 arithmetic, input validation, encodings, and energy costs. It changes a package-private constant location and updates every in-repository caller; it adds no dependency, configuration, or protocol changes.

@github-actions
github-actions Bot requested a review from 3for November 10, 2026 09:31
@Federico2014
Federico2014 force-pushed the hotfix/bn128_initialization_deadlock branch from 6435ee5 to ad5b749 Compare November 10, 2026 09:35
@Federico2014
Federico2014 force-pushed the hotfix/bn128_initialization_deadlock branch from ad5b749 to 7f6a877 Compare November 10, 2026 09:44
@lvs0075 lvs0075 added this to the GreatVoyage-v4.8.3 milestone Nov 10, 2026
@3for

3for commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

[Non-blocking / Follow-up] BN128 correctness, API hardening, and test coverage improvements.

I identified a few low-priority issues in the BN128 implementation that can be addressed in a follow-up PR. The current TVM production paths do not appear to expose these issues directly, so they need not block this PR.

1. equals() / hashCode() consistency

BN128.java:245 overrides equals() but inherits Object.hashCode(). Objects considered equal may therefore have different hash codes, violating the Java contract and potentially affecting HashMap/HashSet behavior.

Additionally, equals() compares raw Jacobian coordinates rather than the represented curve points. Please clarify the intended equality semantics before implementing a consistent hashCode().

I found no production verification path relying on this comparison or hash-based collections, so no security impact has been established.

2. Negative scalar handling

BN128.java:162 accepts a BigInteger scalar but neither rejects negative values nor implements the corresponding point-negation semantics.

Please explicitly require and validate non-negative scalars, or define proper negative-scalar support if needed.

The current TVM multiplication entry point uses unsigned decoding, so I found no on-chain path reaching this case.

3. PairingCheck lifecycle

PairingCheck.run() does not reset accumulated state when called repeatedly, and result() does not verify that computation completed successfully.

Consider enforcing single-use semantics and rejecting result access before successful completion.

The current TVM caller creates a fresh instance and reads the result only after a successful run(), so its usage is correct.

4. Fp2 representation documentation

The Fp2 class documentation and toString() describe elements as a*i+b, while the arithmetic represents a+b*i.

Please correct the documentation and debug output without swapping the underlying coefficients. The existing arithmetic is consistent with the TVM decoding order.

5. Pairing correctness tests

Bn128PairingTimeoutForkTest checks execution status and output length but does not assert the actual pairing result.

Please consider adding known-answer tests for both true and false pairing results, along with differential tests against an independent implementation.

These improvements can be tracked separately and addressed in a subsequent PR.

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.

3 participants