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

3938 migration status #3973

Merged
merged 9 commits into from May 1, 2019

Conversation

@Eirik0
Copy link
Contributor

commented Apr 26, 2019

Closes #3938

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

This depends on #3888 and #3967. The last 3 commits are the ones relevant to this PR.

@Eirik0 Eirik0 added this to Needs Prioritization in Arborist Team via automation Apr 26, 2019

@Eirik0 Eirik0 self-assigned this Apr 26, 2019

@Eirik0 Eirik0 moved this from Needs Prioritization to Arborist Product Priorities in Arborist Team Apr 26, 2019

@Eirik0 Eirik0 moved this from Arborist Product Priorities to In Review in Arborist Team Apr 26, 2019

@Eirik0 Eirik0 requested review from str4d, daira, mdr0id and bitcartel Apr 26, 2019

@LarryRuane LarryRuane self-requested a review Apr 29, 2019

@daira

daira approved these changes Apr 29, 2019

Copy link
Contributor

left a comment

utACK, and ACK for partial coverage, but there is no coverage of the -migrationdestaddress option.

Show resolved Hide resolved src/wallet/wallet.cpp
Show resolved Hide resolved src/wallet/asyncrpcoperation_saplingmigration.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@Eirik0 Eirik0 referenced this pull request Apr 29, 2019

Merged

3961 migration address #3967

@Eirik0 Eirik0 force-pushed the Eirik0:3938-migration-status branch from 704a054 to cb18d93 Apr 29, 2019

@Eirik0 Eirik0 force-pushed the Eirik0:3938-migration-status branch from cb18d93 to b4ecb12 Apr 29, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I've removed the refactoring of GenerateNewSaplingZKey. (Part of the reason for doing this was to make it easier to get the default sapling address on demand when testing, but this was made moot by the following.) I've also update the test use an address provided by the -migrationdestaddress option.

Making these changes, I noticed that the help message for -migration was not consistent with other help messages for boolean arguments, so I have added another commit to address that.

@Eirik0 Eirik0 requested a review from str4d Apr 29, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

As a note note: The in the ZIP we use the word confirmed to mean a transaction with 10 confirmations. This was changed to finalized at the request of @ioptio and @nathan-at-least. I have updated zcash/zips#229 to note that this change needs to happen there too.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I updated the rpc test to check the migration using both the parameter from the conf file and the default Sapling address. This is accomplished by essentially running the test twice. First, we run the test with one node which has the -migrationdestaddress=... (and -migration) parameter set. This node does a round of migrating at block 500. Then we run the test again with another node that does not have the parameters set (and therefore will use the default Sapling address) which migrates at block 1000.

@daira

daira approved these changes Apr 29, 2019

Copy link
Contributor

left a comment

ut(ACK+cov). Some typos, nonblocking.

Show resolved Hide resolved qa/rpc-tests/sprout_sapling_migration.py Outdated
Show resolved Hide resolved qa/rpc-tests/sprout_sapling_migration.py Outdated
Show resolved Hide resolved qa/rpc-tests/sprout_sapling_migration.py Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

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

@Eirik0 Eirik0 force-pushed the Eirik0:3938-migration-status branch from 5ba36ec to 108e587 Apr 29, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Rebased on to master.

CAmount migrationAmount = 0;
for (const auto& js : tx.vjoinsplit) {
migrationAmount += js.vpub_new;
}

This comment has been minimized.

Copy link
@str4d

str4d Apr 29, 2019

Contributor

This doesn't take into account fees, and it is more likely than not that users will assume the value shown in the output is the amount they have received in the destination address. Instead, you could iterate over js.vpub_old looking for non-zero amounts to continue from, and then just use -valueBalance as the migration amount.

It's unclear to me which strategy is better when taking into account possible non-migration transactions, specifically ones that include transparent inputs or outputs. Did the ZIP not define a migration transaction as having vin.empty() && vout.empty()?

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 29, 2019

Author Contributor

I think it makes sense to update the spec to include vin.empty() && vout.empty() in the definitions of migration transactions. Also I think it is correct to to use -valueBalance as opposed to js.vpub_new, so I will update that. Regarding js.vpub_old, it might make sense to change the definition to be one or more Sprout JoinSplits all zero vpub_old fields rather than one or more Sprout JoinSplits with nonzero vpub_new field, but would require more discussion so I am leaving it is for the time being.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 29, 2019

Author Contributor

One thing to note is that the changes regarding vin, vout, and the proposed change regarding vpub_old will only effect manually created migration transactions which may be considered unsupported as there is no way to create them using a standard zcashd node. For that reason I do not think it is super pressing to switch to using vpub_old rather than vpub_new, but it is still something to consider.

This comment has been minimized.

Copy link
@daira

daira Apr 30, 2019

Contributor

I've thought about this and continue to believe that the current definition in the ZIP is the simplest that satisfies the needed criteria.

This comment has been minimized.

Copy link
@daira

daira Apr 30, 2019

Contributor

In particular, what would be the rationale of requiring that migration transactions' vpub_olds are zero, but not also requiring that they have no transparent inputs? Do we also require that they have no transparent outputs? Where do we stop?

This comment has been minimized.

Copy link
@daira

daira Apr 30, 2019

Contributor

Agreed that it is correct to use -valueBalance. I think that gives a sensible result independent of the migration transaction criteria; what we're saying is "there are transactions that in total transferred X into Sapling, at least some of it coming from Sprout (and for transactions created by zcashd, exactly X plus fees will be from Sprout)".

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 30, 2019

Author Contributor

Where do we stop?

Good point. It is a bit of a slippery slope. I will change it back to not look at vin/vout.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Addressed @daira's comments.

@mms710 mms710 requested a review from daira Apr 30, 2019

@str4d

str4d approved these changes Apr 30, 2019

Copy link
Contributor

left a comment

ut(ACK+cov). Comment can be addressed here or in a subsequent PR.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@daira

daira approved these changes Apr 30, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

@daira

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

📌 Commit e14cf96 has been approved by daira

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

⌛️ Testing commit e14cf96 with merge 87a72b0...

zkbot added a commit that referenced this pull request Apr 30, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

The failure was an actual failure caused by the refactoring in 52cfa9c. In rpc_wallet_tests/rpc_wallet_z_exportwallet we run an export on a wallet that does not have an HD Seed. The refactoring (which in hindsight should not have updated this method) caused this to throw an exception. The fix was to partially revert the aforementioned commit, removing the changes made in rpcdump.cpp

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

⌛️ Trying commit 3cad5f4 with merge 981926d...

zkbot added a commit that referenced this pull request Apr 30, 2019

@Eirik0 Eirik0 moved this from In Review to Merge Queue in Arborist Team May 1, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

☀️ Test successful - pr-try
State: approved= try=True

@mdr0id

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

📌 Commit 3cad5f4 has been approved by mdr0id

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

⌛️ Testing commit 3cad5f4 with merge c23a365...

zkbot added a commit that referenced this pull request May 1, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Looks like a transient failure.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

⌛️ Testing commit 3cad5f4 with merge bf22048...

zkbot added a commit that referenced this pull request May 1, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

☀️ Test successful - pr-merge
Approved by: mdr0id
Pushing bf22048 to master...

@zkbot zkbot merged commit 3cad5f4 into zcash:master May 1, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) May 1, 2019

@str4d str4d added this to the v2.0.5 milestone May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.