Skip to content

Conversation

@Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Sep 22, 2025

This is requested and needed by NeoX.

Description

Adds Ethereum-compatible aliases for the BLS12-381 native contract methods (bls12_*) while
retaining the existing bls12381* surface. This keeps Neo and EVM tooling interoperable without
schema changes. Updated tests cover both naming schemes, and the expected genesis manifest now
includes the new ABI entries.

Fixes # (issue)

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • dotnet test tests/Neo.UnitTests/Neo.UnitTests.csproj --filter CryptoLib

Test Configuration:

  • .NET SDK 9.0.301
  • Default protocol settings via test harness

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Jim8y Jim8y force-pushed the feature/bls12-eth-aliases branch from a157ce3 to 1a443fe Compare September 22, 2025 04:32
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Looks good, it can be in the next release

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

What's preventing contracts from using the already existing names?


[ContractMethod(Hardfork.HF_Gorgon, CpuFee = 1 << 19, Name = "bls12_g1add")]
public static InteropInterface Bls12G1Add(InteropInterface x, InteropInterface y)
=> Bls12381Add(x, y);
Copy link
Member

Choose a reason for hiding this comment

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

Adds Ethereum-compatible aliases

CryptoLib calls won't be compatible with Ethereum anyway, so I don't think this PR can be accepted. It adds an ambiguity to the CryptoLib interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, the same thing (if we really care about specific names which is questionable to me since it's about N3 contracts, they're different from EVM/Solidity contracts anyway) can be provided by devpack without contract modifications (and code/manifest bloat).

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

It only change the name?

Wi1l-B0t
Wi1l-B0t previously approved these changes Sep 26, 2025
Copy link
Contributor

@Wi1l-B0t Wi1l-B0t left a comment

Choose a reason for hiding this comment

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

It seems to be just for compatibility.

@Jim8y
Copy link
Contributor Author

Jim8y commented Sep 26, 2025

What's preventing contracts from using the already existing names?

hi roman @roman-khimov , this is a requested change for neox, i am not sure the detail, but i guess its related to eip standards, you or anna should know it better than me.

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Aliases are not needed. We need to add compatibility with https://eips.ethereum.org/EIPS/eip-2537 and the only thing that can be incompatible now is bls12381MultiExp. So there is some functional extension we need here rather than aliases.

@txhsl
Copy link

txhsl commented Sep 26, 2025

We need to add compatibility with https://eips.ethereum.org/EIPS/eip-2537 and the only thing that can be incompatible now is bls12381MultiExp.

Yes, the EIP-2537 has been updated several times since Feb 2024, but now it gets finalized. The history can be referred https://github.com/ethereum/EIPs/commits/master/EIPS/eip-2537.md.

@erikzhang
Copy link
Member

@neo-project/ngd-shanghai Need testing.

{
var scalar = ParseScalar(pair[1]);
if (!scalar.IsZero)
g1Accumulator += new G1Projective(g1Affine) * scalar;
Copy link

@txhsl txhsl Oct 20, 2025

Choose a reason for hiding this comment

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

We need to make sure that subgroup check is executed before any multiplication operation. This was fixed in Ethereum through ethereum/EIPs#8456.

Briefly speaking, we need:

  1. "IsOnCurve" check after G1 point decoding and G2 point decoding, e.g. https://github.com/ethereum/go-ethereum/blob/v1.16.5/core/vm/contracts.go#L1212;
  2. "IsInSubGroup" check before multiply and pairing computation, e.g. https://github.com/ethereum/go-ethereum/blob/v1.16.5/core/vm/contracts.go#L1005 and https://github.com/ethereum/go-ethereum/blob/v1.16.5/core/vm/contracts.go#L1173.

About the detailed implementation of these checks, please ref https://github.com/Consensys/gnark-crypto/blob/v0.19.0/ecc/bls12-381/g1.go#L193-L218 and https://github.com/Consensys/gnark-crypto/blob/v0.19.0/ecc/bls12-381/g2.go#L200-L223.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// <param name="pairs">Array of [point, scalar] pairs.</param>
/// <returns>The accumulated point.</returns>
[ContractMethod(Hardfork.HF_Faun, CpuFee = 1 << 23)]
public static InteropInterface Bls12381MultiExp(Array pairs)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to check the length, a max is required or it could deny the service with 1024 pairs

Copy link
Member

Choose a reason for hiding this comment

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

Cpu should be paid in each iteration

@cschuchardt88
Copy link
Member

the right way to execute a test by filters is

 dotnet test tests/Neo.UnitTests/Neo.UnitTests.csproj -- --filter CryptoLib

@superboyiii
Copy link
Member

failed_tests_20251107_120156.log
I made 10 more tests, 2 failed. This log is including failed data and created scripts. You can invoke this script to CryptoLib to get stack results in UT to compare. @Jim8y

Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

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

My fault. My go test code has a little bug and it's fixed. Now I tried several test cases, all passed.

@Wi1l-B0t
Copy link
Contributor

Wi1l-B0t commented Nov 7, 2025

My fault. My go test code has a little bug and it's fixed. Now I tried several test cases, all passed.

Can you show your testcases.

Manual testing is not enough and testcase scripts also need to be added.

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 8, 2025

@txhsl

@Jim8y Jim8y added Port-to-3.x Feature or PR must be ported to Neo 3.x branch Hardfork Core labels Nov 8, 2025
@shargon
Copy link
Member

shargon commented Nov 8, 2025

Agree with @Wi1l-B0t , the testcases should be added

/// <param name="pairs">Array of [point, scalar] pairs.</param>
/// <returns>The accumulated point.</returns>
[ContractMethod(Hardfork.HF_Faun, CpuFee = 1 << 23)]
public static InteropInterface Bls12381MultiExp(Array pairs)
Copy link
Member

Choose a reason for hiding this comment

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

Cpu should be paid in each iteration

@superboyiii
Copy link
Member

superboyiii commented Nov 10, 2025

My fault. My go test code has a little bug and it's fixed. Now I tried several test cases, all passed.

Can you show your testcases.

Manual testing is not enough and testcase scripts also need to be added.

I made a repo for my test tools: BLS12-381-TEST. It's including a go tool. It randomly picks G1 or G2 points and scalars and get bls12381MultiExp results due to EVM Lib: consensys/gnark-crypto/ecc/bls12-381. Then the shell will compare results with Bls12381MultiExpHelper - A C# implementation from this PR which will generate scripts and invoke CLI to simulate a real production environment. I make all random and atuomatic, test many times, so this PR is LGTM.

Copy link

@txhsl txhsl left a comment

Choose a reason for hiding this comment

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

  1. The input length of Bls12381Pairing needs to be extended to pairs;
  2. The EVM test data can be used like #4185.

Otherwise, LGTM. The encoding seems good now.

{
var scalar = ParseScalar(pair[1]);
if (!scalar.IsZero)
g1Accumulator += new G1Projective(g1Affine) * scalar;
Copy link

@txhsl txhsl Nov 10, 2025

Choose a reason for hiding this comment

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

Similar to what I've mentioned in #4185 (comment), MultiExp() is also a choice here, ref https://github.com/ethereum/go-ethereum/blob/v1.16.7/core/vm/contracts.go#L1015. Only for performance.

@superboyiii
Copy link
Member

Tested. g1add, g1mul, g2add, g2mul and pairing all work well.

@Wi1l-B0t
Copy link
Contributor

Wi1l-B0t commented Nov 13, 2025

Tested. g1add, g1mul, g2add, g2mul and pairing all work well.

Can you show your testcases and testdata ?

@superboyiii
Copy link
Member

superboyiii commented Nov 14, 2025

Tested. g1add, g1mul, g2add, g2mul and pairing all work well.

Can you show your testcases and testdata ?

Again: https://github.com/superboyiii/BLS12-381-TEST#
Already updated. All random pick.
Compare consensus/gnark/BLS12381 results.

@shargon
Copy link
Member

shargon commented Nov 14, 2025

Can this test be added as UT?

G1Projective g => new(g),
_ => throw new ArgumentException("BLS12-381 type mismatch")
};
EnsureG1PointValid(in g1a);
Copy link
Member

@AnnaShaleva AnnaShaleva Nov 14, 2025

Choose a reason for hiding this comment

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

This change (along with the change a couple of lines below) needs a mainnet states compatibility check from @superboyiii because it changes the existing CryptoLib API.

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably needs a hardfork guard in this case.

}

[ContractMethod(Hardfork.HF_Faun, CpuFee = 1 << 21, Name = "bls12_g1mul")]
public static byte[] Bls12G1Mul(byte[] input)
Copy link
Member

@AnnaShaleva AnnaShaleva Nov 14, 2025

Choose a reason for hiding this comment

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

So far, we had bls12381Add, bls12381Mul and etc. in CryptoLib manifest. And right now we add bls12_g1add, bls12_g1mul and etc., so it's not clear from a side user PoW that these methods work with Eth-compatible serialization format. So at least the naming must be improved and unified.

The bigger question is compatibility with existing CryptoLib interface. Why can't we introduce deserialisation/serialization APIs that will deserialize/serialize from/to Eth-compatible format? This allow to remove these duplicating bls12_g1add, bls12_g1mul and etc. which makes CryptoLib's API cleaner.

This problem is similar to https://github.com/neo-project/neo/pull/4185/files/546a6d07afa3a7171891b1f761f5bb26e63528ab#r2526423615.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @AnnaShaleva, it will be easier and clearer. But we should have a clear comment on what format each method accepts

@superboyiii
Copy link
Member

superboyiii commented Nov 20, 2025

We need add check for G2 like this, when infinity, return Identity. But seems need a hardfork for safe.
551f656e-9ce6-43a4-a81f-492f9d42eed7

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

change to master please

@superboyiii
Copy link
Member

superboyiii commented Nov 21, 2025

We need change remote branch to master, then I will make compatiblity test on mainnet to ensure no storage issue.

@Wi1l-B0t
Copy link
Contributor

Any progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core Hardfork Port-to-3.x Feature or PR must be ported to Neo 3.x branch Waiting for Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.