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

Psetv2 signer, finaliser and extractor #184

Closed
wants to merge 54 commits into from

Conversation

sekulicd
Copy link
Collaborator

@sekulicd sekulicd commented Nov 25, 2021

This adds signer, finaliser and extractor role.
SIGHASH_RANGEPROOF is considered in functions that calculates hash for signature.
Currently rangeproofs are added to the hash if appropriate sighash_type is passed to the HashForWitnessV0/HashForSignature func, IMO i think rangeproofs hash should always be added to the sig hash for security reasons.

@tiero, @altafan please review.

Copy link
Collaborator

@altafan altafan left a comment

Choose a reason for hiding this comment

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

@sekulicd the Blinder should not directly handle blinding private keys and generate blinding factors, commitments and proofs for outputs or inputs issuances.

Rather, it should behave similar to the Updater/Signer (Sign() method), therefore it should take care of performing all the sanity checks to add blinding data to the partial transaction. In addition, since this is very related to the new v2 format, it could take care of calculating and keeping up-to-date the correct global scalar of the partial transaction. The user would be really happy about this cause it shouldn't be aware of the matemagics behind the blinding.

For instance, blinding an output means adding asset/value commitments, valueblind/range/surjection proofs and nonce to an existing PSET output (and update the PSET global scalar).
Our blinder should perform all the necessary checks to be sure that the PSET output is actually one of those to be blinded (as per spec) and that the provided blinidng data are valid.
Something similar would be done to blind input issuances.

After we have these 2 main "building block" methods, we can think about adding more abstract methods like to blind many outputs, or to blind an issuance and the related outputs. But this is not required in this PR. Let's keep things simple 'cause this is already a huge effort.

Copy link
Collaborator

@altafan altafan left a comment

Choose a reason for hiding this comment

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

I would stick with the btcutil reference implementation and just define Sign() as a method of the Updater in psetv2/signer.go. Basically, the signer could be a copy-paste of the v1.

Also, psetv2/blinder.go could follow this convention in light of the required changes above.

@tiero
Copy link
Member

tiero commented Nov 26, 2021

Also, psetv2/blinder.go could follow this convention in light of the required changes above.

Sorry to stress on this point, let's strive to have a separated package blinder to be detached from the psetv2 so we can compile it to WASM & gomobile

@altafan
Copy link
Collaborator

altafan commented Nov 26, 2021

Sorry to stress on this point, let's strive to have a separated package blinder to be detached from the psetv2 so we can compile it to WASM & gomobile

For this, it could be enough for the methods that we will define in psetv2/blinder.go to expect not only the blinding data to add but also the sanity checks that validate those data.

Every check related to the PSET that does not involve secp-zkp could be done by the "base" blinder, like checking that the current output can be blinded, or that the PSET is not locked for modifications etc.

This way, psetv2 would not directly depend on secp-zkp and could be compiled to WASM & gomobile.

@tiero
Copy link
Member

tiero commented Nov 26, 2021

psetv2/blinder.go

I mean we must move in a different folder & different package CGO stuff, otherwise go build will ignore the whole package if just one file has one dependency with CGO

You can just try it and see if ignores the whole package or no
GOOS=js GOARCH=wasm go build ./psetv2

@sekulicd sekulicd changed the title Psetv2 signer Psetv2 signer, finaliser and extractor Nov 29, 2021
psetv2/pset.go Outdated
@@ -6,6 +6,8 @@ import (
"encoding/hex"
"errors"

"github.com/vulpemventures/go-elements/confidential"
Copy link
Member

Choose a reason for hiding this comment

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

this will prevent to compile in non-CGO environment.

Let's move this in a blinderultil pkg maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I didnt reply so far.
Should we maybe, in this moment, try create some transactions like in pset_test and see where we are with this package?
Later we could try to decouple from confidential pkg, but not sure how we do it unless we just pass all blinding stuff for each output.
I attempted to follow spec as much as possible so that pset pkg has all roles like specified.
@tiero @altafan what do you think? how to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

making it as pure function so we pass the *Pset as argument

Copy link
Member

Choose a reason for hiding this comment

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

I understand for the blinder.go file which is handy now to keep single pkg at this stage, but this validation method seems pretty easy to have it outside of pset pkg. The spec does not dictate the API/library architecture itself (I understand this would be theoretically more go idiomatic)

@sekulicd
Copy link
Collaborator Author

sekulicd commented Dec 1, 2021

@tiero @altafan please check if u are ok with the way how i decoupled psetv2 from confidential pkg.
Here is just POC with the separate pkg for testing so that psetv2 stays decoupled.

@tiero
Copy link
Member

tiero commented Dec 1, 2021

@tiero @altafan please check if u are ok with the way how i decoupled psetv2 from confidential pkg. Here is just POC with the separate pkg for testing so that psetv2 stays decoupled.

If I understand, the decoupling is done using the Blinder interface.

We still missing the blindProofsValid to be moved out, but looks good solution for now. It should be possible now to compile to WASM and create a wrapper on top to provider necessary bytes data from the JS runtime.

@@ -495,7 +502,7 @@ func (b *BlinderRole) blindOutputs(
}

assetCommitment, err := b.blinderSvc.AssetCommitment(
output.outputAsset,
output.outputAsset[1:],
Copy link
Member

Choose a reason for hiding this comment

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

would leave a comment here or wrap in a descriptive variable (ie. conf/unconf prefix)

@@ -528,11 +538,11 @@ func (b *BlinderRole) blindOutputs(
rangeProof, err := b.blinderSvc.RangeProof(
uint64(*output.outputAmount),
nonce,
output.outputAsset,
output.outputAsset[1:],
Copy link
Member

Choose a reason for hiding this comment

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

here also

confidential/confidential.go Outdated Show resolved Hide resolved
@tiero tiero requested a review from altafan December 16, 2021 10:09
Copy link
Collaborator

@altafan altafan left a comment

Choose a reason for hiding this comment

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

@sekulicd this is missing unit tests for HashForWitnessV0 and HasHForSignature for the newly SIGHASH_RANGEPROOF.

Given the effort so far, I think it's easier to just add another test in psetv2-test using only P2PKH inputs to not leave HashForSignature completely untested. The one already in place uses P2WPKH ins (w/ SIGHASH_RANGEPROOF) so we already tested that HashForWitnessV0 is correct indirectly.

We can add fixtures to transaction/data/tx_valid.json later with another PR.

@altafan
Copy link
Collaborator

altafan commented Dec 27, 2021

@sekulicd solve conflicts

@altafan altafan mentioned this pull request Feb 11, 2022
@tiero tiero closed this Mar 2, 2022
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.

None yet

3 participants