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.

Sign up for GitHub

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

schnorr: simplify some signing math #2042

Merged
merged 1 commit into from
Nov 29, 2023
Merged

schnorr: simplify some signing math #2042

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions btcec/schnorr/signature.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,9 @@ func schnorrSign(privKey, nonce *btcec.ModNScalar, pubKey *btcec.PublicKey, hash
// Step 12.
//
// e = tagged_hash("BIP0340/challenge", bytes(R) || bytes(P) || m) mod n
var rBytes [32]byte
r := &R.X
r.PutBytesUnchecked(rBytes[:])
pBytes := SerializePubKey(pubKey)

commitment := chainhash.TaggedHash(
chainhash.TagBIP0340Challenge, rBytes[:], pBytes, hash,
chainhash.TagBIP0340Challenge, R.X.Bytes()[:], pBytes, hash,
Copy link
Collaborator

Choose a reason for hiding this comment

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

While from a code readability perspective this might be a desired change, I think this will cause the rBytes array to escape to the heap (meaning it would need to be garbage collected) while before the change it would've been kept on the stack.
Would be nice to confirm or deny that with a benchmark test...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could swear that Go actually has some tooling that can show explicitly what goes on the heap and what stays on the stack. I'll have a look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I thought about that as well directly after sending my comment above. See here: https://medium.com/@trinad536/escape-analysis-in-golang-fc81b78f3550

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guggero

btcec % go build -v -gcflags -m ./schnorr
schnorr/signature.go:308:43: new([32]byte) does not escape

I think this means we're good?

FWIW:

go version
go version go1.21.4 darwin/arm64

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I'm not very well versed in reading that escape analysis messages. But I would think you'd need to take a look at the escape analysis for the implementation of Bytes() itself to get a clearer picture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guggero The call to Bytes()gets inlined:

schnorr/signature.go:308:43: inlining call to secp256k1.(*FieldVal).Bytes

That is why the escape analysis shows that the new allocation is happening within schnorrSign, on the stack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah cool, good to know. I guess I should run the escape analysis more often myself then, pretty interesting to see what the compiler actually ends up doing.

)

var e btcec.ModNScalar
Expand All @@ -325,7 +321,7 @@ func schnorrSign(privKey, nonce *btcec.ModNScalar, pubKey *btcec.PublicKey, hash
s := new(btcec.ModNScalar).Mul2(&e, privKey).Add(&k)
k.Zero()

sig := NewSignature(r, s)
sig := NewSignature(&R.X, s)

// Step 14.
//
Expand Down
Loading