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

Cache Sapling witnesses in the wallet #3353

Merged
merged 18 commits into from Jul 26, 2018

Conversation

@Eirik0
Copy link
Contributor

Eirik0 commented Jun 21, 2018

Closes #3062

I have not update the tests in test_wallet.cpp. Also, there are several other methods in the wallet that have to do with witnesses and note data which will need to be updated, but this PR focuses on IncrementNoteWitnesses and DecrementNoteWitnesses.

@str4d str4d changed the title 3062 cache sapling witnesses Cache Sapling witnesses in the wallet Jul 2, 2018

@str4d str4d added this to the v2.0.0 milestone Jul 2, 2018

@str4d str4d added the wallet label Jul 2, 2018

@str4d str4d added this to In Progress in Zcashd Team Jul 2, 2018

@str4d str4d moved this from In Progress to In Review in Zcashd Team Jul 9, 2018

@str4d
Copy link
Contributor

str4d left a comment

Solid PR overall 🙂

noteData[jsoutpt] = nd;
wtx.SetNoteData(noteData);
wallet.AddToWallet(wtx, true, NULL);

block.vtx.push_back(wtx);
wallet.IncrementNoteWitnesses(&index, &block, tree);

return jsoutpt;
return jsoutpt;

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Remove whitespace from this commit.

@@ -326,14 +326,14 @@ class JSDescription
};

/** An outpoint - a combination of a transaction hash and an index n into its vout */

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Move this comment down so it stays with COutPoint (as it specifically refers to vout).

@@ -144,7 +144,7 @@ uint256 JSDescription::h_sig(ZCJoinSplit& params, const uint256& joinSplitPubKey
return params.h_sig(randomSeed, nullifiers, joinSplitPubKey);
}

std::string COutPoint::ToString() const
std::string BaseOutPoint::ToString() const

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Un-rename this, and instead implement ToString() for SaplingOutPoint (so it can correctly refer to itself in the string instead of COutPoint).

{
return (a.hash == b.hash && a.n == b.n);
}

friend bool operator!=(const COutPoint& a, const COutPoint& b)
friend bool operator!=(const BaseOutPoint& a, const BaseOutPoint& b)
{
return !(a == b);
}

std::string ToString() const;

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Per above, move this into COutPoint and SaplingOutPoint.

@@ -458,6 +468,8 @@ class CWalletTx : public CMerkleTx
READWRITE(nTimeReceived);
READWRITE(fFromMe);
READWRITE(fSpent);
// TODO:
//READWRITE(mapSaplingNoteData);

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Since we know that only v4 Sapling transactions will ever have this field set, we should be able to make this READWRITE conditional on the transaction version, similar to how we turn on or off parts of the CTransaction serialization on isV4Sapling.

However, do not implement this until we have finished adding all the necessary fields to SaplingNoteData, otherwise we may run into serialization versioning issues.

}
}
::UpdateWitnessHeights(wtxItem.second.mapSproutNoteData, pindex->nHeight, nWitnessCacheSize);
::UpdateWitnessHeights(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize);

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Move this into the Increment sapling note witnesses commit, so this commit is just the extraction.

if (nd->witnessHeight < indexHeight) {
nd->witnessHeight = indexHeight;
// Check the validity of the cache
// See earlier comment about validity.

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

// See comment in CopyPreviousWitnesses about validity.

assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, tree));
assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, sproutTree));
// TODO:
//assert(pcoinsTip->GetSaplingAnchorAt(pindex->hashSaplingAnchor, saplingTree));

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor
if (pindex->pprev) {
    assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingAnchor, saplingTree));
}
// Sapling
for (uint32_t i = 0; i < tx.vShieldedOutput.size(); i++) {
const OutputDescription& outputDesc = tx.vShieldedOutput[i];
const uint256& note_commitment = outputDesc.cm;

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

These two lines could be combined:

const uint256& note_commitment = tx.vShieldedOutput[i].cm;

::AppendNoteCommitment(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize, note_commitment);
}

// If this is our note, witness it

This comment has been minimized.

@str4d

str4d Jul 10, 2018

Contributor

Nit: Now that WitnessNote() has been extracted, this comment doesn't make as much sense (because txIsOurs only says that something in the transaction is ours; we check if it is this note inside WitnessNote()). We should think about rewording it, or possibly renaming WitnessNote() to WitnessNoteIfMine().

This comment has been minimized.

@Eirik0

Eirik0 Jul 12, 2018

Contributor

I had also considered calling this method MaybeWitnessNote and passing in txIsOurs, but I didn't like that, and couldn't come up with something better in two minutes so I left it. If you have something you like I can change it, otherwise I am relatively happy leaving it as is.

This comment has been minimized.

@Eirik0

Eirik0 Jul 13, 2018

Contributor

I actually just went ahead and made the name change.

@str4d str4d added the Sapling label Jul 11, 2018

@@ -333,12 +334,12 @@ double benchmark_increment_note_witnesses(size_t nTxs)
auto note = GetNote(*pzcashParams, sk, wtx, 0, 1);
auto nullifier = note.nullifier(sk);

mapNoteData_t noteData;
mapSproutNoteData_t noteData;

This comment has been minimized.

@arcalinea

arcalinea Jul 12, 2018

Contributor

Are we adding benchmarks for sapling Sapling notes?

This comment has been minimized.

@Eirik0

Eirik0 Jul 12, 2018

Contributor

I think this would be a good idea, but I would prefer to do that as another PR. I created #3395 for this.

@Eirik0 Eirik0 force-pushed the Eirik0:3062-cache-sapling-witnesses branch from 91e727a to bcf1b0e Jul 13, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 13, 2018

Addressed @str4d's comments and force pushed.

@Eirik0 Eirik0 force-pushed the Eirik0:3062-cache-sapling-witnesses branch from bcf1b0e to 5bdd2c7 Jul 13, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 13, 2018

I noticed I missed one comment so I fixed that and force pushed again.

@str4d
Copy link
Contributor

str4d left a comment

Flushing my comments. I have not reviewed the final commit yet.

// This should never fail: we should always be able to get the tree
// state on the path to the tip of our chain
assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, tree));
assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, sproutTree));
// TODO: (when hashFinalSaplingAnchor is added)

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

It's already there - it's in the block header, which pindex contains.

This comment has been minimized.

@Eirik0

Eirik0 Jul 13, 2018

Contributor

Ok I see it now. It was called hashFinalSaplingRoot not hashFinalSaplingAnchor.

}
}
::CopyPreviousWitnesses(wtxItem.second.mapSproutNoteData, pindex->nHeight, nWitnessCacheSize);
::CopyPreviousWitnesses(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize);

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

Verified done 😄

@@ -932,7 +933,8 @@ UniValue AsyncRPCOperation_sendmany::perform_joinsplit(AsyncJoinSplitInfo & info
uint256 anchor;
{
LOCK(cs_main);
pwalletMain->GetNoteWitnesses(outPoints, witnesses, anchor);
pwalletMain->GetSproutNoteWitnesses(outPoints, witnesses, anchor);
// TODO: Sapling witnesses

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

Sapling witnesses won't be fetched here, as Sapling spends are handled in a very different way to Sprout JoinSplits.

This comment has been minimized.

@Eirik0

Eirik0 Jul 13, 2018

Contributor

I assume that this pertains to asyncoperation_mergetoaddress.cpp also. Based on that and this comment I am removing all TODOs I have added to those two files in this PR.

@@ -92,18 +102,33 @@ JSOutPoint CreateValidBlock(TestWallet& wallet,
auto wtx = GetValidReceive(sk, 50, true);
auto note = GetNote(sk, wtx, 0, 1);
auto nullifier = note.nullifier(sk);

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

Whitespace.

// Shielded Output
OutputDescription od;
uint256 cm;
od.cm = cm;

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

This does nothing: both od.cm and cm will initialize to the all-zeroes byte array.

@Eirik0 Eirik0 force-pushed the Eirik0:3062-cache-sapling-witnesses branch from 5bdd2c7 to 331c887 Jul 13, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 13, 2018

Force pushed to address the last set of comments.

I should note that there appear to be some performance issues with this PR, most noticeably with
wallet_tests.CachedWitnessesCleanIndex which I am still looking in to.

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

@daira

daira approved these changes Jul 17, 2018

Copy link
Contributor

daira left a comment

Excellent PR! Was very easy to review because of the careful separation of commits. ut(ACK+cov) with comments.

(Sigh, a bunch of my comments are showing as outdated because they were on individual commits for code touched by a later commit. They're still valid.)

for (uint8_t j = 0; j < jsdesc.commitments.size(); j++) {
const uint256& note_commitment = jsdesc.commitments[j];
tree.append(note_commitment);
}

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

(No action required) I wish diff would be smarter about changes like this by default :-/

If you select "Hide whitespace changes", it gets it right though.

assert(nWitnessCacheSize >= nd->witnesses.size());
// Subtract 1 to compare to what nWitnessCacheSize will be after
// decrementing.
assert((nWitnessCacheSize - 1) >= nd->witnesses.size());

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

(No action required.) I like this change. Now, if the assertion fails we'll have only just executed the code that caused it to fail.

COutPoint() : BaseOutPoint() {};
COutPoint(uint256 hashIn, uint32_t nIn) : BaseOutPoint(hashIn, nIn) {};
std::string ToString() const;
};

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

Add a class description here, saying that the index is that of a Sapling Output description.

{
public:
std::list<ZCIncrementalWitness> witnesses;
int witnessHeight;

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

Do the comments on the corresponding members of SproutNoteData also apply here? (It isn't obvious, and things like the -1 placeholder are pretty subtle.)

}
for (auto& item : noteDataMap) {
auto* nd = &(item.second);
// Only increment witnesses that are not above the current height

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

This should say "Only decrement witnesses..." (The error was in the original comment, probably a cut-and-paste error from IncrementNoteWitnesses.)

// Subtract 1 to compare to what nWitnessCacheSize will be after
// decrementing.
assert((nWitnessCacheSize - 1) >= nd->witnesses.size());
// See comment below (this would be invalid if there was a

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

Grammar nitpick: s/was/were/

ZCIncrementalMerkleTree newSproutTree;
ZCSaplingIncrementalMerkleTree newSaplingTree;
assert(pcoinsTip->GetSproutAnchorAt(pcoinsTip->GetBestAnchor(SPROUT), newSproutTree));
assert(pcoinsTip->GetSaplingAnchorAt(pcoinsTip->GetBestAnchor(SAPLING), newSaplingTree));

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

It looks like the Sprout and Sapling trees are very often used together. Would it make sense to have a CommitmentTrees type that refers to both (or all, in future) trees?

This comment has been minimized.

@Eirik0

Eirik0 Jul 17, 2018

Contributor

I like that idea. It would be nice to do some refactoring around some of the sprout v sapling logic and I think this would be a step in the right direction.

This comment has been minimized.

@bitcartel

bitcartel Jul 18, 2018

Contributor

@Eirik0 Can you file a Github issue about this refactoring? It can be done later.

This comment has been minimized.

@Eirik0
@@ -46,6 +46,10 @@ CWalletTx GetValidReceive(ZCJoinSplit& params,
inputs, outputs, 2*value, 0, false};
mtx.vjoinsplit.push_back(jsdesc);

// Shielded Output
OutputDescription od;
mtx.vShieldedOutput.push_back(od);

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

This adds a shielded output but the transaction version is 2. It won't be valid unless the caller changes the version to 4 (and it won't allow testing pre-Sapling txns when that is intended).

This comment has been minimized.

@Eirik0

Eirik0 Jul 17, 2018

Contributor

I am going to push an additional a commit adding a parameter for the version.

witnesses.clear();
uint256 anchor;
wallet.GetSproutNoteWitnesses(notes, witnesses, anchor);

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

Why is there not another call to GetWitnessesAndAnchors here?

This comment has been minimized.

@Eirik0

Eirik0 Jul 17, 2018

Contributor

This is a mistake. Thanks for catching this!

@@ -1419,6 +1419,33 @@ void CWallet::GetSproutNoteWitnesses(std::vector<JSOutPoint> notes,
}
}

void CWallet::GetSaplingNoteWitnesses(std::vector<SaplingOutPoint> notes,
std::vector<boost::optional<ZCSaplingIncrementalWitness>>& witnesses,
uint256 &final_anchor)

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

Nit: fix the indentation here and for GetSproutNoteWitnesses.

@Eirik0 Eirik0 force-pushed the Eirik0:3062-cache-sapling-witnesses branch from 331c887 to 84d91f0 Jul 17, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 17, 2018

Thanks @daira for the thorough review! I've made updates based on your comments and forced pushed them.

// This should never fail: we should always be able to get the tree
// state on the path to the tip of our chain
assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, tree));
assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, sproutTree));
if (pindex->pprev) {

This comment has been minimized.

@bitcartel

bitcartel Jul 18, 2018

Contributor

@Eirik0 Double-checking that we need this conditional if (pindex->pprev) ?

This comment has been minimized.

@Eirik0

Eirik0 Jul 18, 2018

Contributor

This was added based on a comment from @str4d:
#3353 (comment)
I would like to defer to him for the answer to that question.

This comment has been minimized.

@bitcartel

bitcartel Jul 22, 2018

Contributor

@str4d ^ ... also, could you re-review with a focus on changes to state on disk? I think it's fine but would benefit from another look.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 18, 2018

@Eirik0 General question: is any state on disk changed because of this PR? From comment above, #3353 (comment), I can see that CWalletTx does not serialize mapSaplingNoteData to the wallet.dat file.

I see there is #3388 for persisting Sapling data to disk, so afaict, this PR should not impact current users. That is, users should be able to build from this branch, run on mainnet, go back to v1.1.2, and their wallet should still only contain Sprout related data.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 19, 2018

@bitcartel As far as I know these changes do not affect the state on the disk. As mentioned the serialization has not been updated and I think that is sufficient to confirm this.

@ravibhojwani

This comment has been minimized.

Copy link

ravibhojwani commented Jul 23, 2018

If I had an old install running a node but not mining and it had helped the network would it still be helping the network?

@ebfull

ebfull approved these changes Jul 23, 2018

@bitcartel
Copy link
Contributor

bitcartel left a comment

Nack for now. When running using a mainnet node's .zcash folder, which had been synced from a few days ago using v1.1.2, assertion was triggered:

zcashd: wallet/wallet.cpp:1912: int CWallet::ScanForWalletTransactions(CBlockIndex*, bool): Assertion `pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, saplingTree)' failed.

EDIT:
From debug.log:

2018-07-24 19:09:13 init message: Loading wallet...
2018-07-24 19:09:14 nFileVersion = 1010250
2018-07-24 19:09:14 Keys: 114 plaintext, 0 encrypted, 114 w/ metadata, 114 total
2018-07-24 19:09:14 ZKeys: 3 plaintext, 0 encrypted, 3 w/metadata, 3 total
2018-07-24 19:09:14  wallet                  744ms
2018-07-24 19:09:14 init message: Rescanning...
2018-07-24 19:09:14 Rescanning last 1544 blocks (from block 359904)...

EDIT:
Added more logging:

2018-07-24 21:26:43 init message: Rescanning...
2018-07-24 21:26:43 Rescanning last 1544 blocks (from block 359904)...
2018-07-24 21:26:43 nHeight=359903, hashFinalSaplingRoot=0000000000000000000000000000000000000000000000000000000000000000

I also did manage to reproduce on testnet too, where originally I had no problem and it had been synced past sapling activation, but after I started a reindex, and I stopped shortly after, I launched again and got the error.

2018-07-24 21:29:20 init message: Rescanning...
2018-07-24 21:29:20 Rescanning last 3782 blocks (from block 0)...
2018-07-24 21:29:20 nHeight=0, hashFinalSaplingRoot=0000000000000000000000000000000000000000000000000000000000000000

Since sapling empty root is 3e49b5f954aa9d3545bc6c37744661eea48d7c34e3000d82b7f0010c30f4c2fb, when calling GetSaplingAnchorAt() with 0x00... false is returned and the assert is triggered.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 24, 2018

@bitcartel The assertion you mention was something I wasn't sure about. Does it make sense to leave it as a TODO for now? I am curious if @str4d has further insight.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jul 25, 2018

assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, saplingTree));

This assertion is using hashFinalSaplingRoot (the field in the header) before it's constrained to equal the correct content. Here's some consensus code that does the right thing:

if (NetworkUpgradeActive(pindex->pprev->nHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) {
        view.PopAnchor(pindex->pprev->hashFinalSaplingRoot, SAPLING);
    } else {
        view.PopAnchor(ZCSaplingIncrementalMerkleTree::empty_root(), SAPLING);
}

So, the assertion should be changed so that it only applies when NetworkUpgradeActive(pindex->pprev->nHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING).

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 25, 2018

I have pushed a fix which has been verified against mainnet and am now testing against testnet. Once the test passes I will ack and merge.

@bitcartel bitcartel force-pushed the Eirik0:3062-cache-sapling-witnesses branch from d928a57 to 2f0b2a2 Jul 26, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 26, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 26, 2018

📌 Commit 2f0b2a2 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 26, 2018

⌛️ Testing commit 2f0b2a2 with merge c1ebe7e...

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

Auto merge of #3353 - Eirik0:3062-cache-sapling-witnesses, r=bitcartel
Cache Sapling witnesses in the wallet

Closes #3062

I have not update the tests in test_wallet.cpp. Also, there are several other methods in the wallet that have to do with witnesses and note data which will need to be updated, but this PR focuses on IncrementNoteWitnesses and DecrementNoteWitnesses.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 26, 2018

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 26, 2018

Failed on debian 9, macOS, centos, archlinux CI builders.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 26, 2018

⌛️ Testing commit 2f0b2a2 with merge 3eefe12...

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

Auto merge of #3353 - Eirik0:3062-cache-sapling-witnesses, r=bitcartel
Cache Sapling witnesses in the wallet

Closes #3062

I have not update the tests in test_wallet.cpp. Also, there are several other methods in the wallet that have to do with witnesses and note data which will need to be updated, but this PR focuses on IncrementNoteWitnesses and DecrementNoteWitnesses.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 26, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 3eefe12 to master...

@zkbot zkbot merged commit 2f0b2a2 into zcash:master Jul 26, 2018

1 check passed

homu Test successful
Details

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

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