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

Add transaction size and zaddr output limit checks to z_sendmany #1808

Merged
merged 1 commit into from Nov 7, 2016

Conversation

@bitcartel
Copy link
Contributor

bitcartel commented Nov 6, 2016

Estimate and check if the size of a transaction, for a given number of taddr and zaddr outputs, will be valid. Based on the current implementation of z_sendmany, one zaddr output requires (at least) one joinsplit. Future optimizations should reduce the number of joinsplits required.

The distribution of value across input notes impacts the number of joinsplits required for a zaddr output e.g. small notes being used to send a big value to a zaddr. We should add more sophisticated checking in future.

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.

In practice, we might want to communicate and recommend a user to use upto 50 zaddr outputs, which will require 50 joinsplits, and take around 25-30 minutes to compute on an i7.

@str4d
Copy link
Contributor

str4d commented Nov 6, 2016

utACK, only comment is on the size of the CTxIn: how large is an empty one? The transparent dust threshold used 148 bytes as the minimum CTxIn size required to spend a transparent output, so as long as we are consistent with that, I'm happy.

@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 6, 2016

Empty CTxIn is only 41 bytes. Updating patch to use 148 bytes, per the comment in transaction.h.

@sammy007
Copy link

sammy007 commented Nov 6, 2016

Please add an RPC call to check if TX satisfies the conditions so pools can check it before submission.

@bitcartel bitcartel force-pushed the bitcartel:master_z_sendmany_limits branch from 8d3338f to 6c22ca0 Nov 6, 2016
@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 7, 2016

@sammy007 Estimating and checking the size of the transaction will happen before any joinsplit processing takes place, so you won't spend any computing resources up-front.

// If input notes are small, we might actually require more than one joinsplit per zaddr output.
// For now though, we assume we use one joinsplit per zaddr output (and the second output note is change).
// We reduce the result by 1 to ensure there is room for non-joinsplit CTransaction data.
#define Z_SENDMANY_MAX_ZADDR_OUTPUTS ((MAX_TX_SIZE / 1802) - 1) // Serialized JSDescription is 1802 bytes

This comment has been minimized.

Copy link
@ebfull

ebfull Nov 7, 2016

Contributor

I would rather use GetSerializeSize() on a blank JSDescription (they are constant size) than forget to modify this constant. :)

This comment has been minimized.

Copy link
@bitcartel

bitcartel Nov 7, 2016

Author Contributor

Done.

@@ -3284,6 +3292,29 @@ Value z_sendmany(const Array& params, bool fHelp)
}
}

// Check the number of zaddr outputs does not exceed the limit.
if (zaddrRecipients.size() > Z_SENDMANY_MAX_ZADDR_OUTPUTS) {

This comment has been minimized.

Copy link
@ebfull

ebfull Nov 7, 2016

Contributor

What if a change address takes this over the limit? Are there other ways the underlying API might add data to the transaction that wouldn't be accounted for in these changes?

This comment has been minimized.

Copy link
@bitcartel

bitcartel Nov 7, 2016

Author Contributor

Added txout change, which is likely, when sending from taddr.

@ebfull
Copy link
Contributor

ebfull commented Nov 7, 2016

I would rather add this kind of stuff to an underlying abstraction than to sendmany in particular. We need a transaction assembler.

ACK modulo comments.

Copy link
Contributor

nathan-at-least left a comment

utACK. Minor request to replace magic numbers with named constants.

CTransaction tx(mtx);
txsize += tx.GetSerializeSize(SER_NETWORK, tx.nVersion);
if (fromTaddr) {
txsize += 148; // transaction.h comment: spending taddr output requires CTxIn >= 148 bytes.

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Nov 7, 2016

Contributor

Replace 148 with a named constant or #define.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Nov 7, 2016

Author Contributor

Ok

if (fromTaddr) {
txsize += 148; // transaction.h comment: spending taddr output requires CTxIn >= 148 bytes.
}
txsize += 34 * taddrRecipients.size(); // rule of thumb, a regular taddr output is 34 bytes

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Nov 7, 2016

Contributor

Replace 34 with a named constant or #define.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Nov 7, 2016

Author Contributor

Ok

@bitcartel bitcartel force-pushed the bitcartel:master_z_sendmany_limits branch from 6c22ca0 to 3920292 Nov 7, 2016
@ebfull
Copy link
Contributor

ebfull commented Nov 7, 2016

utACK w.r.t new commits.

@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 7, 2016

@nathan-at-least
Copy link
Contributor

nathan-at-least commented Nov 7, 2016

@bitcartel utACK on my minor nits about defined constants.

I've run the "wallet test" on this branch as is, and now I'm going to rerun it after reverting all the non-test changes.

@nathan-at-least
Copy link
Contributor

nathan-at-least commented Nov 7, 2016

ACK w/ pos/neg verification of the wallet test.

@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 7, 2016

@zkbot r+

@zkbot
Copy link
Collaborator

zkbot commented Nov 7, 2016

📌 Commit 3920292 has been approved by bitcartel

@zkbot
Copy link
Collaborator

zkbot commented Nov 7, 2016

Testing commit 3920292 with merge 3920292...

@zkbot
Copy link
Collaborator

zkbot commented Nov 7, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 3920292 into zcash:master Nov 7, 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.

None yet

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