zcash / zcash

Zcash - Internet Money
https://z.cash/
Other
4.93k stars 2.04k forks source link

z_sendmany multiple output to the same address #2955

Open Tomas-M opened 6 years ago

Tomas-M commented 6 years ago

Zcash currently has memo field limited to 512 bytes. To owercome that limit, I was thinking to send the desired amount divided in multiple outputs, so I could push through longer memo (split into 512 byte chunks)

In order to simplify combining the memos later, I was thinking the best way could be to send all in a single transaction. But when I use a single z_sendmany call to send multiple amounts to a single output address, it does not work. Error message is Invalid parameter, duplicated address: ...

Here is what I am trying to do (let me split it into multiple lines and write the memos in plaintex, for better readability):

#!/bin/bash
FROM=t1abc...
OUT=zcABCD....
zcash-cli z_sendmany $FROM
[
  {"address":$OUT, "amount":0.1, "memo":"1/3 memo part 1..."},
  {"address":$OUT, "amount":0.1, "memo":"2/3 memo part 2..."},
  {"address":$OUT, "amount":0.1, "memo":"3/3 memo part 3..."},
   ...
]

Currently zcash does not allow this. If zcash allowed this, the receiver could very easily combine the memos to create a final memo longer than 512 bytes, while all would be under a SINGLE TRANSACTION ID, which would extremely simplify things.

Please consider allowing duplicite output address in z_sendmany.

Do you have any suggestion how to perhaps achieve similar thing? I know that if I just make multiple transactions, it will send it properly, but then each transaction will have different ID so it will not be obvious which memo part belongs to which other trasnaction.

Thank you

Tomas-M commented 6 years ago

It looks to me this change is very simple, simply remove the duplicity check in z_sendmany. As far as I tested, it works just fine. But I'm not sure if there are any consequences.

Tomas-M commented 6 years ago

I made few shielded transactions back and forth (on my private test network) and it works fine, coins can be spent and received without any issue.

This is the patch I've used:

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -3405,9 +3403,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
     if (outputs.size()==0)
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, amounts array is empty.");

-    // Keep track of addresses to spot duplicates
-    set<std::string> setAddress;
-
     // Recipients
     std::vector<SendManyRecipient> taddrRecipients;
     std::vector<SendManyRecipient> zaddrRecipients;
@@ -3437,10 +3432,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
             }
         }

-        if (setAddress.count(address))
-            throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address:     ")+address);
-        setAddress.insert(address);
-
         UniValue memoValue = find_value(o, "memo");
         string memo;
         if (!memoValue.isNull()) {
leto commented 6 years ago

@Tomas-M @daira this change is interesting to me from the point of view of Hushlist protocol, which attempts to support any size message and must break things into smaller memo chunks. In any case, this change would still only allow messages of size 54*512, do you foresee needing memos larger than that @Tomas-M ?

Tomas-M commented 6 years ago

I personaly don't need more than say 20. While you mention 54*512, where does this limit come from? I thought we're limited by total transaction size, which appears to be like 100KB, but 54*512 is around 27KB only.

ioptio commented 6 years ago

We probably won't change this for now even though it's an interesting proposal. Deduplication was added for efficiency reasons and might be fine especially for Sapling. We'll need to discuss this internally.

leto commented 6 years ago

@Tomas-M Given that the maximum size of a transaction is currently 100,000 bytes, the maximum number of joinsplits in a tx is 55. This is reduced to 54 to be conservative and ensure there is room for CTransaction data.see https://github.com/zcash/zcash/issues/1808 for details

Hushlist protocol will support large on-chain "memos" which are actually stored in different tx's and stitched together, that part of the protocol is not yet in the whitepaper: https://github.com/leto/hushlist/blob/master/whitepaper/protocol.pdf

I would love to talk more to see if HushList protocol can help VOT

leto commented 6 years ago

I have been experimenting with this on the Hush chain, here is a transaction with 54 duplicate zaddr receivers which encodes a 27KB PNG of the Hush logo:

https://explorer.myhush.org/tx/b2375b5ebbefffad6c0cdda7a2b1ee9ac2e0f206e37ebd2186c426bd65c024b7

One issue did arise that I wanted to ask @daira about: It seems that output order is not preserved inside a transaction, in a specific way, i.e. the first 2 outputs swapped their order. My first memo contained the magic bytes for a PNG header but for some reason it's order was swapped with the next output, but all the remaining outputs/memos stayed in the correct order. I have a hunch this could be related to giving the taddr change or internal JoinSplit magic that I don't fully understand.

My question is:

Does Zcash intend to preserve the order of outputs, i.e. from a z_sendmany and z_listreceivedbyaddress ? Is the above behavior a bug, or the intended behavior, or undefined behavior? It would be useful to specific in the docs.

I understand that Zcash does not allow duplicate receivers, but this issue still seems relevant to clarify for 3rd party developers. Thanks.

str4d commented 6 years ago

Does Zcash intend to preserve the order of outputs, i.e. from a z_sendmany and z_listreceivedbyaddress ? Is the above behavior a bug, or the intended behavior, or undefined behavior? It would be useful to specific in the docs.

Input and output orders are intentionally randomized, to help mitigate potential information leaks. See #778, #1561, #1790, and #1814.

Tomas-M commented 6 years ago

@leto: what I did for my purposes is to use first data byte of the memo as index, and then sort the memos in the transaction by it (simple by the hex value, sorting two-byte string like 00 01 02 03 04 is easy for my purposes)

LarryRuane commented 4 years ago

A couple of days ago, Aditya (Zecwallet developer) also asked for this change. Note that this restriction is not enforced by the consensus rules (you can create a raw transaction with multiple outputs to the same address).

This check originally comes from Bitcoin Core, 2011. I can't find any documentation on why this restriction exists; my guess is to prevent copy-and-paste errors when the user intends to send to several different outputs. There's probably little to no downside to removing this restriction.

hloo commented 4 years ago

This change would also be helpful for an application that I am working on.

This check originally comes from Bitcoin Core, 2011. I can't find any documentation on why this restriction exists; my guess is to prevent copy-and-paste errors when the user intends to send to several different outputs. There's probably little to no downside to removing this restriction.

@LarryRuane I think that your intuition is correct. See:

https://github.com/bitcoin/bitcoin/pull/2366

Bitcoin did not accept this pull request because doing so would have run afoul of the JSON-RPC standard, but that objection does not apply to z_sendmany. (Unlike Zcash's z_sendmany, in Bitcoin's sendmany there is a JSON object containing address-amount key-value pairs, so allowing duplicate addresses would lead to duplicate keys.)

nuttycom commented 2 years ago

I think we should just add a flag to z_sendmany that disables the redundancy check; the check would remain on by default but would be skipped if you set the flag.