z_mergetoaddress does not support concurrent execution

[peterbitfly]

Describe the issue

Assume having 100 unspent notes which should be merged into one t1 address. Each z_mergetoaddress, with the default settings, will merge 10 notes into one output. When starting the z_mergetoaddress call 10 times (without waiting before the previous async z_mergetoaddress call finishes) will cause the first call to succeed and all consecutive calls to fail with the "18: bad-txns-joinsplit-requirements-not-met" error.

Can you reliably reproduce the issue?

If so, please list the steps to reproduce below:

1. Execute zcash-cli z_mergetoaddress '["ANY_ZADDR"]' t1xxx multiple times immediately after each other
2. The first call will succeed while all other consecutive calls will fail

Expected behaviour

Similar to z_sendmany, z_mergetoaddress should support firing off multiple calls at once.

The version of Zcash you were using:

1.0.15

[bitcartel]

Hi, when you say "without waiting" are you firing off the RPC calls from code eg. a Python client? Or do you mean on the command line? Thanks.

[str4d]

Firstly, thanks for testing this experimental feature! 😄

I believe this is because there is no locking mechanism for notes yet. z_mergetoaddress selects notes before creating the async operation (in order to provide note quantity feedback to the user). The same note is probably being selected multiple times, because all of the RPC calls happen in sequence in the time it takes to finish the first async call, after which the notes become spent and later calls fail.

It doesn't affect z_sendmany because note selection happens inside the async call, and the async runner currently only has a single thread, so all async operations happen in the order they were created. But if/when we add concurrent creation, z_sendmany would also have this problem.

The solution is to implement note locking and unlocking, which is #1377. See also #1277.

[peterbitfly]

Thanks for your response. After going over the z_sendmany implementations and its comments I figured that it will be related to the missing note locking.

Looking forward to the note locking implementation as it will certainly make the call more usable and less error prone.

[str4d]

I have reproduced this issue in an RPC test.

[bitcartel]

@str4d Where's the RPC test that reproduces the issue? I'd like to take a look. Thx.

[str4d]

@bitcartel it's the first commit in the note locking PR

…

On Wed, 28 Mar 2018, 1:00 AM bitcartel, ***@***.***> wrote: @str4d <https://github.com/str4d> Where's the RPC test that reproduces the issue? I'd like to take a look. Thx. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3046 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEwzB1Qq4l5Pkf9Db4yyhlEw9PpqexASks5tisR1gaJpZM4Sb4Oc> .

[braddmiller]

@bitcartel, as a followup to what @str4d said. The first commit in the PR has the original RPC tests, but they're slightly flawed. My understanding is that only one 0 fee transaction can be included in any single block. So the fee on one or both result1 and result2 should be changed to something >0.

[daira]

I wasn't aware of such a rule. Where is it implemented?

[braddmiller]

In CreateNewBlock in miner.cpp there is this idea of skipping a free transaction if the minimum block size is met. Check the few lines following this link. https://github.com/braddmiller/zcash/blob/b3a656cf535df043450cca4dc92a9137adc773f5/src/miner.cpp#L267

This manifested in the RPC tests as showing two transactions in the mempool corresponding to result1 and result2 but only including one at a time when generate was called and leaving the other in the mempool. Once a non-zero fee was set, generate was able to include both.

[bitcartel]

The conditional around continue won't ever evaluate to true, since bool fSortedByFee = (nBlockPrioritySize <= 0); which is false, as nBlockPrioritySize = std::min(nBlockMaxSize, nBlockPrioritySize); and if you work backwards and check the various constants, you'll find that nBlockPrioritySize is a big positive number.

[braddmiller]

Not sure it's true that this conditional will never evaluate to true, at least in the context of regtest. Doing manual debugging output to the console I believe I received a printout from inside that conditional, which is the only reason I noticed the 0 fee issue.

[braddmiller]

This issue should be resolved by PR 3106 which has been incorporated into master. If this issue persists please reopen this ticket and provide additional details on the failure.