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

Add challenge to proof ownership flows #1077

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Aug 12, 2024

Description

Currently, our ProveAssetOwnership / VerifyAssetOwnership calls create an ownership proof by creating a vPSBT that spends the asset to a NUMS key, and that challenge witness would be appended to the proof for the verifier to check.

In this PR we add a new [32]byte challenge field on both methods.

  • ProveAssetOwnership: if a challenge is defined we instead spend it to NUMS + challenge*G, otherwise just NUMS (noop)
  • VerifyAssetOwnership: if a challenge is defined we will construct a virtual tx that spends to NUMS +challenge*G and verify that state transition, otherwise just NUMS (noop)

Comment

In the original issue there's the expectation to encode the challenge in the proof. Since the verifier is supposed to have knowledge of the challenge, we instead add the challenge as a parameter to the VerifyAssetOwnership. This way we don't have to change the Proof TLV encoders to accept an optional [32]byte challenge, which significantly reduces code pollution and minimizes diff.

Closes #819

@GeorgeTsagk GeorgeTsagk self-assigned this Aug 12, 2024
@coveralls
Copy link

coveralls commented Aug 12, 2024

Pull Request Test Coverage Report for Build 10405979240

Details

  • 49 of 89 (55.06%) changed or added relevant lines in 6 files are covered.
  • 6 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.03%) to 40.194%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannel/aux_funding_controller.go 0 1 0.0%
tapfreighter/wallet.go 0 2 0.0%
tappsbt/create.go 0 4 0.0%
proof/verifier.go 14 22 63.64%
rpcserver.go 0 25 0.0%
Files with Coverage Reduction New Missed Lines %
tapdb/addrs.go 2 79.04%
tappsbt/create.go 2 53.22%
commitment/tap.go 2 84.43%
Totals Coverage Status
Change from base Build 10385622104: 0.03%
Covered Lines: 23731
Relevant Lines: 59041

💛 - Coveralls

@GeorgeTsagk
Copy link
Member Author

GeorgeTsagk commented Aug 12, 2024

Needs rebase after merging #1075
Will do with 1st round of fixes

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks a lot!
Have a couple of improvements and nits, nothing major.

address/address.go Outdated Show resolved Hide resolved
address/address.go Outdated Show resolved Hide resolved
address/address.go Show resolved Hide resolved
proof/proof.go Outdated Show resolved Hide resolved
proof/verifier.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
taprpc/assetwalletrpc/assetwallet.proto Outdated Show resolved Hide resolved
itest/ownership_test.go Outdated Show resolved Hide resolved
itest/ownership_test.go Outdated Show resolved Hide resolved
@dstadulis dstadulis added this to the v0.4.2 milestone Aug 13, 2024
@GeorgeTsagk GeorgeTsagk force-pushed the ownership-proof-challenge branch from 25caeae to c5d1d5b Compare August 15, 2024 15:08
@GeorgeTsagk GeorgeTsagk requested a review from guggero August 15, 2024 15:09
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, LGTM 🎉

@guggero
Copy link
Member

guggero commented Aug 15, 2024

Oh, linter is unhappy. A line is too long.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 💫

Nice n clean!

Left one comment re a simplification we can make:

  • We don't need to obtain G, to then create the point G*challenge
  • Instead, we can just do G*challenge, which is known as a scalar base mult

asset.NUMSPubKey.AsJacobian(&nums)

// Multiply G by 1 to get G as a Jacobian point.
secp256k1.ScalarBaseMultNonConst(
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify slightly: we can just do: challenge*G here, which is the same operations as creating a new EC point from a private scalar. Under the hood it handles obtaining the generator as a Jacobian point: https://github.com/decred/dcrd/blob/9aba0ced85c954fb3bff07eb2e9af7688fa21c94/dcrec/secp256k1/curve.go#L1235-L1238

Copy link
Member

Choose a reason for hiding this comment

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

In other words, we only need to do:

var challengePoint btcec.JacobianPoint
secp256k1.ScalarBaseMultNonConst(challenge, challengePoint)

@@ -370,7 +373,7 @@ func CreateOwnershipProofAsset(
}

outputAsset := ownedAsset.Copy()
outputAsset.ScriptKey = asset.NUMSScriptKey
outputAsset.ScriptKey = address.GenChallengeNUMS(challengeBytes)
Copy link
Member

Choose a reason for hiding this comment

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

👍

return asset.NUMSScriptKey
}

var challengeBytes [32]byte
Copy link
Member

Choose a reason for hiding this comment

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

Can can also further bind the challenge if we use h(nums || challenge) instead of just challenge.

@Roasbeef Roasbeef merged commit 038000c into lightninglabs:main Aug 20, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[feature]: ownership/prove & ownership/verify should accept random challenge
5 participants