Update KeyPair.java

[jahoefne]

Fix:
If a new KeyPair was created based on an existing PrivateKey the publicKey-field stayed null forever. Each time .getPublicKey() was called the PublicKey was recalculated.

Moved this code in the constructor where it belongs to.

[abstractj]

@jahoefne Hi, thanks in advance. Would you mind to add unit tests to reproduce the issue?

[Kilobyte22]

I definitely agree and would like to see this merged. I already saw this myself and it was bugging me

[abstractj]

@Kilobyte22 what was bugging you? Could you please be more specific? From the design decision, that makes sense, I'm just not sure how to reproduce the issue with the publicKey field being null forever like @jahoefne.

If you guys have a testing case scenario, I would be more than happy to look at this.

[jahoefne]

@abstractj
It is not really unit-testable because the code does work, however it is inefficient and kind of logically wrong.

The problem is:
If you create a new KeyPair kp from an existing PrivateKey. The kp.publicKey is never set (verify this with a debugger). Instead of calculating this field only once in the constructor, it is calculated each time it is accessed.

This line is the problem:

byte[] key = publicKey != null ? publicKey : point.mult(secretKey).toBytes();

If the KeyPair was created from an existing PrivateKey this will always call point.mult because publicKey is always null.
KeyPair is immutable therefore this calculation always returns the same value for any given instance, so move it in the constructor to avoid this expensive and useless calculations.

Further the 'inner' logical object state as a representation of a KeyPair is wrong because kp.publicKey (being null) and kp.privateKey do not fit together.

[jahoefne]

At least on the JVM I'm using this will fail for the existing code but work for the patch.

@Test
public void testKeyPairs(){
    KeyPair kp = new KeyPair();
    KeyPair kp2 = new KeyPair(kp.getPrivateKey().toBytes());
    Assert.assertEquals(kp, kp2);
}

However this will depend on how the JVM implements .equals and should not be used.

[abstractj]

@jahoefne I'm not against the change, but the patch just fix an API design decision. The assumption of the public key always null is not true, because it will be calculated. I can't see how that is possible and would be happy if you have a scenario where NPE will be raised.

About the public key being calculated every time, that's correct and I have no problems on merging it. But this won't fix the real problem, at least for me this unit test will fail even after your patch.

I real issue into this way: when a private key is provided from the generated key pair, the public key is not the same. Am I wrong? To reproduce we can do:

@Test
public void testKeyPairs(){
    KeyPair kp = new KeyPair();
    KeyPair kp2 = new KeyPair(kp.getPrivateKey().toBytes());
    assertEquals(kp, kp2);
}

Or

@Test
public void testKeyPairs(){
    KeyPair kp = new KeyPair();
    KeyPair kp2 = new KeyPair(kp.getPrivateKey().toBytes());
    assertEquals(HEX.encode(kp.getPublicKey().toBytes()), HEX.encode(kp2.getPublicKey().toBytes()));
 }

The outcome will be pretty much the same, for now I will merge your change and add the unit test with @ignore as a friendly reminder to myself to fix it in the further releases.

Thank you