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

Pset add blinder role #79

Merged
merged 22 commits into from
Jun 8, 2020
Merged

Conversation

sekulicd
Copy link
Collaborator

@sekulicd sekulicd commented Jun 4, 2020

Before there was no option to calculate blind pset outputs.

After the method BlindOutputs is added, it is possible to blind outputs of PSET.

It closes #45 .

As per comment here this will close #74

Please @altafan can you review this?

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.

The structure of the package is so clean! Well done @sekulicd.
Most of the todos are ok.. there are some changes required to fix "bugs", but in general it looks really good to me.

It would be nice if the blinder could expect an argument with a rng function so that it uses the generateRandomNumber only as a fallback.

pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
@sekulicd
Copy link
Collaborator Author

sekulicd commented Jun 4, 2020

It would be nice if the blinder could expect an argument with a rng function so that it uses the generateRandomNumber only as a fallback.

Can you elaborate more on this?

@altafan
Copy link
Collaborator

altafan commented Jun 4, 2020

Can you elaborate more on this?

The NewBlinder factory function could expect one more argument of type func that just like our generateRandomNumber would return a byte slice or error so that the user can "customize " the random number generation so it's not forced to rely only on crypto/rand package.

Of course, the blinder type would need an additional RNG or whatsoever field that the factory sets to the custom function, if passed, or to the generateRandomNumber as fallback.

pset/blinder.go Outdated Show resolved Hide resolved
pset/blinder.go Outdated Show resolved Hide resolved
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.

This should provide an integration test to test that the blinder role works as expected and that the transaction can be broadcasted.
This is an integration test taken from liquid js that shows how to create, blind, sign and broadcast a confidential transaction that used native segwit confidential addresses.
This is the integration test for a non-confidential transaction using our pset package.

NOTE:
Since our library actually supports only segwit confidential addresses we should use p2wpkh blech32 addreseses in the test.

@vulpemventures vulpemventures deleted a comment from sekulicd Jun 4, 2020
@sekulicd
Copy link
Collaborator Author

sekulicd commented Jun 5, 2020

I am trying to blind same PSET tx as in this test case, so there is one input and 3 outputs.
My test is failing in this line cause vbl=4 != gbl=3.
vbl = input + output values = 1 input + 3 output
gbl = generatorBlindings = inputVbfs + outpuVbfs = 1 input + 2output

I was debugging mentioned JS test case and I see that method blindGeneratorBlindSum receives input with same count of vbl and gbl and it TC is passing
Screenshot 2020-06-05 at 14 36 37

@altafan do you think that it can be issue with validation in line ?

@altafan
Copy link
Collaborator

altafan commented Jun 5, 2020

My guess is that you are including the value of the fee output within values while it should not.

pset/blinder.go Outdated Show resolved Hide resolved
@sekulicd
Copy link
Collaborator Author

sekulicd commented Jun 5, 2020

@altafan, test case is passing now and I can see confirmed blinded transaction in the explorer.
can you double-check if anything should be updated?

@tiero tiero marked this pull request as ready for review June 5, 2020 15:09
Copy link
Member

@tiero tiero left a comment

Choose a reason for hiding this comment

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

I pushed a change to updated the readme roadmap, just to let you know in case you want to sync. @sekulicd

LGTM

@altafan
Copy link
Collaborator

altafan commented Jun 5, 2020

Awesome! @sekulicd

LGTM, but I would add another test cases that uses confidential input instead of non confidential ones. Basically we need to do the same test, but instead of funding a bech32 address with the faucet, we fund a blech32 one.

This means that at the very beginning of the test we create a random private blinding key that we will use to then unblind the faucet output that we'll use as input of our test transaction. The public key derived, instead, is used to create the confidential blech32 address.

EDIT:
Our library actually does not support any kind of confidential address because we do not have any function in the payment package that helps to blind an unconfidential address with some blinding key, so the user would need to do this by himself. For now, we are fine with the test we already have in place.

@sekulicd
Copy link
Collaborator Author

sekulicd commented Jun 5, 2020

Awesome! @sekulicd

LGTM, but I would add another test cases that uses confidential input instead of non confidential ones. Basically we need to do the same test, but instead of funding a bech32 address with the faucet, we fund a blech32 one.

This means that at the very beginning of the test we create a random private blinding key that we will use to then unblind the faucet output that we'll use as input of our test transaction. The public key derived, instead, is used to create the confidential blech32 address.

Considering that this is the way to create blech32 address:

        privkey, err := btcec.NewPrivateKey(btcec.S256())
	if err != nil {
		t.Fatal(err)
	}
	pubkey := privkey.PubKey()

	publicKeyHashCompressed := btcutil.Hash160(pubkey.SerializeCompressed())

	program := append(pubkey.SerializeCompressed(), publicKeyHashCompressed...)

	blech32Addr := &address.Blech32{
		Prefix:  "el",
		Version: 0,
		Data:    program,
	}

How should I create scriptpubkey and signature, as we dont have option to create blech32 payment which would hold these two values like in case of p2wpkh for eg.:

changeScript := p2wpkh.Script
legacyScript := append(append([]byte{0x76, 0xa9, 0x14}, p2wpkh.Hash...), []byte{0x88, 0xac}...)

@sekulicd
Copy link
Collaborator Author

sekulicd commented Jun 8, 2020

Awesome! @sekulicd
LGTM, but I would add another test cases that uses confidential input instead of non confidential ones. Basically we need to do the same test, but instead of funding a bech32 address with the faucet, we fund a blech32 one.
This means that at the very beginning of the test we create a random private blinding key that we will use to then unblind the faucet output that we'll use as input of our test transaction. The public key derived, instead, is used to create the confidential blech32 address.

Considering that this is the way to create blech32 address:

        privkey, err := btcec.NewPrivateKey(btcec.S256())
	if err != nil {
		t.Fatal(err)
	}
	pubkey := privkey.PubKey()

	publicKeyHashCompressed := btcutil.Hash160(pubkey.SerializeCompressed())

	program := append(pubkey.SerializeCompressed(), publicKeyHashCompressed...)

	blech32Addr := &address.Blech32{
		Prefix:  "el",
		Version: 0,
		Data:    program,
	}

How should I create scriptpubkey and signature, as we dont have option to create blech32 payment which would hold these two values like in case of p2wpkh for eg.:

changeScript := p2wpkh.Script
legacyScript := append(append([]byte{0x76, 0xa9, 0x14}, p2wpkh.Hash...), []byte{0x88, 0xac}...)

UPDATE:
@altafan, for the changeScript and legacyScript i took bellow values:

changeScript := append([]byte{0x00, 0x14}, publicKeyHashCompressed...)
legacyScript := append(append([]byte{0x76, 0xa9, 0x14}, publicKeyHashCompressed...), []byte{0x88, 0xac}...)

Please advise if this is correct.

Assuming that this is correct I am trying to add WitnessUtxo:
updater.AddInWitnessUtxo
but considering that Utxo is now confidential, as we funded blech32 address, I need to add Surejction and Range proof.

Since Utxo doesnt contain Surjection and Range proof, idea is to fetch full transaction.
In order to do this I took txId from utxo and invoked:
/tx/{txId}/hex

I receive full transaction in raw bytes which I am able to decode using elements-cli, now the problem is that the method, I assumed, should be used to decode raw tx:
transaxction.NewTxFromBuffer
is throwing error and I am not able to fetch previous transaction from which I am planning to take Surjection and Range proof that are needed when unblinding inputs.
Please advise if I missed something.

@altafan
Copy link
Collaborator

altafan commented Jun 8, 2020

I was not intending to modify the running integration test by using a confidential input, but rather to create a new one.

Said that, the confidential address format, blech32 this case, is just a way so that I, receiver of a transaction, can pass my blinding pubkey to the sender in order to him being able to blind my outputs with a key that I'm aware of.

For a confidential output, thus, the output script does not change at all compared to the unconfidential one, so you should not be worried it.

The WitnessUtxo field of a partial input is of type transaction.TxOutput that already contains the field for range and surjection proofs.
If you fetch a utxo that does not have proofs it may mean that it's not a confidential utxo at all?

In general the NonWitnessUtxo is used when managing utxos locked by p2pkh script, just to let you know.

@altafan altafan merged commit c0d3b45 into vulpemventures:master Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants