Nullable `ECPoint` and `ECFieldElement` #3758

shargon · 2025-02-13T14:36:27Z

Description

Check nullability in ECPoint and ECFieldElement

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Current

Test Configuration:

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

cschuchardt88 · 2025-02-14T06:30:44Z

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
  Failed TestECFieldElementConstructor [4 ms]
  Error Message:
   Assert.ThrowsException failed. Expected exception type:<System.ArgumentNullException>. Actual exception type:<System.NullReferenceException>. 
  Stack Trace:
     at Neo.UnitTests.Cryptography.ECC.UT_ECFieldElement.TestECFieldElementConstructor() in /home/runner/work/neo/neo/tests/Neo.UnitTests/Cryptography/ECC/UT_ECFieldElement.cs:line 44
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

  Failed TestOpMultiply [< 1 ms]
  Error Message:
   Assert.ThrowsException failed. Expected exception type:<System.ArgumentNullException>. Actual exception type:<System.ArgumentException>. 
  Stack Trace:
     at Neo.UnitTests.Cryptography.ECC.UT_ECPoint.TestOpMultiply() in /home/runner/work/neo/neo/tests/Neo.UnitTests/Cryptography/ECC/UT_ECPoint.cs:line 337
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

src/Neo.CLI/CLI/MainService.Vote.cs

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

shargon · 2025-02-19T10:37:11Z

@cschuchardt88 is not the first time that your "fixes" break the build, please fix it

cschuchardt88 · 2025-02-19T10:50:06Z

It was styling issue for variable name and yes we all make mistakes. But I put these rules in place to protect us... all of us, even me.

Should be fixed now.

@cschuchardt88 is not the first time that your "fixes" break the build, please fix it

I can count on one hand, even with missing fingers too.

cschuchardt88 · 2025-02-19T10:57:16Z

src/Neo.CLI/CLI/MainService.Vote.cs

+            var hexPubKey = ((ByteString)resJArray[2])?.GetSpan().ToHexString();
+            if (string.IsNullOrEmpty(hexPubKey))
+            {
+                ConsoleHelper.Error("Error parsing the result");
+                return;
+            }
+
+            var publickey = ECPoint.Parse(hexPubKey, ECCurve.Secp256r1);


ECPoint.TryParse? Index for resJArray[2] could be out of range as well.

cschuchardt88 · 2025-02-19T10:59:43Z

tests/Neo.UnitTests/Cryptography/ECC/UT_ECPoint.cs

-            action = () => new ECPoint(null, Y, null);
-            Assert.ThrowsException<ArgumentException>(action);


Should still check if the output is correct. No test functionality should be removed. But should be adjusted for new logic.

Is not null now

Well whatever it should be, can we check that? What will new ECPoint(null, Y, null) be?

We can, but it has no logic to have tests with null, when the argument is not null, but it's ok, if this is your wish I will change it

A test with a pragma for avoid the warning compilation seems weird, why are you asking for this test? Is not null argument, no coverage is increased

To make sure the ECPoint is returning the right thing. What if doesn't return null.

It's a constructor

It's a constructor

What i meant is what does new ECPoint(null, Y, null) equal? for example new ECPoint(null, Y, null) == CCurve.G.

Can we test to make sure it equals the right thing?

cschuchardt88 · 2025-02-19T10:59:55Z

tests/Neo.UnitTests/Cryptography/ECC/UT_ECPoint.cs

-            ECPoint p = null;
-            byte[] n = new byte[] { 1 };
-            Action action = () => p = p * n;
-            Assert.ThrowsException<ArgumentNullException>(action);
+            var p = ECCurve.Secp256k1.G;

-            p = ECCurve.Secp256k1.G;
-            n = null;
-            action = () => p = p * n;
-            Assert.ThrowsException<ArgumentNullException>(action);
-
-            n = new byte[] { 1 };
-            action = () => p = p * n;


Jim8y · 2025-02-25T09:11:25Z

@shargon conflicts in this pr please.

cschuchardt88 · 2025-02-26T07:47:11Z

tests/Neo.UnitTests/Cryptography/ECC/UT_ECPoint.cs

-            n = new byte[] { 1 };
-            action = () => p = p * n;
+            byte[] n = [1];
+            var action = () => p = p * n;


Suggested change

var action = () => p = p * n;

Action action = () => p = p * n;

shargon and others added 3 commits February 13, 2025 15:30

Nullable ecpoint

03820d7

more vars

789fae1

Merge branch 'master' into nullable-ecpoint

8bcf136

cschuchardt88 assigned shargon Feb 13, 2025

cschuchardt88 added the Waiting for Review label Feb 13, 2025

cschuchardt88 requested review from cschuchardt88, nan01ab and Jim8y February 13, 2025 16:46

shargon added 2 commits February 13, 2025 18:37

Clean TryParse

5452131

Merge branch 'master' into nullable-ecpoint

f4f3aa9

cschuchardt88 previously approved these changes Feb 14, 2025

View reviewed changes

cschuchardt88 added the Needs Attention label Feb 14, 2025

shargon and others added 3 commits February 14, 2025 03:44

Merge branch 'master' into nullable-ecpoint

f16b6f5

Merge branch 'master' into nullable-ecpoint

45be5fa

Remove null tests

a9921f0

shargon dismissed cschuchardt88’s stale review via a9921f0 February 14, 2025 12:05

shargon removed the Needs Attention label Feb 14, 2025

shargon and others added 2 commits February 15, 2025 02:12

Merge branch 'master' into nullable-ecpoint

003f186

Merge branch 'master' into nullable-ecpoint

9fdda7d

cschuchardt88 reviewed Feb 15, 2025

View reviewed changes

src/Neo.CLI/CLI/MainService.Vote.cs Outdated Show resolved Hide resolved

shargon added 3 commits February 17, 2025 07:22

Merge branch 'master' into nullable-ecpoint

50aa5d5

Merge branch 'master' into nullable-ecpoint

244e5d8

Merge branch 'master' into nullable-ecpoint

c17fe3f

nan01ab previously approved these changes Feb 19, 2025

View reviewed changes

Update src/Neo.CLI/CLI/MainService.Vote.cs

c2b2927

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

shargon dismissed nan01ab’s stale review via c2b2927 February 19, 2025 03:55

fixed

7327b0a

cschuchardt88 reviewed Feb 19, 2025

View reviewed changes

cschuchardt88 added the Core label Feb 22, 2025

Merge branch 'master' into nullable-ecpoint

a889081

cschuchardt88 reviewed Feb 26, 2025

View reviewed changes

Merge branch 'master' into nullable-ecpoint

d28bb5d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nullable `ECPoint` and `ECFieldElement` #3758

Nullable `ECPoint` and `ECFieldElement` #3758

shargon commented Feb 13, 2025

cschuchardt88 commented Feb 14, 2025

shargon commented Feb 19, 2025 •

edited

Loading

cschuchardt88 commented Feb 19, 2025 •

edited

Loading

cschuchardt88 Feb 19, 2025 •

edited

Loading

cschuchardt88 Feb 19, 2025

shargon Feb 19, 2025

cschuchardt88 Feb 19, 2025 •

edited

Loading

shargon Feb 19, 2025

shargon Feb 26, 2025

cschuchardt88 Feb 26, 2025

shargon Feb 27, 2025

cschuchardt88 Feb 27, 2025 •

edited

Loading

cschuchardt88 Feb 19, 2025

Jim8y commented Feb 25, 2025

cschuchardt88 Feb 26, 2025

		action = () => new ECPoint(null, Y, null);
		Assert.ThrowsException<ArgumentException>(action);

	var action = () => p = p * n;
	Action action = () => p = p * n;

Nullable ECPoint and ECFieldElement #3758

Are you sure you want to change the base?

Nullable ECPoint and ECFieldElement #3758

Conversation

shargon commented Feb 13, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

cschuchardt88 commented Feb 14, 2025

shargon commented Feb 19, 2025 • edited Loading

cschuchardt88 commented Feb 19, 2025 • edited Loading

cschuchardt88 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Feb 25, 2025

Choose a reason for hiding this comment

Nullable `ECPoint` and `ECFieldElement` #3758

Nullable `ECPoint` and `ECFieldElement` #3758

shargon commented Feb 19, 2025 •

edited

Loading

cschuchardt88 commented Feb 19, 2025 •

edited

Loading

cschuchardt88 Feb 19, 2025 •

edited

Loading

cschuchardt88 Feb 19, 2025 •

edited

Loading

cschuchardt88 Feb 27, 2025 •

edited

Loading