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 code to test partial blinding functionality to qa/rpc-tests #550

Merged

Conversation

Projects
None yet
3 participants
@instagibbs
Copy link
Collaborator

commented Mar 29, 2019

forward port of #510

@dgpv

@instagibbs

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2019

only real difference is that the asset commitment list size must match the raw transaction input size

@instagibbs instagibbs force-pushed the instagibbs:partial_blinding_test_port branch from 706b96a to 23f0f43 Mar 29, 2019

@dgpv

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

only real difference is that the asset commitment list size must match the raw transaction input size

That changes the coverage of the tested code, as assetcommitments are also used when inputs are not supplied

This code:

// Process any additional targets from auxiliary_generators
will not be covered by this test

@stevenroose
Copy link
Member

left a comment

Looks look, just left some nits.

# on top of inputs and outputs of the blinded, but incomplete transaction.
# We also append empty witness instances to make witness arrays match
# vin/vout arrays
btx.wit.vtxinwit.append(CTxInWitness())

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 1, 2019

Member

I don't see inputs being added, just input witnesses... It already had 3 inputs, so now it looks like it has 5 input witnesses and the extra 2 added here will be ignored.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Apr 1, 2019

Author Collaborator

outdated comment, ill fix

This comment has been minimized.

Copy link
@dgpv

dgpv Apr 6, 2019

Contributor

That two surplus btx.wit.vtxinwit.append(CTxInWitness()) still made it into master, and the comment vin/vout, too - it should be just 'vout'

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 9, 2019

Member

@instagibbs I guess I took your word for it. Should I PR a nitfix?

# Input 0 is bitcoin asset (already blinded)
# Input 1 is also bitcoin asset
# Input 2 is our new asset
# Input 3 is fee that we added above (also bitcoin asset)

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 1, 2019

Member

^ s/Input/Output/ ?

This comment has been minimized.

Copy link
@dgpv

dgpv Apr 1, 2019

Contributor

Yes, this was my mistake in the original PR. It should read Output

@stevenroose

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

I don't think I did a review on the original PR on 0.14.1.. So this might be an exact port that I'm not commenting on. But worth the small nits anyway I think.

@instagibbs

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 1, 2019

That changes the coverage of the tested code, as assetcommitments are also used when inputs are not supplied

No, this branch disallows different sized inputs(behavior change during rebase)

@dgpv

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

branch disallows different sized inputs

The only place where auxiliary_generators is specified when calling BlindTransaction is from blindrawtransaction. If blindrawtransaction disallows len(inputs) != len(auxiliary_generators), then the code at src/blind.cpp:328 is not used from anywhere, and can be considered dead code.

@instagibbs

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 2, 2019

It's still a more sane internal API and may be used in the future.

@dgpv

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

It's still a more sane internal API and may be used in the future.

To my taste, especially because this code was already related to a bug (#514), it is future-bug-prone to leave it dangling and not covered by tests. My humble opinion: I feel that it is better to remove this code and and enforce len(vin) == len(auxiliary_generators) at the start of BlindTransaction, and if there is a need for this functionality in the future, reintroduce the code.

@instagibbs instagibbs force-pushed the instagibbs:partial_blinding_test_port branch from 23f0f43 to 4258f9f Apr 2, 2019

@instagibbs

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 2, 2019

Well, the response to that is to add more tests not rip out code imo

Rebased, though Travis is being naughty today.

@stevenroose

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

ACK 4258f9f

@stevenroose stevenroose merged commit 4258f9f into ElementsProject:master Apr 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stevenroose added a commit that referenced this pull request Apr 3, 2019

Merge #550: add code to test partial blinding functionality to qa/rpc…
…-tests

4258f9f add code to test partial blinding functionality to qa/rpc-tests (Dmitry Petukhov)

Pull request description:

  forward port of #510

  @dgpv

Tree-SHA512: 3c6b8afb63234d98f9c917ad17f08f0d426577891178cecff0cfca5787f5285607e64811119a328cbe09eacae1693201deb458ea8db0c97600dfcffd1cbf90be
@instagibbs

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.