ellswift: introduce ElligatorSwift encoding and decoding funcs #2219
Conversation
Pull Request Test Coverage Report for Build 11147308122Details
💛 - Coveralls |
|
This is great! fwiw, I had great success using the csv files from the BIP324 directly for the tests and it gave me great test coverage too, see esp the big test running through all the "packet_encoding_test_vectors.csv" rows here |
|
cc: @ellemouton @ProofOfKeags for review |
Crypt-iQ
force-pushed
the
ellswift
branch
2 times, most recently
from
7227ec8
to
20aa98d
Compare
I don't have a strong opinion on whether we should add the csv files directly. In other parts of the codebase (i.e. other BIP test vectors), we just have the test vectors in the format that I have in this PR |
The BIP324 ElligatorSwift test vectors are also included.
|
cc @ellemouton for review comments on this pr. Thanks |
|
cc: @kcalvinalvin for review |
| ) | ||
|
|
||
| func init() { | ||
| c.SetByteSlice(cBytes[:]) |
There was a problem hiding this comment.
nit: instead of relying on the init function you should also be able to do this in the var section
_ = c.SetByteSlice(cBytes[:])
| return nil, err | ||
| } | ||
|
|
||
| if initiating { |
There was a problem hiding this comment.
nit: I think would it make sense to only have the ellswiftOurs / ellswiftTheirs order toggle in the if to make clear that everything else stays the same?
Something like this:
var msg []byte
if initiating {
// Initiating, place our public key encoding first.
var msg []byte
msg = append(msg, ellswiftOurs[:]...)
msg = append(msg, ellswiftTheirs[:]...)
} else {
msg = append(msg, ellswiftTheirs[:]...)
msg = append(msg, ellswiftOurs[:]...)
}
msg = append(msg, ecdhPoint[:]...)
return chainhash.TaggedHash(ellswiftTag, msg), nil
| return chainhash.TaggedHash(ellswiftTag, msg), nil | ||
| } | ||
|
|
||
| var msg []byte |
There was a problem hiding this comment.
Can pre-allocate here
msg := make([]byte, 0, 64+64+32)
| // point exists on the curve, it returns true and false otherwise. | ||
| // TODO: Use quadratic residue formula instead (see: BIP340)? | ||
| func liftX(x *FieldVal) (*PublicKey, bool) { | ||
| ySqr := new(FieldVal).Add(x).Square().Mul(x).AddInt(7) |
There was a problem hiding this comment.
How about using the DecompressY() function?
DecompressY(x, false, y)
pubKey := NewPublicKey(x, y)
return pubKey, pubKey.IsOnCurve()
| var randPrivKeyBytes [64]byte | ||
|
|
||
| // Generate a random private key | ||
| _, err := rand.Read(randPrivKeyBytes[:]) |
There was a problem hiding this comment.
Dumb question but is the reason why we don't have to get a random number between 1..p-1 is because PrivKeyFromBytes does a modulo if the given bytes are over p-1?
| ) | ||
|
|
||
| // TestXSwiftECVectors checks the BIP324 test vectors for the XSwiftEC function. | ||
| func TestXSwiftECVectors(t *testing.T) { |
There was a problem hiding this comment.
Sanity checked all the values in both of the tests to see if they match the test vector in the bip
| quadYSqr := new(FieldVal).Add(ySqr).MulInt(4) | ||
| firstX := new(FieldVal).Add(u).Add(quadYSqr) | ||
|
|
||
| firstXCurve := new(FieldVal).Add(firstX).Square().Mul(firstX).AddInt(7) |
There was a problem hiding this comment.
Maybe refactor the check for if x is on the curve into a separate function since it's used 3 times?
| } | ||
|
|
||
| // Should have found a square above. | ||
| panic("unreachable - no calculated x-values were square") |
There was a problem hiding this comment.
Not sure about the panic here. If there's a bug somewhere in the library and we reach this part of the code, it'll crash the nodes. Returning an error will allow us to gracefully shut the node down in case we reach here.
| rhs := new(FieldVal).Add(uSqr).MulInt(3).Mul(s) | ||
|
|
||
| // Add the two terms together and multiply by -s. | ||
| lhs.Add(rhs).Normalize().Mul(s).Negate(1) |
There was a problem hiding this comment.
Does it not matter if we call Normalize() during or after? Asking since
lhs.Add(rhs).Mul(s).Negate(1).Normalize()
passes the tests and the BIP specifies that the modulo be done afterwards
| return nil | ||
| } | ||
|
|
||
| switch caseNum & 5 { |
There was a problem hiding this comment.
Feels like we can reduce code duplication here
|
WriteV2MessageN should allow specify the garbage []byte, if nil default garbage is used Rationale: this will permit custom nonstandard packets communication in the protocol, while still being compatible with bitcoin. |
|
// transportVersion is the transport version we are currently using. packet likely needed here to make inbound connections work with respect to bitcoin 0.27.1 acting as client |
|
when bitcoin 27.1 is connecting as a client, we don't find his garbage terminator inside CompleteHandshake() and it gets stuck. |
The BIP324 ElligatorSwift test vectors are also included. They can be found here and here. This code could be more optimized, possibly by not using so many
new(FieldVal)invocations among some other things.