vulpemventures / go-elements

Go support for Liquid/Elements transactions
MIT License
27 stars 12 forks source link

Add support for psetv2 #189

Closed altafan closed 1 year ago

altafan commented 2 years ago

This contribs to the initial effort of @sekulicd in #184 and takes the psetv2 package to a ready-to-use version.

I made a huge refactor regarding named variables, errors messages, and, more importantly, the business logic.

In particular, the blinder role has been basically split into 2 fundamental parts:

The package already contains some tests showing how to use psetv2 to:

Please @tiero @sekulicd @louisinger review this.

This closes #167 This closes #189

altafan commented 2 years ago

Minor: I added some (tested) util methods to the elementsutils package and renamed the existing ones. They all come from projects using this lib. There, we noticed both the usefulness and the lack of such methods, so this seemed a gold opportunity to add them all. Shouldn't be that problematic fixing their usage in other projects once updating go-elements to the next version.

tiero commented 2 years ago

I would prefer to rename blinder.Blind into blinder.AddBlinders or eventually split it up into two methods like blinder.AddIssuanceBlinders and blinder.AddOutputBlinders to keep the methods shorters if possible.

Also I am more fan of two separated method AddOutputBlindersNotLast and AddOutputBlindersLast instead of an implicit boolean, what do you think?

Understood splitting Issuance and Output blinders can be problematic with state, but would split last and nonLast methods?

altafan commented 2 years ago

Understood splitting Issuance and Output blinders can be problematic with state, but would split last and nonLast methods?

Yeah, at the beginning my idea was to have 2 different methods for outputs and issuance for the blinder role, but then it became evident that an internal state would have been required for the calculation of the global scalar. For this reason I opted for a unique Blind() method.

I'm gonna split the method into BlindLast() and BlindNonLast() and get rid of Blind() + lastBlinder.

tiero commented 2 years ago

I find API this way defined, harder to use and it requires more PSET knowledge from pkg user. Wouldnt be nicer that we ask for required inputs, needed for blinding and signing

On this specific matter, I do think it's best to be less abstracted instead, to allow more exotic/complex cross-compilation scenarios (WASM etc..) or cooperative blinding use-cases

I see go-elements as a collection of utils/pkgs to manipulate elements tx/blocks, we should leave be maybe more abstracted on upper layers, more "high level" let's say "Golang Development Kit"

some examples:

etc...

altafan commented 2 years ago

I suggest that in future we do changes over existing PRs, since this way we lose commit and contributor history and its harder to review.

My branch adds commits to yours (the one for which you opened #184) in order to not lose any of your contributions indeed. And I also think this was the best way to go because otherwise, in order to add commits to your branch, I would have needed write rights on your repo. This is not something you should want to grant to other users over your repos, in general.

I dont understnad why it is important to extract some blind/sign logic from pset pkg itself.

Extracting the singing logic from the pset is essential to allow every wallet supporting psetv2 to use this library. Otherwise, following your idea, only wallets allowing to dump private keys and use them manually could use our psetv2 pkg. For sure this is not ideal for the usability of the library.

As you showed with your example, I think it's totally fine that one produces the signatures on his own and then uses our signer to add them to the partial transaction, with the role ensuring that everything's correct when doing such operation.

For blinding, we actually shouldn't need such "extraction". We need it though to make psetv2 not dependent from secp256k1-zkp, which is a CGO porting and cause troubles if we want to compile psetv2 to WASM.

So I decided to isolate blinding key management into the handler, which is dependent from secp256k1-zkp and responsible for generating commitments, proof etc and also to unblind blinded data, just like a wallet handle keys to produce signatures to make a comparison.

The other part is the pset's blinder role which ensures that everything's correct when adding commitments, proofs etc to input issuances or outputs. Furthermore, it takes care of keeping the global scalars up-to-date, which is something very pset related.

sekulicd commented 2 years ago

My branch adds commits to yours (the one for which you opened https://github.com/vulpemventures/go-elements/pull/184) in order to not lose any of your contributions indeed.

Hmm in my IDE i was not able to see history of commits, but now when i check GH history i see them, sorry.

Extracting the singing logic from the pset is essential to allow every wallet supporting psetv2 to use this library. Otherwise, following your idea, only wallets allowing to dump private keys and use them manually could use our psetv2 pkg. For sure this is not ideal for the usability of the library.

Sorry i dont understand this fully, if wallet wants to sign tx it needs to access to priv key? Can u elaborate more on this and maybe if you have example of a wallet which fits your scenario?

For blinding, we actually shouldn't need such "extraction". We need it though to make psetv2 not dependent from secp256k1-zkp, which is a CGO porting and cause troubles if we want to compile psetv2 to WASM.

secp256k1-zkp is referenced, in pset pkg, through confidential pkg and that is the reason I introduced this interface for "confidential" methods, so basically moving out blinder logic only to be able to compile WASM (@tiero) seems not a valid reson.

One last thing, I think we need to move pset_test.go in other pkg if we want to decouple pset from confidential.

Regarding decoupling from secp256k1-zkp, did u checked this?

altafan commented 2 years ago

One last thing, I think we need to move pset_test.go in other pkg if we want to decouple pset from confidential. Regarding decoupling from secp256k1-zkp, did u checked this?

The package declared in pset_test.go is psetv2_test, which is not psetv2. This lets us import psetv2 as external pkg to have the exact "pov" of the user. Importing psetv2 in other projects won't import psetv2_test too, therefore it's alright.

Sorry i dont understand this fully, if wallet wants to sign tx it needs to access to priv key? Can u elaborate more on this and maybe if you have example of a wallet which fits your scenario?

In the previous comment, after the blind/sign snippet, you said:

Remember that we are going to replicate above code in all of our projects and it is not part of any pkg but only test.

This is true only for now, just because every wallet we implemented so far are single key P2WPKH wallets, meaning that they sign the inputs in exactly the same way, the one you posted in the snippet. If we created a sign method like Sign(privateKeys [][]byte, inIndexes[]uint32) implementing your snippet:

  1. Only single-key P2WPKH wallets could use it
  2. Such wallets must have an API to retrieve the list of private keys to provide to the sing method.

And guess what? The majority of single-key wallets outthere are neither native-segwit, nor they let the user directly manipulate private keys such that this hypothetical snippet is possible:

privateKeys := walletSvc.GetPrivateKeys()
signer := psetv2.NewSigner(ptx)
signedPtx :=  signer.Sign(privateKeys, inIndexes)

secp256k1-zkp is referenced, in pset pkg, through confidential pkg and that is the reason I introduced this interface for "confidential" methods, so basically moving out blinder logic only to be able to compile WASM (@tiero) seems not a valid reason.

I think your initial idea is good indeed, I just tried to improve it in order for the blinder role to look similar to the signer one, ie. a role adding sensitive data to the pset by enforcing that all the conditions stated in the spec are respected.

tiero commented 2 years ago

https://github.com/btcsuite/btcd/pull/1847