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

Sprout -> Sapling migration status nullptr defence #3987

Merged
merged 3 commits into from May 6, 2019

Conversation

4 participants
@Eirik0
Copy link
Contributor

commented May 6, 2019

No description provided.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

It was discovered that when calling z_getmigrationstatus while migration transactions were in the mempool would cause nullptr to be dereferenced. The issue was was that the transactions are not yet associated with blocks, ad we were trying to get block times for nonexistent blocks. In fixing this issue, it was also discovered that the unmigrated_amount was incorrect in this situation - this is also fixed in the first commit of this PR.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

The second commit in the PR is defensive. We were concerned that storing the sapling migration async operation object could run into a race condition when trying to cancel it. The fix for this was to store the async operation's id rather than the operation itself, and then to go through the async rpc queue to recall, cancel, and remove migration operations which should not continue to execute.

As a note -
There is an independent issue: It is not possible to cancel async operations while they are running. AsyncRPCOperation provides a mechanism for creating custom cancel methods - one is to overrides the cancel method. The way the interface is currently designed, this is required to create operations that are cancellable while they are running. This issue is the cancel method is not declared as virtual, so this does not actually work, and, for whatever reason, adding the virtual keyword to this method exposes a circular dependency and causes the build to break. This needs to be fixed, but because of potential complexity. it should be done as a separate issue.

@Eirik0 Eirik0 added this to Needs Prioritization in Arborist Team via automation May 6, 2019

@str4d

str4d approved these changes May 6, 2019

Copy link
Contributor

left a comment

utACK. Please open an issue for the cancel() overriding problem.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Created #3988.

@daira

daira approved these changes May 6, 2019

Copy link
Contributor

left a comment

There's a typo in the commit comment of the second commit.

ut(ACK+cov); all comments are nonblocking.

@@ -3967,7 +3967,8 @@ UniValue z_getmigrationstatus(const UniValue& params, bool fHelp) {
{
std::vector<CSproutNotePlaintextEntry> sproutEntries;
std::vector<SaplingNoteEntry> saplingEntries;
pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, "", 1);
std::set<PaymentAddress> zaddrs;

This comment has been minimized.

Copy link
@daira

daira May 6, 2019

Contributor

I suggest renaming this to noFilter.

This comment has been minimized.

Copy link
@daira

daira May 6, 2019

Contributor

Also add a comment saying what this is intending to do. If I'm not mistaken, it's intended to find unspent notes sent in transactions at any chain depth or in the mempool, to any address for which we have a spending key, including locked notes.

@@ -4000,7 +4001,6 @@ UniValue z_getmigrationstatus(const UniValue& params, bool fHelp) {
continue;
}
migrationTxids.push_back(txPair.first.ToString());
CBlockIndex* blockIndex = mapBlockIndex[tx.hashBlock];
// A transaction is "finalized" iff it has at least 10 confirmations.
// TODO: subject to change, if the recommended number of confirmations changes.
if (tx.GetDepthInMainChain() >= 10) {

This comment has been minimized.

Copy link
@daira

daira May 6, 2019

Contributor

Is there a constant we can use instead of hardcoding 10 here?

This comment has been minimized.

Copy link
@Eirik0

Eirik0 May 6, 2019

Author Contributor

I was not able to find a constant for this.

@daira

daira approved these changes May 6, 2019

Copy link
Contributor

left a comment

Untested re-ACK

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

📌 Commit 94e419f has been approved by Eirik0

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

Auto merge of #3987 - Eirik0:migration-null-defence, r=Eirik0
Sprout -> Sapling migration status nullptr defence
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

⌛️ Testing commit 94e419f with merge cc80501...

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

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

@zkbot zkbot merged commit 94e419f into zcash:master May 6, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Needs Prioritization to Released (Merged in Master) May 6, 2019

@str4d str4d added this to the v2.0.5 milestone May 23, 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.