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

Adjust fees to that z_sendmany tx using default built-in fee are not rejected by nodes #1855

Merged

Conversation

bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Nov 14, 2016

Resolves #1719 by setting min relay tx fee to 1000 to match upstream (they were experimenting wtih 5000 when zcash forked from upstream).

Temporary resolution for #1851 so that z_sendmany with thousands of taddr recipients will be accepted and not rejected because of insufficient fee. Users are currently only able to use the default built-in fee as there is no option to adjust the fee in the z_sendmany interface and no fee calculation policy has been agreed upon yet.

@bitcartel bitcartel added engineering meeting agenda S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-rpc-interface Area: RPC interface A-wallet Area: Wallet labels Nov 14, 2016
@bitcartel bitcartel added this to the 1.0.3 milestone Nov 14, 2016
@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 14, 2016

@ebfull @nathan-at-least @str4d Discussed during meeting. Please review and confirm ACK.

@ebfull
Copy link
Contributor

ebfull commented Nov 15, 2016

ACK

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Nov 15, 2016

📌 Commit 1069848 has been approved by ebfull

@zkbot
Copy link
Contributor

zkbot commented Nov 15, 2016

Testing commit 1069848 with merge 08d90e1...

zkbot pushed a commit that referenced this pull request Nov 15, 2016
…ny_tx, r=ebfull

Adjust fees to that z_sendmany tx using default built-in fee are not rejected by nodes

Resolves #1719 by setting min relay tx fee to 1000 to match upstream (they were experimenting wtih 5000 when zcash forked from upstream).

Temporary resolution for #1851 so that z_sendmany with thousands of taddr recipients will be accepted and not rejected because of insufficient fee.  Users are currently only able to use the default built-in fee as there is no option to adjust the fee in the z_sendmany interface and no fee calculation policy has been agreed upon yet.
@zkbot
Copy link
Contributor

zkbot commented Nov 15, 2016

💔 Test failed - zcash

@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 15, 2016

Investigating.

bitcartel added 2 commits Nov 15, 2016
A txout will be considered dust when it has a value <546 zatoshis.
Helps to address zcash#1719.
Issue zcash#1851 shows that a zaddr->taddr can be rejected from mempools
due to not meeting fee requirements given the size of the transaction.
Fee calculation for joinsplit txs has not yet been agreed upon, so
during this interim period, this patch ensures  joinsplit txs using
the default fee are not rejected due to an insufficient fee.
@bitcartel bitcartel force-pushed the 1719_1851_fee_adjustment_for_z_sendmany_tx branch from 1069848 to 9ddb6ad Compare Nov 15, 2016
@bitcartel
Copy link
Contributor Author

bitcartel commented Nov 15, 2016

Incorrect method name used in test. Now fixed to self.wait_and_assert_operationid_status(myopid).

Not doing zkbot retry as that failed on PR 1847.

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Nov 15, 2016

📌 Commit 9ddb6ad has been approved by bitcartel

@zkbot
Copy link
Contributor

zkbot commented Nov 15, 2016

Testing commit 9ddb6ad with merge 9ddb6ad...

@zkbot
Copy link
Contributor

zkbot commented Nov 15, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 9ddb6ad into zcash:master Nov 15, 2016
1 check passed
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 A-wallet Area: Wallet S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants