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

Fixes #1779 so that sending to multiple zaddrs no longer fails. #1790

Merged
merged 2 commits into from Nov 6, 2016

Conversation

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Nov 5, 2016

Closes #1779

Commit 2eeb6b randomized the order of input and output notes,
but this is now known to prevent the chaining of multiple joinsplits
in a single transaction. The root cause has yet to be determined.

This patch is a temporary fix and disables the shuffling of input
and output notes. It also adds a chained joinsplit test to the
python qa test suite.

Commit 2eeb6b randomized the order of input and output notes,
but this is now known to prevent the chaining of multiple joinsplits
in a single transaction.  The root cause has yet to be determined.

This patch is a temporary fix and disables the shuffling of input
and output notes.  It also adds a chained joinsplit test to the
python qa test suite.
@ebfull
Copy link
Contributor

@ebfull ebfull commented Nov 5, 2016

What was passed in for gen to Randomized before? Is there some default value in the header file?

Anyway, ACK.

@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 5, 2016

Default in transaction.h is GetRandInt.

Copy link
Contributor

@str4d str4d left a comment

ACK after comments addressed.

@@ -57,8 +57,10 @@ JSDescription JSDescription::Randomized(
// Randomize the order of the inputs and outputs
inputMap = {0, 1};
outputMap = {0, 1};
MappedShuffle(inputs.begin(), inputMap.begin(), ZC_NUM_JS_INPUTS, gen);
MappedShuffle(outputs.begin(), outputMap.begin(), ZC_NUM_JS_OUTPUTS, gen);
if (gen) {

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

I would prefer to pass in a no-op function rather than an empty one, ie. one that always returns N_MAX-1 (which causes a shuffle that results in itself). See GenMax() in src/gtest/utils.cpp and its effect in src/gtest/test_random.cpp.

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

(The reason being that with this conditional section, it is no longer obvious to the caller that if they accidentally pass in an empty function, JSDescription::Randomized no longer does what it says on the box. With an explicit identity function, it is obvious.)

This comment has been minimized.

@bitcartel

bitcartel Nov 5, 2016
Author Contributor

That's what I did originally, but it was more confusing. See below.

This comment has been minimized.

@bitcartel

bitcartel Nov 5, 2016
Author Contributor

Also, without this conditional section, if a user passes in an empty func, MappedShuffle is still invoked and the behaviour is undefined and an exception is thrown.

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

I consider this correct behaviour. However, we could check if it is empty and throw an explicit error.

@@ -875,6 +875,7 @@ Object AsyncRPCOperation_sendmany::perform_joinsplit(
{info.vjsout[0], info.vjsout[1]};
boost::array<size_t, ZC_NUM_JS_INPUTS> inputMap;
boost::array<size_t, ZC_NUM_JS_OUTPUTS> outputMap;
std::function<int(int)> emptyFunc;

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

std::function<int(int)> identityGen = [](int n) {
    return n-1;
};

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

If desired, we could define this in src/random.h, so that users can just treat it as a black-box identity generator, and the actual details of the implementation are right next to those of the shuffle itself.

This comment has been minimized.

@bitcartel

bitcartel Nov 5, 2016
Author Contributor

I wrote the same function but decided not to use it because it was confusing. If it's an identity gen, why does the function not just return n? Why n-1? I can see why from looking at the code in random.h, but it wasn't obvious. I remember we had a discussion about the doc comment [0,n) but I think this is easy to miss.

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

This is why I suggested placing it in src/random.h. It's an identity function relative to the shuffle being used (a Durstenfeld shuffle). The shuffle iterates backwards over the array, swapping the pointed-at element with the element selected by gen (from 0 to the pointer inclusive, so gen(pointer+1) since gen(n) selects a number in the range 0 to n-1). So by always outputting n-1, gen always selects the pointer itself, meaning that the shuffle only swaps elements with themselves, making it a no-op or identity shuffle.

This comment has been minimized.

@bitcartel

bitcartel Nov 5, 2016
Author Contributor

Since we know that calling MappedShuffle with the default gen results in chained joinsplits failing, I think it would be more prudent for the purposes of this fix to not call MappedShuffle, even with an identity gen, in case there are some hidden side-effects.

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

In that case, I would prefer that you just not call JSDescription::Randomized at all, and instead adjust find_output to always return the given query input.

This comment has been minimized.

@str4d

str4d Nov 5, 2016
Contributor

Or, set inputmap and outputmap manually before the call to JSDescription to ensure they are set to what you expect, rather than relying on JSDescription::Randomized to set them.

This comment has been minimized.

@daira

daira Nov 5, 2016
Contributor

I would suggest just reverting 2eeb6b (but keeping the new test). That seems like a safer fix, since it is reverting to code that we were using on testnet for several releases.

@str4d
Copy link
Contributor

@str4d str4d commented Nov 5, 2016

As far as creating the desired outcome goes, this PR is good as-is. But I'm not comfortable changing the semantics of JSDescription::Randomized such that it doesn't do what it says it does in non-obvious circumstances, because I worry that we will forget to revert those changes when we figure out the actual fix, and then the altered behaviour will remain. However, I won't block on this if another engineer sides with @bitcartel after reviewing my reasoning.

@ebfull
Copy link
Contributor

@ebfull ebfull commented Nov 5, 2016

I want to investigate the root source, and then revert this commit when it's been fixed. If you agree, ACK and r+ this.

@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 5, 2016

Here is an alternate solution in PR #1796 - it reverts changes made to z_sendmany related to note randomization and adds the same chained joinsplit test.

@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 5, 2016

@ebfull @str4d Can you review #1796 ? Maybe that is preferable. Thanks.

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 5, 2016

I've read the comments, and I'm looking into #1796 now.

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 5, 2016

utACK, although I have the same "taste" as @str4d where I like the inside of a function to have less complexity (fewer branches), and instead to express special cases with special arguments, when possible. This isn't a blocking concern for me, though.

Off to test...

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 5, 2016

NACK:

Actually, coming back to this, I agree with @str4d: Let's define a well-known "identity permutation" that's part of the MappedShuffle API definition. Then, let's pass that into JSDescription::Randomized.

My reasoning is two-fold:

  • JSDescription::Randomized will have one branch removed from the current PR. (I want to reduce "modes of operation" so we stop running into "mainnet only bugs" and other kinds of problems due to "modal code".)
  • The resulting code will exercise all of MappedShuffle instead of shipping with inert code, so tests and live users will be exercising this code which we'll continue to use once we fix the chaining bug.
We use this function in z_sendmany as part of the fix for #1779.
@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 5, 2016

Added changes. I think #1796 is safer, but this PR with or without the identity function passes all tests too. I'll be offline for a bit (so if you need to make changes, please merge into this PR or create a new one) and I don't mind which PR you go with.

@str4d
Copy link
Contributor

@str4d str4d commented Nov 5, 2016

utACK, if we decide to go with this PR.

@str4d str4d modified the milestones: 1.0.3, 1.0.2 Nov 5, 2016
@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 6, 2016

Ping @nathan-at-least NACK -> ACK?

@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 6, 2016

ACK from @ebfull @str4d above and @nathan-at-least (out-of-band)

@bitcartel
Copy link
Contributor Author

@bitcartel bitcartel commented Nov 6, 2016

@zkbot r+

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 6, 2016

📌 Commit 38276c6 has been approved by bitcartel

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 6, 2016

Testing commit 38276c6 with merge fbc69d3...

zkbot pushed a commit that referenced this pull request Nov 6, 2016
…, r=bitcartel

Fixes #1779 so that sending to multiple zaddrs no longer fails.

Closes #1779

Commit 2eeb6b randomized the order of input and output notes,
but this is now known to prevent the chaining of multiple joinsplits
in a single transaction.  The root cause has yet to be determined.

This patch is a temporary fix and disables the shuffling of input
and output notes.  It also adds a chained joinsplit test to the
python qa test suite.
@bitcartel bitcartel dismissed str4d’s stale review Nov 6, 2016

@str4d has utACK'ed above

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 6, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 38276c6 into zcash:master Nov 6, 2016
1 check passed
1 check passed
homu Test successful
Details
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.

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