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

Track Sapling notes and nullifiers in the wallet (in-memory only, no persistence to disk) #3422

Merged
merged 40 commits into from Aug 18, 2018

Conversation

@bitcartel
Copy link
Contributor

bitcartel commented Jul 26, 2018

Part of #3061. Add in-memory tracking of Sapling notes and nullifiers to the wallet.

@@ -61,8 +61,8 @@ class TestWallet : public CWallet {
void SetBestChain(MockWalletDB& walletdb, const CBlockLocator& loc) {
CWallet::SetBestChainINTERNAL(walletdb, loc);
}
bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx) {
return CWallet::UpdatedNoteData(wtxIn, wtx);
bool UpdateSproutNoteData(const CWalletTx& wtxIn, CWalletTx& wtx) {

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

s/UpdateSproutNoteData/UpdatedSproutNoteData/ (and ditto below), or fix the comment.

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

Also, I'm inclined to say just leave the function named as-is, and handle both Sprout and Sapling inside it. But maybe it ends up cleaner to have two separate functions...

This comment has been minimized.

@bitcartel

bitcartel Jul 27, 2018

Contributor

I'm dropping the commit; will just have one fn.

@@ -741,6 +742,11 @@ void CWallet::AddToSpends(const uint256& wtxid)
AddToSpends(nullifier, wtxid);
}
}
// TODO: do we want Sapling nullifiers to be stored with Sprout nullifiers?

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

They would be stored in SaplingNoteData, along with the cached SaplingIncomingViewingKey (analogous to the nullifier and address stored in SproutNoteData).

We definitely want to store ivk, so that we don't have to re-trial-decrypt. We may want to cache the diversifier (so we can easily derive the payment address), and we may want to cache the nullifier (so we don't have to fetch fvk and re-derive it). However, we probably don't need to store the diversifier or nullifier in the on-disk format (i.e. just cache in memory), as both can be calculated on startup (as fvk will never be encrypted in our wallet's threat model, and the diversifier is in the ciphertext already stored inside the CWalletTx by way of it being a subclass of CTransaction).

// TODO: do we want Sapling nullifiers to be stored with Sprout nullifiers?
// Sapling spends
for (const SpendDescription &spend : tx.vShieldedSpend) {
AddToSpends(spend.nullifier, wtxid);

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

This needs to be AddToSaplingSpends(), and we need a mapTxSaplingNullifiers it stores them in (because Sprout and Sapling nullifiers are in different domains).

@@ -1180,6 +1190,10 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
CWalletTx& wtx = (*ret.first).second;
wtx.BindWallet(this);
UpdateSproutNullifierNoteMapWithTx(wtx);

// TODO: Separate function or just use one e.g. UpdateNullifier...

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

Yes, this can just be a single function (removing the commit that renamed to UpdateSproutNullifier...).

// If a transaction changes 'conflicted' state, that changes the balance
// available of the outputs it spends. So force those to be
// recomputed, also:
BOOST_FOREACH(const CTxIn& txin, tx.vin)
BOOST_FOREACH(
const CTxIn &txin, tx.vin)

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

If you're going to introduce line changes in this kind of code, then also migrate it to for (const CTxIn &txin : tx.vin). Or revert the change.

This comment has been minimized.

@bitcartel

bitcartel Jul 28, 2018

Contributor

Accident, will fix.

for (const JSDescription& jsdesc : tx.vjoinsplit) {
for (const uint256& nullifier : jsdesc.nullifiers) {
for (const JSDescription &jsdesc : tx.vjoinsplit) {
for (const uint256 &nullifier : jsdesc.nullifiers) {

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

Are these code-style changes automated by your IDE? They add a bit of noise to the PR.

This comment has been minimized.

@bitcartel

bitcartel Jul 27, 2018

Contributor

I'm experimenting with CLion at the moment, probably somewhere in the settings.

// TODO: Do we want this?
for (const OutputDescription &output : tx.vShieldedOutput) {
// TODO: compute nullifier and check against...
uint256 nullifier; // TODO: compute...

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

No need; iterate over tx.vShieldedSpend, and using the nullifier there. The intent of this function is to find transactions that generated coins or notes being spent by this one, and mark them as dirty.

This comment has been minimized.

@bitcartel

bitcartel Jul 28, 2018

Contributor

Yep, as discussed in meeting this will be updated.

// TODO: we can compute the nullifier if we want to cache it...?
// auto pt = result.get();
// auto note = pt.note(ivk);
// boost::optional<uint256> nullifier(const SaplingSpendingKey &sk, const uint64_t position) const;

This comment has been minimized.

@str4d

str4d Jul 26, 2018

Contributor

See the first commit in #3417, where I alter SaplingNote::nullifier() to take a SaplingFullViewingKey (which is all that is needed). If we decide to not cache nullifiers, then we can punt on doing this here.

This comment has been minimized.

@bitcartel

bitcartel Jul 28, 2018

Contributor

In meeting we decided to not cache nullifiers given that we need the position.

@bitcartel bitcartel force-pushed the bitcartel:3061_track_notes_based_on_3062 branch from dcd4e79 to 2a5e7d6 Jul 28, 2018

@bitcartel bitcartel added this to the v2.0.0 milestone Jul 28, 2018

@bitcartel bitcartel requested review from ebfull and daira Jul 29, 2018

@daira
Copy link
Contributor

daira left a comment

The change to call IsSaplingNullifierFromMe from CWallet::IsFromMe(const CTransaction& tx) is blocking. Other suggested changes are non-blocking.

@@ -350,7 +350,7 @@ TEST(wallet_tests, set_invalid_note_addrs_in_cwallettx) {
EXPECT_THROW(wtx.SetSproutNoteData(noteData), std::logic_error);
}

TEST(wallet_tests, GetNoteNullifier) {
TEST(wallet_tests, GetSproutNoteNullifier) {

This comment has been minimized.

@daira

daira Jul 29, 2018

Contributor

We need corresponding tests for Sapling (or, better, some way to share the test code, but that looks difficult).

@@ -1170,7 +1192,8 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
mapWallet[hash] = wtxIn;
mapWallet[hash].BindWallet(this);
UpdateNullifierNoteMapWithTx(mapWallet[hash]);
AddToSpends(hash);
AddToSproutSpends(hash);
AddToSaplingSpends(hash);

This comment has been minimized.

@daira

daira Jul 29, 2018

Contributor

My suggestion above would eliminate this change and the next one.

{
assert(mapWallet.count(wtxid));
CWalletTx& thisTx = mapWallet[wtxid];
if (thisTx.IsCoinBase()) // Coinbases don't spend anything!
return;

for (const CTxIn& txin : thisTx.vin) {
AddToSpends(txin.prevout, wtxid);
AddToSproutSpends(txin.prevout, wtxid);
}

This comment has been minimized.

@daira

daira Jul 29, 2018

Contributor

It doesn't make sense to me to have transparent and Sprout spends handled in the same function, but Sapling spends handled in a different one.

I suggest:

  • keep this as CWallet::AddToSpends(const uint256& wtxid) and make it handle Sapling spends as well (i.e. merge CWallet::AddToSaplingSpends(const uint256& wtxid) into it);
  • rename CWallet::AddToSproutSpends(const COutPoint& outpoint, const uint256& wtxid) to AddToTransparentSpends.

return noteData;
}

bool CWallet::IsFromMe(const uint256& nullifier) const

This comment has been minimized.

@daira

daira Jul 29, 2018

Contributor

This should be renamed to IsSproutNullifierFromMe or something like that.

This comment has been minimized.

@daira

daira Jul 29, 2018

Contributor

Also, CWallet::IsFromMe(const CTransaction& tx) needs to be updated to call a corresponding IsSaplingNullifierFromMe method.

Incidentally, the locking here looks quite inefficient; ideally we wouldn't be locking/unlocking the wallet once for each nullifier in a transaction in CWallet::IsFromMe(const CTransaction& tx).

void AddToSproutSpends(const uint256& nullifier, const uint256& wtxid);
void AddToSproutSpends(const uint256& wtxid);
void AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid);
void AddToSaplingSpends(const uint256& wtxid);

This comment has been minimized.

@daira

daira Jul 29, 2018

Contributor

Taking my suggestion above, this would become:

    void AddToTransparentSpends(const COutPoint& outpoint, const uint256& wtxid);
    void AddToSproutSpends(const uint256& nullifier, const uint256& wtxid);
    void AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid);
    void AddToSpends(const uint256& wtxid);

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

@bitcartel bitcartel force-pushed the bitcartel:3061_track_notes_based_on_3062 branch from 2a5e7d6 to 470e00a Jul 30, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 30, 2018

Cherry-picked ebfull@1bca1c2

@bitcartel bitcartel force-pushed the bitcartel:3061_track_notes_based_on_3062 branch from 470e00a to fa0040f Aug 1, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 1, 2018

Force pushed update to address review comments so far (apart from tests).

This PR is dependent on the Transaction Builder PR as we use the new position() method on the witness.

In pairing with @daira we determined that UpdateSaplingNullifierNoteMapWithTx requires a new librustzcash method to compute the nullifier. Passing over to @ebfull for next update.

@bitcartel bitcartel force-pushed the bitcartel:3061_track_notes_based_on_3062 branch from fa0040f to 831da21 Aug 1, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 1, 2018

Force-pushed update based on meeting discussion; now caches Sapling note nullifiers.

@bitcartel bitcartel force-pushed the bitcartel:3061_track_notes_based_on_3062 branch from 831da21 to a97e9e7 Aug 3, 2018

@str4d
Copy link
Contributor

str4d left a comment

Up to 503ec52

libzcash::SaplingIncomingViewingKey ivk;

friend bool operator==(const SaplingNoteData& a, const SaplingNoteData& b) {
return (a.ivk == b.ivk);

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

I think we should at least compare witnessHeight as well, because I don't think we want note data from different heights to be considered equal. What necessitates that equality is defined?

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

We use std::pair with SaplingNoteData and it needs equality defined. I've pushed a commit to also compare witnessHeight.

AddToSpends(hash);
AddToSaplingSpends(hash);
>>>>>>> e9b1ce0... fix

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Crept into f4c99ec (I assume it disappears in a later commit).

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

Gremlins.

}
}

void CWallet::AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid)

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Nit: put this above AddToSpends(), below the other inner functions.

AddToSpends(hash);
AddToSaplingSpends(hash);
>>>>>>> e9b1ce0... fix

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Ah, disappears here.

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

Yep, gremlins.


void AddToTransparentSpends(const COutPoint& outpoint, const uint256& wtxid);
void AddToSproutSpends(const uint256& nullifier, const uint256& wtxid);
void AddToSpends(const uint256& wtxid);
void AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid);

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Nit: Move this up one line.

// Protocol Spec: 4.19 Block Chain Scanning (Sapling)
for (uint32_t i = 0; i < tx.vShieldedOutput.size(); ++i) {
const OutputDescription output = tx.vShieldedOutput[i];
for (auto it = mapSaplingIncomingViewingKeys.begin(); it != mapSaplingIncomingViewingKeys.end(); ++it) {

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

mapSaplingIncomingViewingKeys will contain the same ivk many times on the "value" side once we start supporting diversified addresses. We should iterate over the "key" side of mapSaplingFullViewingKeys, where we are guaranteed to have a 1:1 mapping.

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

Pushed commit for this.

CAmount nValueOut = GetValueOut(); // transparent outputs plus all vpub_old
CAmount nValueIn = 0;
nValueIn += GetShieldedValueIn();
CAmount nValueOut = GetValueOut(); // transparent outputs plus all Sprout vpub_old and Sapling valueBalance

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

and negative Sapling valueBalance.

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

Pushed commit for this.

} else if (valueBalance > 0) {
COutputEntry output = {CNoDestination(), valueBalance, (int)vout.size()};
listReceived.push_back(output);
}

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Should this be inside the if (isFromMyTaddr) { scope, rather than outside? Needs checking.

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

I agree it should be inside, like the two blocks above it. Pushed commit for this.

@str4d
Copy link
Contributor

str4d left a comment

Finished reviewing the current contents of the PR (up to 0eb8d34).

SaplingOutPoint op = item.first;
SaplingNoteData nd = item.second;

if (nd.witnesses.size() == 0) {

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Nit: if (nd.witnesses.empty()) {

}
item.second.nullifier = boost::none;
}
else if (nd.witnesses.size() > 0) {

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

} else { should be sufficient, as size() returns an unsigned integer.

} else {
DecrementNoteWitnesses(pindex);
UpdateSaplingNullifierNoteMapForBlock(pblock);

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Nit: If the function isn't being specialised in each case, it could just be called once after the conditionals.

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

Currently not specialised so I can clean this up.

auto optPlaintext = SaplingNotePlaintext::decrypt(output.encCiphertext, nd.ivk, output.ephemeralKey, output.cm);
if (!optPlaintext) {
// An item in mapSaplingNoteData must have already been successfully decrypted,
// otherwise the item would not exist in the first place.

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

This is currently true because we don't provide a way to delete a key from a wallet. I doubt we ever will, however, so this seems like a safe invariant.

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

Prefer, e.g., assert(!!optPlaintext); to if (!optPlaintext) { assert(false); }, because the implementation of assert usually includes the text of the condition in the abort message.

}
}

range = mapTxSaplingNullifiers.equal_range(nullifier);

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

We need separate functions for checking Sprout and Sapling nullifiers, because they are in separate domains and aren't guaranteed to be collision-resistant (otherwise there is a possibility of a nullifier collision, however remote, between Sprout and Sapling causing the spend of one to prevent the spend of the other).

Given how close to identical they would be, could implement this as a template (like was done for e.g. the anchor and tree handling code in src/coins.h IIRC).

This comment has been minimized.

@bitcartel

bitcartel Aug 3, 2018

Contributor

Good point. No to templates! Pushed commit for this.

@bitcartel bitcartel self-assigned this Aug 3, 2018

@bitcartel bitcartel changed the title [WIP] 3061 track notes based on 3062 Track Sapling notes and nullifiers in the wallet (in-memory only, no persistence to disk) Aug 3, 2018

@daira daira dismissed their stale review Aug 3, 2018

starting re-review

@daira
Copy link
Contributor

daira left a comment

Lots still to do here, especially in bringing the Sapling test coverage up.

@@ -35,6 +35,11 @@ TEST(noteencryption, NotePlaintext)
}

SaplingNote note(addr, 39393);
auto cmu_opt = note.cm();

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

This could be a separate PR, but I think the cm() method should be renamed to cmu().

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

Similarly, the cm member of OutputDescription should be renamed to cmu (also change the comment to The u-coordinate of ...).

uint256 cm; //!< The note commitment for the output note.

This comment has been minimized.

@bitcartel

bitcartel Aug 4, 2018

Contributor

Can fix later. There is a ticket for this. #3446

}

TEST(wallet_tests, spent_note_is_from_me) {
TEST(WalletTests, SpentNoteIsFromMe) {

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

SpentSproutNoteIsFromMe, and add a Sapling test.

// Get temporary and unique path for file.
boost::filesystem::path pathTemp = boost::filesystem::temp_directory_path() / boost::filesystem::unique_path();
boost::filesystem::create_directories(pathTemp);
mapArgs["-datadir"] = pathTemp.string();
}

TEST(wallet_tests, note_data_serialisation) {
TEST(WalletTests, NoteDataSerialisation) {

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

SproutNoteDataSerialization. Also add a similar test for Sapling.

This comment has been minimized.

@bitcartel

bitcartel Aug 7, 2018

Contributor

The test will be added as part of #3388

@@ -162,7 +163,7 @@ TEST(wallet_tests, note_data_serialisation) {
}


TEST(wallet_tests, find_unspent_notes) {
TEST(WalletTests, FindUnspentNotes) {

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

FindUnspentSproutNotes. Also add a similar test for Sapling.

This comment has been minimized.

@bitcartel

bitcartel Aug 7, 2018

Contributor

Test should be added in dependent PR #3436 which updates GetFilteredNotes for Sapling.

// Get temporary and unique path for file.
boost::filesystem::path pathTemp = boost::filesystem::temp_directory_path() / boost::filesystem::unique_path();
boost::filesystem::create_directories(pathTemp);
mapArgs["-datadir"] = pathTemp.string();
}

TEST(wallet_tests, note_data_serialisation) {
TEST(WalletTests, NoteDataSerialisation) {
auto sk = libzcash::SproutSpendingKey::random();
auto wtx = GetValidReceive(sk, 10, true);
auto note = GetNote(sk, wtx, 0, 1);

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

Do GetValidReceive, GetValidSpend, and GetNote need to be renamed?

This comment has been minimized.

@str4d

str4d Aug 4, 2018

Contributor

Yes. Does it need to happen in this PR though?

This comment has been minimized.

@bitcartel

bitcartel Aug 7, 2018

Contributor

We can do this in another PR to make life easier for dependent PRs that need to rebase on this PR.

@@ -1807,6 +1973,18 @@ void CWalletTx::GetAmounts(list<COutputEntry>& listReceived,
}
}

// If we sent utxos from this transaction, create output for value taken from (negative valueBalance)
// or added (positive valueBalance) to the transparent value pool by Sapling shielding and unshielding.
if (isFromMyTaddr) {

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

This could be merged with the if block above.

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

On the other hand, I don't understand why it is correct to make this conditional on having sent from our t-addr, rather than from any of our z- or t- addresses.

This comment has been minimized.

@bitcartel

bitcartel Aug 7, 2018

Contributor

"// GetAmounts will determine the transparent debits and credits for a given wallet tx." , and this function is called from transparent related RPCs, such as getbalance so all this function needs to know is the net amount of value moving between shielded and transparent. It doesn't need to know about sending from z addresses. For that, we have z_gettotalbalance.

@@ -3930,7 +4108,7 @@ void CWallet::GetFilteredNotes(
}

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

Either this method and GetUnspentFilteredNotes need to be updated to also find Sapling notes, or these methods need to be renamed and new ones added for Sapling. It seems like the latter is probably simpler, given the dependence on Sprout types in the parameters (and then callers will need to be updated to call both).

This comment has been minimized.

@str4d

str4d Aug 4, 2018

Contributor

I have already done the former in #3436, so leave this out of this PR.

@@ -1027,6 +1044,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
void MarkDirty();
bool UpdateNullifierNoteMap();
void UpdateNullifierNoteMapWithTx(const CWalletTx& wtx);
void UpdateSaplingNullifierNoteMapWithTx(CWalletTx& tx);

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

Change the parameter name to wtx.

}

uint256 cmu_expected;
if (!librustzcash_sapling_compute_cm(

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

This doesn't compute cm, it computes cmu. I know renaming it is awkward but please file a ticket to do that.

This comment has been minimized.

@bitcartel
))
{
return boost::none;
}

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

It was hard to determine whether this is correctly checking that rcm is in range, as the spec requires. (The code is distributed between librustzcash and Fs::is_valid in sapling-crypto.) It is correct, but more comments might have helped.

@str4d str4d moved this from In Progress to In Review in Zcashd Team Aug 4, 2018

@zcash zcash deleted a comment from daira Aug 6, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Aug 6, 2018

@daira I accidentally deleted one of your comments... The question was to do with updating zcbenchmarks for sapling, and I was trying to respond by saying that #3395 has been created to address that.

wtx.mapSproutNoteData = tmp;
return true;

return !unchangedSproutFlag || !unchangedSaplingFlag;

This comment has been minimized.

@Eirik0

Eirik0 Aug 7, 2018

Contributor

This could be written as a template method:

template<typename OutPoint, typename NoteData>
bool UpdateNoteData(const std::map<OutPoint, NoteData>& wtxInMap, std::map<OutPoint, NoteData>& wtxMap)
{
    if (wtxInMap.empty() || wtxInMap == wtxMap) {
        return false;
    }
    auto tmp = wtxInMap;
    // Ensure we keep any cached witnesses we may already have
    for (const std::pair <OutPoint, NoteData> nd : wtxMap) {
        if (tmp.count(nd.first) && nd.second.witnesses.size() > 0) {
            tmp.at(nd.first).witnesses.assign(
                    nd.second.witnesses.cbegin(), nd.second.witnesses.cend());
        }
        tmp.at(nd.first).witnessHeight = nd.second.witnessHeight;
    }
    // Now copy over the updated note data
    wtxMap = tmp;
    return true;
}

bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx)
{
    bool changedSprout = ::UpdateNoteData(wtxIn.mapSproutNoteData, wtx.mapSproutNoteData);
    bool changedSapling = ::UpdateNoteData(wtxIn.mapSaplingNoteData, wtx.mapSaplingNoteData);
    return changedSprout || changedSapling;
}

I prefer this because it prevents future divergence and puts one step closer to an abstraction that encapsulates logic such as this.

This comment has been minimized.

@bitcartel

bitcartel Aug 8, 2018

Contributor

Can be done in a future PR.

@@ -204,14 +205,36 @@ boost::optional<SaplingNotePlaintext> SaplingNotePlaintext::decrypt(

assert(ss.size() == 0);

This comment has been minimized.

@Eirik0

Eirik0 Aug 7, 2018

Contributor

Here we assert before calling librustzcash_sapling_compute_cm. See comment below.

@bitcartel bitcartel force-pushed the bitcartel:3061_track_notes_based_on_3062 branch from 5443c43 to 303f80f Aug 8, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 8, 2018

Review ACK from @str4d "Cannot ACK from here, but ACK. r+ able"

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 8, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 8, 2018

⌛️ Trying commit 303f80f with merge f841ceb...

zkbot added a commit that referenced this pull request Aug 8, 2018

Auto merge of #3422 - bitcartel:3061_track_notes_based_on_3062, r=<try>
Track Sapling notes and nullifiers in the wallet (in-memory only, no persistence to disk)

Closes #3061.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 8, 2018

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

@daira

daira approved these changes Aug 9, 2018

Copy link
Contributor

daira left a comment

Rereviewed d7cf640 to 303f80f inclusive. ut(ACK+cov)

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 15, 2018

@str4d Can you dismiss and update your review state? Thanks.

Based on "Cannot ACK from here, but ACK. r+ able"

@str4d str4d modified the milestones: v2.0.0, v2.0.1 Aug 15, 2018

@str4d

str4d approved these changes Aug 17, 2018

Copy link
Contributor

str4d left a comment

ACK+cov. Comment can be addressed later.

ASSERT_EQ(static_cast<bool>(maybe_tx), true);
auto tx = maybe_tx.get();

// No Sapling notes can be found in tx which belong to the wallet

This comment has been minimized.

@str4d

str4d Aug 17, 2018

Contributor

which does not belong

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 17, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 17, 2018

📌 Commit c9339bb has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 17, 2018

⌛️ Testing commit c9339bb with merge 7c4e406...

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

Auto merge of #3422 - bitcartel:3061_track_notes_based_on_3062, r=str4d
Track Sapling notes and nullifiers in the wallet (in-memory only, no persistence to disk)

Part of #3061.  Add in-memory tracking of Sapling notes and nullifiers to the wallet.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 17, 2018

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 17, 2018

Merge failed due to missing dependency:

Fetching crate_sapling_crypto...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (22) The requested URL returned error: 404 Not Found
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404 Not Found
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 17, 2018

Yes the crate is missing from our depends cache (which we should update now that 2.0.0 is out), but the backup GitHub URL should have worked. Looks like a transient failure to reach GitHub.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 17, 2018

⌛️ Testing commit c9339bb with merge 0d91570...

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

Auto merge of #3422 - bitcartel:3061_track_notes_based_on_3062, r=str4d
Track Sapling notes and nullifiers in the wallet (in-memory only, no persistence to disk)

Part of #3061.  Add in-memory tracking of Sapling notes and nullifiers to the wallet.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 17, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 17, 2018

Not sure why GitHub URL is still failing. I've uploaded the dependencies to our cache, so the primary URL should succeed now.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 17, 2018

⌛️ Testing commit c9339bb with merge 20f87bc...

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

Auto merge of #3422 - bitcartel:3061_track_notes_based_on_3062, r=str4d
Track Sapling notes and nullifiers in the wallet (in-memory only, no persistence to disk)

Part of #3061.  Add in-memory tracking of Sapling notes and nullifiers to the wallet.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 18, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 20f87bc to master...

@zkbot zkbot merged commit c9339bb into zcash:master Aug 18, 2018

1 check passed

homu Test successful
Details

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

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