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

x/crypto/ed25519: a correction and a suggestion for adding comments for crypto/ed25519 #39554

Closed
zhangzhanli opened this issue Jun 12, 2020 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@zhangzhanli
Copy link

zhangzhanli commented Jun 12, 2020

Firstly, a correction should be in edwards25519.go:725. There's a mistake in the comment of "FeSub(&u, &u, &p.Z) // y = y^2-1". According to ed's paper, it should have been u = y^2-1.

Secondly, in edwards25519.go:562 " func fePow22523(out, z *FieldElement)" , we can't know what it will do just by the name of the function. Although it's a internal func, I still suggest adding comments for it, like this:

// to cal z ^ (2^252 - 3)
func fePow22523(out, z *FieldElement) {
	var t0, t1, t2 FieldElement
	var i int

	FeSquare(&t0, z)	//2
	for i = 1; i < 1; i++ {
		FeSquare(&t0, &t0)
	}
	FeSquare(&t1, &t0)	//4
	for i = 1; i < 2; i++ {
		FeSquare(&t1, &t1)	//8
	}
	FeMul(&t1, z, &t1)	//9
	FeMul(&t0, &t0, &t1)	//11
	FeSquare(&t0, &t0)	//22
	for i = 1; i < 1; i++ {
		FeSquare(&t0, &t0)
	}
	FeMul(&t0, &t1, &t0)	//31
	FeSquare(&t1, &t0)
	for i = 1; i < 5; i++ {
		FeSquare(&t1, &t1)	//1f
	}
	FeMul(&t0, &t1, &t0)	//0x3ff denoted by 10 1
 	FeSquare(&t1, &t0)
	for i = 1; i < 10; i++ {
		FeSquare(&t1, &t1)
	}
	FeMul(&t1, &t1, &t0)	//20 1
	FeSquare(&t2, &t1)
	for i = 1; i < 20; i++ {
		FeSquare(&t2, &t2)
	}
	FeMul(&t1, &t2, &t1)	//40 1
	FeSquare(&t1, &t1)
	for i = 1; i < 10; i++ {
		FeSquare(&t1, &t1)
	}
	FeMul(&t0, &t1, &t0)	//50 1
	FeSquare(&t1, &t0)
	for i = 1; i < 50; i++ {
		FeSquare(&t1, &t1)
	}
	FeMul(&t1, &t1, &t0)	//100 1
	FeSquare(&t2, &t1)
	for i = 1; i < 100; i++ {
		FeSquare(&t2, &t2)
	}
	FeMul(&t1, &t2, &t1)	//200 1
	FeSquare(&t1, &t1)
	for i = 1; i < 50; i++ {
		FeSquare(&t1, &t1)
	}
	FeMul(&t0, &t1, &t0)	//250 1
	FeSquare(&t0, &t0)
	for i = 1; i < 2; i++ {
		FeSquare(&t0, &t0)
	}
	FeMul(out, &t0, z)	//2^252 - 3
}
@toothrot toothrot changed the title a correction and a suggestion for adding comments for crypto/ed25519 x/crypto/ed25519: a correction and a suggestion for adding comments for crypto/ed25519 Jun 12, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 12, 2020
@toothrot
Copy link
Contributor

/cc @FiloSottile

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 12, 2020
FiloSottile added a commit to FiloSottile/edwards25519 that referenced this issue Dec 13, 2020
Also, document the Pow22523 ladder. See also golang/go#39554.
@FiloSottile
Copy link
Contributor

Thank you @zhangzhanli! I spotted and fixed the typo while working one the same code some time ago (https://go-review.googlesource.com/c/go/+/273087) and then commented the Pow22523 body in a derived library that might at some point replace internal/edwards25519 (FiloSottile/edwards25519@8cc8037#diff-d06d8ddac2c1a54fad38937cab472d1d7c8afc5641984292e3ced64cc175a2a1R334-R382) partially based on your comments.

@golang golang locked and limited conversation to collaborators Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants