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

Sapling Support for z_mergetoaddress #3619

Merged
merged 8 commits into from Nov 17, 2018

Conversation

@Eirik0
Copy link
Contributor

Eirik0 commented Oct 24, 2018

Closes #3216.
Closes #3063.

@Eirik0 Eirik0 self-assigned this Oct 24, 2018

@Eirik0 Eirik0 added this to the v2.0.2 milestone Oct 24, 2018

@Eirik0 Eirik0 added this to In Review in Zcashd Team Oct 24, 2018

@Eirik0 Eirik0 requested review from str4d , bitcartel and daira and removed request for str4d and bitcartel Oct 24, 2018

@Eirik0 Eirik0 referenced this pull request Oct 24, 2018

Open

Look for TODOs and FIXME in code. #3546

20 of 25 tasks complete

@Eirik0 Eirik0 force-pushed the Eirik0:3216-z-mergetoaddress branch from 89e0b37 to 47776ec Oct 24, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 26, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 26, 2018

⌛️ Trying commit c5c1744 with merge 711e034...

zkbot added a commit that referenced this pull request Oct 26, 2018

Auto merge of #3619 - Eirik0:3216-z-mergetoaddress, r=<try>
Sapling Support for z_mergetoaddress [WIP]

Closes #3216
Also, this is the last item in #3063.

This PR has not yet incorporated the `Add Sprout support to TransactionBuilder` commit from @str4d. Currently an error is thrown when trying to do mix of Sprout and Sapling notes as input, and therefore using `"ANY_ZADDR"`  or `"*"` may not work if the wallet has a mix of sprout and sapling notes.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 26, 2018

💔 Test failed - pr-try

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 26, 2018

Fixed the rpc test. The gtests fail with:

[ RUN      ] TransactionBuilder.Invoke
gtest/test_transaction_builder.cpp:222: Failure
      Expected: static_cast<bool>(maybe_tx4)
      Which is: false
To be equal to: true
[  FAILED  ] TransactionBuilder.Invoke (65170 ms)

which passes for me locally. I'll investigate this further tomorrow.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 26, 2018

@zkbot try

zkbot added a commit that referenced this pull request Oct 26, 2018

Auto merge of #3619 - Eirik0:3216-z-mergetoaddress, r=<try>
Sapling Support for z_mergetoaddress [WIP]

Closes #3216
Also, this is the last item in #3063.

This PR has not yet incorporated the `Add Sprout support to TransactionBuilder` commit from @str4d. Currently an error is thrown when trying to do mix of Sprout and Sapling notes as input, and therefore using `"ANY_ZADDR"`  or `"*"` may not work if the wallet has a mix of sprout and sapling notes.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 26, 2018

⌛️ Trying commit 326d6c4 with merge f3a35de...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 26, 2018

💔 Test failed - pr-try

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 26, 2018

I am still unable to reproduce this locally, but here is more specific information about the failure:

[ RUN      ] TransactionBuilder.Invoke
unknown file: Failure
C++ exception with description "Could not create joinsplit description" thrown in the test body.
[  FAILED  ] TransactionBuilder.Invoke (64532 ms)
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@Eirik0 Eirik0 force-pushed the Eirik0:3216-z-mergetoaddress branch from 326d6c4 to 8a552ab Oct 29, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Nov 16, 2018

@bitcartel

The sprout qa test takes a long time, could it be shortened?

My changed were intended to be a straight refactor, so this test should function exactly the same as before. It probably could be shortened, but I found the coverage it provides helpful when adding the Sapling support. Also I think there is advantage to having both the Sprout and Sapling tests do the same thing (which they do currently).

Would be nice if the commits could be squashed into logical chunks of work.

Agreed. I will do this after having addressed the review comments.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Nov 16, 2018

@bitcartel I've addressed your comments. I will look in to squashing things together once you've double checked those.

@bitcartel
Copy link
Contributor

bitcartel left a comment

Changes look good!

@mdr0id mdr0id self-requested a review Nov 16, 2018

@mdr0id

mdr0id approved these changes Nov 16, 2018

Copy link
Contributor

mdr0id left a comment

utACK

@Eirik0 Eirik0 force-pushed the Eirik0:3216-z-mergetoaddress branch from 3cbfa31 to 172bdc4 Nov 16, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Nov 16, 2018

Reorganized and squashed some of the the commits.

Before rebasing/plan:

3a. e464a3d8f Rename mergetoaddress rpc test
3b. 340cee0cf Move test logic to helper class
3c. 9a7a5e32a cleanup and format
3d. 21692f684 Add mergetoaddress test for sapling
2. 76192e703 Add fail method to rpc test utils
3e. 6b0962dea Show more information when getting an unexpected exception
4a. a0233441f Remove pre-Sapling error check
4b. 0fea484d2 Reorganize checking for Sprout and Sapling
4c. b6129eabb Add sapling support to z_mergetoaddress
4d. c8e7c1f14 Sprout z_mergetoaddress inputs use Sprout spending keys
5. 4c15ac1a3 Add locking for Sapling notes
8. 7bf420a80 Adjust z_mergetoaddress assertions in Sapling rpc test
4e. 812af106a Replace ANY_ZADDR with ANY_SPROUT or ANY_SAPLING and remove *
1. 13b4f06c3 Fix z_mergetoaddress parameter tests
7. 9374b04d0 Add additional z_mergetoaddress parameter tests
4f. 5266dbf8a Increate default note limit for z_mergetoaddress
4g. 5218cf6cc fix comments and other minor cleanup
6. e5fbe69b8 Better error messages when sending between Sprout and Sapling
4i. 3cbfa3135 Do not allow mergetoaddress operations to be constructed with both Sprout and Sapling inputs

After rebasing:

1. Fix z_mergetoaddress parameter tests
2. Add fail method to rpc test utils
3. Extentend sprout mergetoaddress rpc test to also work for sapling
4. Add Sapling support to z_mergetoaddress
5. Add locking for Sapling notes
6. Better error messages when sending between Sprout and Sapling
7. Add additional z_mergetoaddress parameter tests
8. Adjust z_mergetoaddress assertions in Sapling rpc test

As a note, I didn't run in to any issues while doing this, the lines added/removed still match, and the mergetoaddress_sprout.py and mergetoaddress_sapling.py tests both pass for me locally.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 16, 2018

@Eirik0 Looks good, nice squashing!

Non-blocking, there is spelling mistake in commit message for 3: 'Extentend'

Otherwise, rebase (if necessary) after #3680 has merged and we can merge this PR.

@Eirik0 Eirik0 force-pushed the Eirik0:3216-z-mergetoaddress branch from 172bdc4 to 9d72f4d Nov 16, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Nov 16, 2018

Lol, whoops! Looks like I used the extended spelling :)

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

☔️ The latest upstream changes (presumably #3680) made this pull request unmergeable. Please resolve the merge conflicts.

@Eirik0 Eirik0 force-pushed the Eirik0:3216-z-mergetoaddress branch from 9d72f4d to e0b89f5 Nov 17, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 17, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

📌 Commit e0b89f5 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

⌛️ Testing commit e0b89f5 with merge 911d066...

zkbot added a commit that referenced this pull request Nov 17, 2018

Auto merge of #3619 - Eirik0:3216-z-mergetoaddress, r=bitcartel
Sapling Support for z_mergetoaddress

Closes #3216.
Closes #3063.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 911d066 to master...

@zkbot zkbot merged commit e0b89f5 into zcash:master Nov 17, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Nov 17, 2018

jl777 added a commit to jl777/komodo that referenced this pull request Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment