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

xmrswap: Remove C deps for signature encryption and dleq #2936

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

martonp
Copy link
Collaborator

@martonp martonp commented Aug 28, 2024

Adds functions for encrypting / decrypting Schnorr signatures, and uses the athanorlabs/go-dleq library for discrete log equivalence.

@davecgh
Copy link
Member

davecgh commented Aug 28, 2024

Glad to see this! I did notice the dependency has various inefficiencies looking through it, but given that it won't be used in any type of hot paths, that is unlikely to matter much.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Wow.

closes #2918

internal/cmd/xmrswap/main.go Outdated Show resolved Hide resolved
dex/testing/loadbot/go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
@martonp martonp force-pushed the removeCDeps branch 2 times, most recently from a14385a to b16275a Compare September 1, 2024 09:19
@martonp
Copy link
Collaborator Author

martonp commented Sep 1, 2024

@davecgh Thanks for the review!

@martonp martonp marked this pull request as ready for review September 1, 2024 09:27
client/cmd/bisonw-desktop/go.mod Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
@martonp martonp force-pushed the removeCDeps branch 3 times, most recently from c0e9f55 to c0b6848 Compare September 2, 2024 19:02
@martonp
Copy link
Collaborator Author

martonp commented Sep 2, 2024

@JoeGruffins since these adaptor signatures use schnorr signatures rather than ecdsa as was in the C library, I've updated the tx scripts to use OP_CHECKSIGALT instead of OP_CHECKMULTISIG.

internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor_test.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I haven't looked at the DLEQ dep for correctness, but aside from the last nit in my most recent comment, the adaptor sig code looks accurate and reasonably efficient.


import "github.com/decred/dcrd/txscript/v4"

func LockRefundTxScript(kal, kaf []byte, locktime int64) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Since probably nothing other than the internal code will use it, I'm sure you guys probably aren't worried about it, but I'd point out that with no function comment and variables named kal and kaf, it's pretty unclear what they are here without looking at the code to determine they're Schnorr sigs over secp256k1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think @JoeGruffins will do some refactor regarding variable names throughout after this one is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

@martonp
Copy link
Collaborator Author

martonp commented Sep 3, 2024

I haven't looked at the DLEQ dep for correctness, but aside from the last nit in my most recent comment, the adaptor sig code looks accurate and reasonably efficient.

OK thanks for the reviews! I think DLEQ dep will have to be audited before we release anything, but I think it is fine for this POC currently.

@davecgh
Copy link
Member

davecgh commented Sep 5, 2024

I followed up a bit on this comment, but it's resolved, so I figured I'd post a link to it here so it doesn't get missed.

internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
internal/adaptorsigs/adaptor.go Outdated Show resolved Hide resolved
@JoeGruffins
Copy link
Member

@martonp is on vacay, we'll pick up when they're back

@martonp martonp force-pushed the removeCDeps branch 2 times, most recently from 134d9ce to 4fdfbb8 Compare September 20, 2024 10:27
@JoeGruffins
Copy link
Member

@martonp srry, conflicts need rebase

@JoeGruffins JoeGruffins merged commit cfa6f07 into decred:master Jan 31, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants