vulpemventures / go-elements

Go support for Liquid/Elements transactions
MIT License
29 stars 13 forks source link

Blind outputs with blinders #152

Closed louisinger closed 3 years ago

louisinger commented 3 years ago

This PR lets to use inputs blinders to blind a transaction. Follow the same principle of liquidjs' PR

For an unknown reason, these changes make the test results random. Sometimes all the blinded transactions are valid, but often, we get the bad-txns-in-ne-out, value in != value out (code 16) (happen on random txs). I can't find where the problem appears, any idea to help me with this is welcome.

it closes #149

EDIT: Include a new factory NewBlinderByIndex using a map to select the outputs to blind. it closes #147.

@tiero @altafan please review

tiero commented 3 years ago

For an unknown reason, these changes make the test results random. Sometimes all the blinded transactions are valid, but often, we get the bad-txns-in-ne-out, value in != value out (code 16) (happen on random txs). I can't find where the problem appears, any idea to help me with this is welcome.

It is using the latest go-secp256 tagged release? Ref: https://github.com/vulpemventures/go-secp256k1-zkp/pull/8

louisinger commented 3 years ago

For an unknown reason, these changes make the test results random. Sometimes all the blinded transactions are valid, but often, we get the bad-txns-in-ne-out, value in != value out (code 16) (happen on random txs). I can't find where the problem appears, any idea to help me with this is welcome.

It is using the latest go-secp256 tagged release? Ref: vulpemventures/go-secp256k1-zkp#8

Yes v1.1.0

tiero commented 3 years ago

Sorry I meant Travis.yml since we have It :joy: It's ok to haave both at this point since you spent time adding it, but would you update .travis.yml as well?

altafan commented 3 years ago

For an unknown reason, these changes make the test results random. Sometimes all the blinded transactions are valid, but often, we get the bad-txns-in-ne-out, value in != value out (code 16) (happen on random txs). I can't find where the problem appears, any idea to help me with this is welcome.

Good news! I solved this.. It was indeed a random problem!! 😱 Looping over a map (b.blindingPubKeyByOutputIndex) does not grant to maintain the order of the outputs while blinding which is something crucial for us. You can cherry-pick my commit.

altafan commented 3 years ago

LGTM but NewBlinder AFAIK will break tdex-daemon?

This is actually right because we changed one of its args.. If we have no choice but to make a breaking change, then I would drop NewBlinderByIndex and modify also its blindingPubkeys [][]byte arg into a outputsPubKeyByIndex map[int][]byte

tiero commented 3 years ago

LGTM but NewBlinder AFAIK will break tdex-daemon?

This is actually right because we changed one of its args.. If we have no choice but to make a breaking change, then I would drop NewBlinderByIndex and modify also its blindingPubkeys [][]byte arg into a outputsPubKeyByIndex map[int][]byte

We are not a point yet where we need to be backward compatible. But the idea would have been to have a NewBlinderSomething doing the new stuff.

ACK for breaking compatibility, at this point I agree with @altafan to drop NewBlinderByIndex

@louisinger

louisinger commented 3 years ago

Update: NewBlinderByIndex renamed into NewBlinder @tiero