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

z_sendmany multiple output to the same address #2955

Open
Tomas-M opened this issue Feb 16, 2018 · 12 comments
Open

z_sendmany multiple output to the same address #2955

Tomas-M opened this issue Feb 16, 2018 · 12 comments
Assignees
Labels
A-rpc-interface Area: RPC interface E-good-first-issue Effort: Suitable for someone new to the codebase. F-memo-field Feature: Memo field usability

Comments

@Tomas-M
Copy link
Contributor

Tomas-M commented Feb 16, 2018

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
Copy link
Contributor Author

Tomas-M commented Feb 16, 2018

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
Copy link
Contributor Author

Tomas-M commented Feb 16, 2018

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()) {

@Tomas-M Tomas-M changed the title z_sendmany multiple output to the same address fails z_sendmany multiple output to the same address Feb 16, 2018
@daira daira added A-rpc-interface Area: RPC interface F-memo-field Feature: Memo field C-simplification Category: Changes that simplify the protocol specification or consensus rules. feature selection meeting agenda labels Feb 17, 2018
@leto
Copy link
Contributor

leto commented Feb 18, 2018

@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
Copy link
Contributor Author

Tomas-M commented Feb 18, 2018

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
Copy link
Contributor

ioptio commented Feb 19, 2018

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
Copy link
Contributor

leto commented Feb 19, 2018

@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 #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
Copy link
Contributor

leto commented Mar 12, 2018

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
Copy link
Contributor

str4d commented Mar 13, 2018

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
Copy link
Contributor Author

Tomas-M commented Mar 13, 2018

@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
Copy link
Collaborator

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
Copy link

hloo commented May 14, 2020

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:

bitcoin/bitcoin#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.)

@str4d str4d added usability and removed feature selection meeting agenda C-simplification Category: Changes that simplify the protocol specification or consensus rules. labels Aug 18, 2020
@nuttycom nuttycom added the E-good-first-issue Effort: Suitable for someone new to the codebase. label Aug 10, 2022
@nuttycom
Copy link
Contributor

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.

@sellout sellout self-assigned this Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface E-good-first-issue Effort: Suitable for someone new to the codebase. F-memo-field Feature: Memo field usability
Projects
None yet
Development

No branches or pull requests

9 participants