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

Randomize JoinSplit input and output orders #1561

Merged
merged 6 commits into from Oct 20, 2016

Conversation

@str4d
Copy link
Contributor

@str4d str4d commented Oct 18, 2016

Closes #778.

str4d added 3 commits Oct 18, 2016
boost::array<size_t, ZC_NUM_JS_OUTPUTS>& outputMap,
CAmount vpub_old,
CAmount vpub_new,
bool computeProof)

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

This could take the random generator as a parameter to make it more easily/thoroughly testable. Does not block.

0, 0);
BOOST_CHECK(jsdesc.Verify(*p, pubKeyHash));

std::set<size_t> inputSet;

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

You can use inputSet(inputMap.begin(), inputMap.end()) here instead of the loop, I think. Does not block.

if (outputMap[i] == n) {
return i;
}
}

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

No find method? Apparently not. Sigh.

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

Not for json_spirit::Array, because it doesn't have begin() or end().


if (jsChange > 0) {
changeOutputIndex = find_output(obj, 1);
}
}
}

This comment has been minimized.

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

I can't follow that link to anywhere useful.

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

Line 504 (after the commit).

@@ -845,11 +867,20 @@ Object AsyncRPCOperation_sendmany::perform_joinsplit(
);

// Generate the proof, this can take over a minute.
JSDescription jsdesc(*pzcashParams,
boost::array<libzcash::JSInput, ZC_NUM_JS_INPUTS> inputs

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

This can be inputs(info.vjsin); I think.

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

Not with boost::array AFAICT.

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

Huh, I thought I checked the docs.

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

// construct/copy/destruct
template<typename U> array& operator=(const array<U, N>&);

http://www.boost.org/doc/libs/1_62_0/doc/html/boost/array.html

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

That's from another array; info.vjsin is a std::vector<JSInput>.

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

OK, never mind then.

@@ -910,10 +941,21 @@ Object AsyncRPCOperation_sendmany::perform_joinsplit(
encryptedNote2 = HexStr(ss2.begin(), ss2.end());
}

Array arrInputMap;

This comment has been minimized.

@daira

daira Oct 18, 2016
Contributor

This can be arrInputMap(inputMap); instead of the loop, I think.

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

Not with json_spirit::Array AFAICT.

This comment has been minimized.

@nathan-at-least

nathan-at-least Oct 19, 2016
Contributor

[Comment] Maybe some day we should add the standard iteration API to json_spirit::Array? I personally prefer shorter conventional idioms instead of repeated larger code blocks.

@daira
Copy link
Contributor

@daira daira commented Oct 18, 2016

ut(ACK+cov) modulo the stale comment at 2eeb6be#diff-63ec75bfb71c2d4e26f2a95d5168f5e5R504 . Addressing other comments is optional. Note that I wrote the first two commits with @str4d, so another ACK is needed.

str4d added 2 commits Oct 18, 2016
@str4d
Copy link
Contributor Author

@str4d str4d commented Oct 18, 2016

@daira comments addressed.

@daira
Copy link
Contributor

@daira daira commented Oct 18, 2016

ut(ACK+cov). Still needs another ACK.

@ebfull
Copy link
Contributor

@ebfull ebfull commented Oct 18, 2016

This looks good to me, cannot review it extensively enough to ACK though.

@@ -25,6 +26,29 @@ int GetRandInt(int nMax);
uint256 GetRandHash();

/**
* Rearranges the elements in the range [first,first+len) randomly, assuming

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

) should be ] ?

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

No, the end of the range is exclusive.

This comment has been minimized.

@bitcartel

bitcartel Oct 19, 2016
Contributor

Hmm, ok, maybe it's a C++ thing to use interval notation in documentation. I can't recall seeing this style for other languages.

* that gen is a uniform random number generator. Follows the same algorithm as
* std::shuffle in C++11 (a Durstenfeld shuffle).
*
* The elements in the range [mapFirst,mapFirst+len) are rearranged according to

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

) should be ] ?

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

No, the end of the range is exclusive.

* The elements in the range [mapFirst,mapFirst+len) are rearranged according to
* the same permutation, enabling the permutation to be tracked by the caller.
*
* gen takes an integer n and produces a uniform random output in [0,n).

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

) should be ] ?

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

No, the end of the range is exclusive.

std::function<int(int)> gen)
{
for (size_t i = len-1; i > 0; --i) {
auto r = gen(i+1);

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

function gen could return a value which is out of bounds for the iterator.

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

The definition of gen is that it returns a value in [0,n); I will add assertions that this is the case.

@@ -429,6 +446,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {
}

obj = perform_joinsplit(info, outPoints);

if (jsChange > 0) {
changeOutputIndex = find_output(obj, 1);

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

This should never return -1 right? Since we always have two output notes?

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

It should never return -1 because outputmap is always length 2 and contains 1. If we want to have a stronger assertion, I can add it.

This comment has been minimized.

@nathan-at-least

nathan-at-least Oct 19, 2016
Contributor

[Comment] I would like an assertion here so that with minimal effort a new reader doesn't have to know those "remote invariants", eg: always length 2. (Does not block.)

This comment has been minimized.

@daira

daira Oct 19, 2016
Contributor

We could raise std::logic_error instead of returning -1; that would avoid the risk of an out-of-bounds access if the -1 were used.

@@ -645,6 +662,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {
}

obj = perform_joinsplit(info, witnesses, jsAnchor);

if (jsChange > 0) {
changeOutputIndex = find_output(obj, 1);

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

Ditto above, this should never be -1 right?

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

Yep.

template <typename RandomAccessIterator, typename MapRandomAccessIterator>
void MappedShuffle(RandomAccessIterator first,
MapRandomAccessIterator mapFirst,
size_t len,

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

The len parameter can be removed. Both iterators should span the same distance, so you can compute the len value in the body of the function with something like size_t len = std::distance(first.begin(), first.end()). Could use an assert to ensure both iterators are of the same len,

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

Or just last-first, which is what the equivalence code in std::shuffle uses. But that means we have to pass in both begin() and end() for both collections (which is why @daira and I opted at the time for len). If you prefer that style, I can change it.

This comment has been minimized.

@daira

daira Oct 19, 2016
Contributor

I think this is clearer as it is.

This comment has been minimized.

@bitcartel

bitcartel Oct 19, 2016
Contributor

I don't mind.

// Randomize the order of the inputs and outputs
inputMap = {0, 1};
outputMap = {0, 1};
MappedShuffle(inputs.begin(), inputMap.begin(), ZC_NUM_JS_INPUTS, gen);

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

Assert that inputs and inputMap are the same length? And see comment below about not needing the ZC_NUM_JS_INPUTS parameter.

This comment has been minimized.

@daira

daira Oct 19, 2016
Contributor

You can already locally see that they must be the same length, from the declarations, so I don't think that an assertion adds anything.

@@ -13,7 +13,7 @@

using ::testing::Return;

ZCJoinSplit* params = ZCJoinSplit::Unopened();
extern ZCJoinSplit* params;

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2016
Contributor

Why is this changing? Thanks.

This comment has been minimized.

@str4d

str4d Oct 18, 2016
Author Contributor

Because otherwise g++ complains that params is defined twice.

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Oct 18, 2016

First review pass complete and ran tests. Will review a second time after feedback on comments.

Copy link
Contributor

@nathan-at-least nathan-at-least left a comment

ACK code review.

Not-yet-ACK'd test coverage.

@@ -25,6 +26,29 @@ int GetRandInt(int nMax);
uint256 GetRandHash();

/**
* Rearranges the elements in the range [first,first+len) randomly, assuming
* that gen is a uniform random number generator. Follows the same algorithm as
* std::shuffle in C++11 (a Durstenfeld shuffle).

This comment has been minimized.

@nathan-at-least

nathan-at-least Oct 19, 2016
Contributor

[comment] I'm curious why we don't use std::shuffle if we're reimplementing the same algorithm. If we std::shuffle a list of pairs of (i, x) where i is the original index, could we achieve the same result while reusing std? (I can't tell if that'd be nicer or worse for ergonomics, yet.)

This comment has been minimized.

@daira

daira Oct 19, 2016
Contributor

@bitcartel originally used that approach, but the specification of std::shuffle and URNG are not strong enough to guarantee determinism (needed for first and firstMap to be permuted in the same way), use of a secure RNG, and a uniform distribution over permutations.

This comment has been minimized.

@bitcartel

bitcartel Oct 19, 2016
Contributor

I think you mean @str4d ?

auto r = gen(i+1);
std::swap(first[i], first[r]);
std::swap(mapFirst[i], mapFirst[r]);
}

This comment has been minimized.

@nathan-at-least

nathan-at-least Oct 19, 2016
Contributor

[comment] I'm surprised, at first glance, this make each permutation equally likely. I'll have to sit down with that sometime for fun.

@@ -429,6 +446,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {
}

obj = perform_joinsplit(info, outPoints);

if (jsChange > 0) {
changeOutputIndex = find_output(obj, 1);

This comment has been minimized.

@nathan-at-least

nathan-at-least Oct 19, 2016
Contributor

[Comment] I would like an assertion here so that with minimal effort a new reader doesn't have to know those "remote invariants", eg: always length 2. (Does not block.)

@@ -910,10 +941,21 @@ Object AsyncRPCOperation_sendmany::perform_joinsplit(
encryptedNote2 = HexStr(ss2.begin(), ss2.end());
}

Array arrInputMap;

This comment has been minimized.

@nathan-at-least

nathan-at-least Oct 19, 2016
Contributor

[Comment] Maybe some day we should add the standard iteration API to json_spirit::Array? I personally prefer shorter conventional idioms instead of repeated larger code blocks.

@str4d
Copy link
Contributor Author

@str4d str4d commented Oct 19, 2016

Pending concerns addressed.

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Oct 20, 2016

ACK (have not checked code coverage)

@daira
Copy link
Contributor

@daira daira commented Oct 20, 2016

ACK aa36398. I'm happy with coverage, so @zkbot r+

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 20, 2016

📌 Commit aa36398 has been approved by daira

zkbot pushed a commit that referenced this pull request Oct 20, 2016
Randomize JoinSplit input and output orders

Closes #778.
@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 20, 2016

Testing commit aa36398 with merge 0dfe612...

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 20, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit aa36398 into zcash:master Oct 20, 2016
1 check passed
1 check passed
homu Test successful
Details
@Zenitur
Copy link

@Zenitur Zenitur commented Oct 29, 2016

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Oct 29, 2016

@Zenitur What version of g++ are you using? What's your OS environment?

@Zenitur
Copy link

@Zenitur Zenitur commented Oct 29, 2016

@bitcartel, Debian Linux 7.0 Wheezy, x86_64 arch

I have been copied binaries compiled with GCC 4.9.2 from another PC. Now I just tell you about an error in code

@Zenitur
Copy link

@Zenitur Zenitur commented Oct 30, 2016

After applying #838 (comment) there is no compiling errors anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.