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

Sapling migration RPC #3888

Merged
merged 7 commits into from Apr 27, 2019

Conversation

@Eirik0
Copy link
Contributor

commented Mar 14, 2019

Closes #3873

@Eirik0 Eirik0 self-assigned this Mar 14, 2019

@mms710 mms710 added this to Needs Prioritization in Arborist Team via automation Mar 14, 2019

@mms710 mms710 moved this from Needs Prioritization to R&D Backlog in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from R&D Backlog to Sapling Priorities in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Sapling Priorities to Current Sprint in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Current Sprint to In Progress in Arborist Team Mar 14, 2019

@Eirik0 Eirik0 force-pushed the Eirik0:3873-sapling-migration branch 3 times, most recently from 1aee041 to 9c97c2e Mar 18, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Force pushed with some updates after pairing with @str4d.

Note: This is dependent on #3848. I will rebase on master and fix the merge conflict once that goes in.

To reviewers: The only commit relevant to this PR (currently) is the last one.

@Eirik0 Eirik0 changed the title [WIP] 3873 sapling migration Sapling migration RPC Mar 19, 2019

@Eirik0 Eirik0 moved this from In Progress to In Review in Arborist Team Mar 19, 2019

@Eirik0 Eirik0 requested review from str4d, daira and bitcartel Mar 19, 2019

@ioptio ioptio added this to Blocked/Tracking in Documentation Mar 28, 2019

@Eirik0 Eirik0 force-pushed the Eirik0:3873-sapling-migration branch from 9c97c2e to 9803a18 Apr 9, 2019

@daira
Copy link
Contributor

left a comment

fSaplingMigrationEnabled is shared between multiple threads and therefore all accesses to it (read and write) need to be locked.

(I'll rereview once that is fixed.)

Show resolved Hide resolved src/miner.cpp Outdated
Show resolved Hide resolved src/gtest/test_miner.cpp Outdated
@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@daira regarding locking for fSaplingMigrationEnabled, I am not really sure what to lock on. I am also less familiar than I could be with synchronization in cpp. In Java, I could create some dummy object and then synchronize on that wherever we modify or read the boolean. There are also more sophisticated ways of doing this using reentrant locks and the like.

What do you suggest here? Perhaps it would be sufficient to use a std::atomic<bool>?

@str4d
Copy link
Contributor

left a comment

Looking nice overall!

@@ -3104,6 +3105,10 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock *

EnforceNodeDeprecation(pindexNew->nHeight);

if (fSaplingMigrationEnabled) {
GetMainSignals().RunSaplingMigration(pindexNew->nHeight);
}

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

This introduces a new global dependency between pwalletMain and the main node. Instead, I would prefer that fSaplingMigrationEnabled be a property of CWallet, and we provide here a generic signal indicating that the chain tip is now at height pindexNew->nHeight (which in fact, we already do with the ChainTip() signal).

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

With the above change, locking of fSaplingMigrationEnabled would be handled by CWallet::cs_wallet.

Show resolved Hide resolved src/wallet/wallet.cpp
"\nResult:\n"
"enabled (boolean) Whether or not migration is enabled (echos the argument).\n"
);
fSaplingMigrationEnabled = params[0].get_bool();

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor
Suggested change
fSaplingMigrationEnabled = params[0].get_bool();
LOCK(pwalletMain->cs_wallet);
pwalletMain->fSaplingMigrationEnabled = params[0].get_bool();
throw runtime_error(
"z_setmigration enabled\n"
"When enabled the Sprout to Sapling migration will attempt to migrate all funds from this wallet’s\n"
"Sprout addresses to either the address for Sapling account 0 or one specified by the parameter\n"

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor
Suggested change
"Sprout addresses to either the address for Sapling account 0 or one specified by the parameter\n"
"Sprout addresses to either the address for Sapling account 0 or the address specified by the parameter\n"
"z_setmigration enabled\n"
"When enabled the Sprout to Sapling migration will attempt to migrate all funds from this wallet’s\n"
"Sprout addresses to either the address for Sapling account 0 or one specified by the parameter\n"
"'-migrationdestaddress'. This migration is designed to minimize information leakage. As a result,\n"

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

Insert newlines into the output before This migration to make it a separate paragraph, for readability.

"\nArguments:\n"
"1. enabled (boolean, required) 'true' or 'false' to enable or disable respectively.\n"
"\nResult:\n"
"enabled (boolean) Whether or not migration is enabled (echos the argument).\n"

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

Is there precedent for echoing the argument here? Do other RPCs do something similar when they don't have their own results to return, or do they usually return nothing? Alternatively, is there a specific use-case in mind for the returned argument?

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 16, 2019

Author Contributor

I didn't do this for any particular reason. my guess is returning nothing is more common in similar situations. I will change this to return NullUniValue.

Show resolved Hide resolved src/wallet/wallet.cpp
saplingMigrationOperation = operation;
q->addOperation(operation);
} else if (blockHeight % 500 == 499) {
for (const CTransaction& transaction : pendingSaplingMigrationTxs) {

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

Related to the pendingSaplingMigrationTxs race condition: we could call saplingMigrationOperation->cancel() before this loop to ensure that pendingSaplingMigrationTxs is not mutated while using it.

Speculation This might also help in a possibly-adversarial edge case: if we reach this height before all transactions are generated, but then get rolled back a block or two, the remaining transactions would be finished and added to pendingSaplingMigrationTxs. The remaining transactions would then be published when this code is re-executed (when this height is reached again). This seems like an avenue for an adversary to measure the performance of the node, which might be a useful (though very low-bandwidth and noisy) side-channel.

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

The ZIP should be updated to specify this behaviour.

// Derive m/32'/coin_type'
auto m_32h_cth = m_32h.Derive(bip44CoinType | ZIP32_HARDENED_KEY_LIMIT);

// Derive account key at next index, skip keys already known to the wallet

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

Stale copy-pasta comment.

Suggested change
// Derive account key at next index, skip keys already known to the wallet
// Derive m/32'/coin_type'/0'
if type(result['result']) is dict and result['result'].get('txid'):
ret = result['result']['txid']
else:
ret = result['result']

This comment has been minimized.

Copy link
@str4d

str4d Apr 15, 2019

Contributor

Which RPC required making this change?

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 17, 2019

Author Contributor

This change was to accommodate AsyncRPCOperation_saplingmigration which does not return a txid in it's result. This ended up being a little uglier than I had expected, so I am open to suggestions.

This comment has been minimized.

Copy link
@str4d

str4d Apr 17, 2019

Contributor

This particular function is expected to return a txid, so if you want to access the entire result, you should refactor this into two functions: an inner function that returns the entire result, and the existing function that calls the inner function, then only returns the txid (and returns None in the case of z_saplingmigration).

@Eirik0 Eirik0 force-pushed the Eirik0:3873-sapling-migration branch from 9803a18 to 55738cd Apr 18, 2019

@daira

daira approved these changes Apr 23, 2019

Copy link
Contributor

left a comment

ut(ACK+cov) modulo comments.


assert_equal(in_status, status, "Operation returned mismatched status. Error Message: {}".format(errormsg))

if errormsg is not None:
assert_true(in_errormsg is not None, "No error retured. Expected: {}".format(errormsg))

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

I don't understand this assertion message. This is the case where we did have an error but didn't specify an expected error message in the test, right?

Suggested change
assert_true(in_errormsg is not None, "No error retured. Expected: {}".format(errormsg))
assert_true(in_errormsg is not None, "Unexpected error returned: {}".format(errormsg))

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 23, 2019

Author Contributor

Looking back at this, I don't really understand it either. I am changing it to check assert_equal(in_errormsg, errormsg) in the if status == "failed": block rather than the two asserts that we have.

std::vector<SaplingNoteEntry> saplingEntries;
{
LOCK2(cs_main, pwalletMain->cs_wallet);
// Consider, should notes be sorted?

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

I thought they were already sorted by value in GetFilteredNotes? I could be wrong.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 23, 2019

Author Contributor

This is not the case. We sort do in asyncrpcoperation_sendmany, and possibly other places, but not GetFilteredNotes.

// sort in descending order, so big notes appear first

// Consider, should notes be sorted?
pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, "", 0);
}
if (sproutEntries.empty()) { // Consider, should the migration remain enabled?

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

If not then that should be added to ZIP 308.

Leaving it enabled might be more convenient: the user could receive further funds to Sprout addresses, and probably also wants those migrated.

On the other hand, I'm concerned that migration is left enabled then eventually there won't be a large enough anonymity set of other migration transactions to hide in. I'm not sure what we can do about that.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Apr 23, 2019

Author Contributor

This could be covered by, "if the remaining amount to be migrated is less than 0.01 ZEC, end the migration," but we could more explicit. I agree that some users may want to leave this enabled, so this could be revisited, but eventually sending funds to a Sprout address will be disabled and this will become a moot point.

// The amount chosen *includes* the 0.0001 ZEC fee for this transaction, i.e.
// the value of the Sapling output will be 0.0001 ZEC less.
builder.SetFee(FEE);
builder.AddSaplingOutput(ovkForShieldingFromTaddr(seed), migrationDestAddress, amountToSend - FEE);

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

The ZIP should probably specify that ovk is generated as if for a transfer from a t-addr.

"Sprout addresses to either the address for Sapling account 0 or the address specified by the parameter\n"
"'-migrationdestaddress'.\n"
"\n"
"This migration is designed to minimize information leakage. As a result,\n"

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

Nit: this line is significantly shorter than the others in the paragraph.

saplingMigrationOperation = operation;
q->addOperation(operation);
} else if (blockHeight % 500 == 499) {
for (const CTransaction& transaction : pendingSaplingMigrationTxs) {

This comment has been minimized.

Copy link
@daira

daira Apr 23, 2019

Contributor

The ZIP should be updated to specify this behaviour.

@mms710 mms710 requested a review from str4d Apr 23, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@daira I created zcash/zips#229 as a reminder to update the ZIP.

@Eirik0 Eirik0 force-pushed the Eirik0:3873-sapling-migration branch from 55738cd to 4ee0747 Apr 23, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Force pushed with changes to reflect @daira's comments (documentation text alignment and fixed confusing assert)

@Eirik0 Eirik0 referenced this pull request Apr 23, 2019

Merged

3961 migration address #3967

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

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

@Eirik0 Eirik0 force-pushed the Eirik0:3873-sapling-migration branch from 4ee0747 to d2a584e Apr 24, 2019

@str4d
Copy link
Contributor

left a comment

A couple more points that need to be addressed.

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

@str4d

str4d approved these changes Apr 25, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

📌 Commit 5519d16 has been approved by Eirik0

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

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

⌛️ Testing commit 5519d16 with merge 489e396...

@Eirik0 Eirik0 referenced this pull request Apr 26, 2019

Merged

3938 migration status #3973

@Eirik0 Eirik0 moved this from In Review to Merge Queue in Arborist Team Apr 26, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

The failure was an actual failure in wallet_protectcoinbase caused by d2a584e. I added another commit to update that test to check for the full error message when asserting failures in async rpcs.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

📌 Commit 6acb37b has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

⌛️ Testing commit 6acb37b with merge bfde79a...

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

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

The failures appear to be timeouts.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

⌛️ Testing commit 6acb37b with merge 2082161...

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

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing 2082161 to master...

@zkbot zkbot merged commit 6acb37b into zcash:master Apr 27, 2019

1 check passed

homu Test successful
Details

Documentation automation moved this from Blocked/Tracking to Complete Apr 27, 2019

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

@mms710 mms710 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.