Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ellswift: introduce ElligatorSwift encoding and decoding funcs #2219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Crypt-iQ
Copy link
Collaborator

@Crypt-iQ Crypt-iQ commented Jul 25, 2024

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.

@Crypt-iQ Crypt-iQ added the btcec label Jul 25, 2024
@coveralls
Copy link

coveralls commented Jul 25, 2024

Pull Request Test Coverage Report for Build 11147308122

Details

  • 162 of 275 (58.91%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 57.244%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcec/ellswift.go 162 275 58.91%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 2 87.32%
Totals Coverage Status
Change from base Build 10970025081: 0.01%
Covered Lines: 30023
Relevant Lines: 52447

💛 - Coveralls

@lnliz
Copy link

lnliz commented Jul 31, 2024

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

@saubyk
Copy link

saubyk commented Aug 21, 2024

cc: @ellemouton @ProofOfKeags for review

btcec/ellswift_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

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

Looks to be a correct implementation of the BIP324 gear.

btcec/ellswift.go Show resolved Hide resolved
btcec/ellswift.go Show resolved Hide resolved
btcec/ellswift.go Show resolved Hide resolved
btcec/ellswift.go Outdated Show resolved Hide resolved
btcec/ellswift.go Show resolved Hide resolved
btcec/ellswift.go Show resolved Hide resolved
btcec/ellswift.go Outdated Show resolved Hide resolved
@Crypt-iQ Crypt-iQ force-pushed the ellswift branch 2 times, most recently from 7227ec8 to 20aa98d Compare September 27, 2024 04:00
@Crypt-iQ
Copy link
Collaborator Author

Crypt-iQ commented Oct 2, 2024

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

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.
@saubyk
Copy link

saubyk commented Oct 2, 2024

cc @ellemouton for review comments on this pr. Thanks

@saubyk
Copy link

saubyk commented Oct 7, 2024

cc: @kcalvinalvin for review

)

func init() {
c.SetByteSlice(cBytes[:])
Copy link

Choose a reason for hiding this comment

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

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 {
Copy link

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants