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 support for z_listreceivedbyaddress #3499

Merged
merged 1 commit into from Sep 28, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented Aug 30, 2018

Closes #3379.

std::vector<CSproutNotePlaintextEntry> sproutEntries;
std::vector<SaplingNoteEntry> saplingEntries;
pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, fromaddress, nMinDepth, false, false);
auto nullifierSet = hasSproutSpendingKey ? pwalletMain->GetNullifiersForAddresses({zaddr}) : std::set<std::pair<PaymentAddress, uint256>>();

auto nullifierSet = hasSpendingKey ? pwalletMain->GetNullifiersForAddresses({zaddr}) : std::set<std::pair<PaymentAddress, uint256>>();

This comment has been minimized.

@arcalinea

arcalinea Aug 30, 2018

Contributor

GetNullifiersForAddresses: This function should support both types of payment addresses, but looks like it's currently going over only mapSproutNoteData

@bitcartel bitcartel added the Sapling label Sep 4, 2018

@bitcartel bitcartel added this to the v2.0.1 milestone Sep 4, 2018

if (!pwalletMain->HaveSproutSpendingKey(zaddr)) {
return false;
}
return true;

This comment has been minimized.

@LarryRuane

LarryRuane Sep 6, 2018

Contributor

suggestion: a more concise way to code the previous 4 lines would be:

return pwalletMain->HaveSproutSpendingKey(zaddr);
!pwalletMain->HaveSaplingSpendingKey(fvk)) {
return false;
}
return true;

This comment has been minimized.

@LarryRuane

LarryRuane Sep 6, 2018

Contributor

likewise here, you could replace the above if with:

return pwalletMain->GetSaplingIncomingViewingKey(zaddr, ivk) &&
             pwalletMain->GetSaplingFullViewingKey(ivk, fvk) &&
             pwalletMain->HaveSaplingSpendingKey(fvk));

@bitcartel bitcartel added this to In Progress in Zcashd Team Sep 11, 2018

@bitcartel bitcartel force-pushed the arcalinea:3379_z_listreceivedbyaddress branch from 913d64e to 4457540 Sep 11, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 11, 2018

Forced push a rebase and fix for build error.

@bitcartel
Copy link
Contributor

bitcartel left a comment

I can list the notes received at sapling addresses, but the change flag is always false in my testing.

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 12, 2018

@bitcartel I have reproduced the change false bug (manually, but I'll write an automated rpc test as part of the fix).

@Eirik0 Eirik0 self-requested a review Sep 18, 2018

auto nullifierSet = hasSproutSpendingKey ? pwalletMain->GetNullifiersForAddresses({zaddr}) : std::set<std::pair<PaymentAddress, uint256>>();
for (CSproutNotePlaintextEntry & entry : sproutEntries) {

if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr) {

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

Since we are only passing in one address to pwalletMain->GetFilteredNotes I expect that that method will either have populated sproutEntries or saplingEntries. Even if not, we should not have to call boost::get<libzcash::SproutPaymentAddress>(&zaddr) because the contents of the loop do not depend on whether or not this is the case.

@@ -542,6 +559,25 @@ bool CWallet::IsNoteChange(const std::set<std::pair<libzcash::PaymentAddress, ui
return false;
}

bool CWallet::IsNoteSaplingChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const SaplingOutPoint & op)

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

If we changed CWallet::IsNoteChange to take in a collection of nullifiers (populated from either mapWallet[jsop.hash].vjoinsplit or mapWallet[op.hash].vShieldedSpend) we would not need separate sprout/sapling versions of this method.

if (hasSproutSpendingKey) {
obj.push_back(Pair("change", pwalletMain->IsNoteChange(nullifierSet, entry.address, entry.jsop)));
if (hasSpendingKey) {
auto nullifierSet = pwalletMain->GetNullifiersForAddresses({zaddr});

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

This can be pulled out of the for loop as it is the same for sprout/sapling.

obj.push_back(Pair("txid", entry.op.hash.ToString()));
obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.note.value()))));
obj.push_back(Pair("memo", HexStr(entry.memo)));
obj.push_back(Pair("outindex", (int)entry.op.n));

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

There's almost enough shared between the JSON generated for sprout/sapling to justify extracting a method. Shrug

std::map<libzcash::SaplingIncomingViewingKey, libzcash::SaplingPaymentAddress> ivkMap;
for (const auto & addr : addresses) {
if (boost::get<libzcash::SaplingPaymentAddress>(&addr) != nullptr) {
auto saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(addr);

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

The indentation within this for loop is not the standard.

// Sapling ivk -> addr map
std::map<libzcash::SaplingIncomingViewingKey, libzcash::SaplingPaymentAddress> ivkMap;
for (const auto & addr : addresses) {
if (boost::get<libzcash::SaplingPaymentAddress>(&addr) != nullptr) {

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

In many other places as we have been adding sapling support we have been removing calls to boost::get<..>(...) and replacing it with a boost::static_visitor of some sort. If the get is necessary here, we may want to consider adding a visitor to handle getting the nullifiers.

auto saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(addr);
libzcash::SaplingIncomingViewingKey ivk;
this->GetSaplingIncomingViewingKey(saplingAddr, ivk);
ivkMap.insert(std::make_pair(ivk, saplingAddr));

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

I think that one ivk can have multiple sapling addresses. If this is the case then we will end up returning more nullifiers than we need. I think that the way we use this method (to determine if a note is change), it would not be wrong to return all nullifiers for the wallet, just unnecessary. It's worth it to double check this.

@LarryRuane LarryRuane force-pushed the arcalinea:3379_z_listreceivedbyaddress branch 2 times, most recently from d9d844d to 15ceadc Sep 19, 2018

@str4d str4d moved this from In Progress to In Review in Zcashd Team Sep 19, 2018

for (const auto & addr : addresses) {
if (boost::get<libzcash::SaplingPaymentAddress>(&addr) != nullptr) {
auto saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(addr);
auto saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(&addr);

This comment has been minimized.

@LarryRuane

LarryRuane Sep 20, 2018

Contributor

gratuitous change (calling get() only once instead of twice), not for performance but simpler code (although this single trivial change did reduce the size of the object file by 8k bytes!)

this->GetSaplingIncomingViewingKey(saplingAddr, ivk);
ivkMap.insert(std::make_pair(ivk, saplingAddr));
this->GetSaplingIncomingViewingKey(*saplingAddr, ivk);
ivkMap[ivk].push_back(*saplingAddr);

This comment has been minimized.

@LarryRuane

LarryRuane Sep 20, 2018

Contributor

pretty cool how this constructs the empty vector on the first reference to ivkMap[]

// Sapling ivk -> addr map
std::map<libzcash::SaplingIncomingViewingKey, libzcash::SaplingPaymentAddress> ivkMap;
// Sapling ivk -> list of addrs map
std::map<libzcash::SaplingIncomingViewingKey, std::vector<libzcash::SaplingPaymentAddress>> ivkMap;

This comment has been minimized.

@LarryRuane

LarryRuane Sep 20, 2018

Contributor

Eirik said (in a comment that github considers outdated but really isn't) that more than one payment address can be associated with a single incoming viewing key. Sean agreed, although he says this isn't possible yet (but will be in the future). So the "second" of this map is becoming a list of addresses rather than just a single address. This is not tested yet (except in the trivial case of a single address).

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 20, 2018

I squashed a bunch of commits (down to 2 now).
@bitcartel:

I can list the notes received at sapling addresses, but the change flag is always false in my testing.

The change flag should be fixed now (and there's an RPC test). Also, I rebased and cleaned out those unrelated commits.

@arcalinea

This comment has been minimized.

Copy link
Contributor

arcalinea commented Sep 21, 2018

lgtm now, test passes

@bitcartel
Copy link
Contributor

bitcartel left a comment

ACK modulo review comment/nit. I ran this on testnet and confirmed the change flag is working.

assert_equal(new_height, self.nodes[0].getblockcount())

def run_test_release(self, release, expected_memo, height):
print "testing", release

This comment has been minimized.

@bitcartel

bitcartel Sep 21, 2018

Contributor

Remove this print

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 21, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 21, 2018

⌛️ Trying commit 3544a50 with merge 2d9405a...

zkbot added a commit that referenced this pull request Sep 21, 2018

Auto merge of #3499 - arcalinea:3379_z_listreceivedbyaddress, r=<try>
Sapling support for z_listreceivedbyaddress

Sapling support for z_listreceivedbyaddress
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 21, 2018

💔 Test failed - pr-try

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 21, 2018

@arcalinea @LarryRuane CI says "Failing tests: wallet_listreceived.py"

@LarryRuane LarryRuane force-pushed the arcalinea:3379_z_listreceivedbyaddress branch 2 times, most recently from bb91e84 to 69c9257 Sep 21, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 24, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 24, 2018

@LarryRuane: 🔑 Insufficient privileges: and not in try users

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 24, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 24, 2018

⌛️ Trying commit 69c9257 with merge 4512722...

zkbot added a commit that referenced this pull request Sep 24, 2018

Auto merge of #3499 - arcalinea:3379_z_listreceivedbyaddress, r=<try>
Sapling support for z_listreceivedbyaddress

Closes #3379.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 24, 2018

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

@daira
Copy link
Contributor

daira left a comment

Response to the comment about potential duplicate entries in the output needed.

my_memo = 'c0ffee' # stay awake
my_memo = my_memo + '0'*(1024-len(my_memo))

invalid_memo = 'f6' + ('0'*1022)

This comment has been minimized.

@daira

daira Sep 25, 2018

Contributor

This memo isn't "invalid". As stated in section 5.5 of the protocol spec,

A start byte of 0xF6 followed by 511 0x00 bytes means “no memo”. A start byte of 0xF6 followed by anything else, or a start byte of 0xF7 or greater, are reserved for use in future Zcash protocol extensions.

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "From address does not belong to this node, zaddr spending key or viewing key not found.");
}

This comment has been minimized.

@daira

daira Sep 25, 2018

Contributor

nit: remove trailing whitespace (here and in other new code)

Show resolved Hide resolved src/wallet/wallet.cpp
if (nullifier && ivkMap.count(ivk)) {
for (const auto & addr : ivkMap[ivk]) {
nullifierSet.insert(std::make_pair(addr, nullifier.get()));
}

This comment has been minimized.

@daira

daira Sep 25, 2018

Contributor

If there is more than one (equivalent) diversified address for the ivk of this note, then this will add a pair for each of them, even though the note was sent to a specific address. This will result in more than one entry in the output of z_listreceivedbyaddress for a single note. Even though a careful consumer of the output could deduplicate based on the txid and outindex, I think this is problematic and surprising: I believe that consumers will not in practice perform that deduplication. This breaks any simple accounting of how much was sent to the address (or set of addresses where GetNullifiersForAddresses is used elsewhere internally) by summing the amount fields, for example.

This comment has been minimized.

@daira

daira Sep 25, 2018

Contributor

This also means that a user of z_listreceivedbyaddress can't tell which diversified address a payment was received on, despite that being an important design goal of diversified addresses.

This comment has been minimized.

@bitcartel

bitcartel Sep 25, 2018

Contributor

@daira So @LarryRuane and myself have gone over this. For z_listreceivedbyaddress there is no impact because you can only pass in one address, and GetFilteredNotes will only return notes for any given address(es) it is passed. However, there might be an issue with z_listunspent because a user can pass in a list of addresses, where these addresses may all be derived from the same spending key but with a different diversifier. In that situation, GetNullifiersForAddresses would create an ivkMap where ivkMap[ivk] is a vector of the addresses passed in, meaning that the nullifier set returns extra entries which are not correct e.g. (addr1, nullifier1), (addr2, nullifier1), (addr3, nullifier1). However, in this case, z_listunspent only uses the nullifier set to set the change flag, and it will look for (addr1, nullifier1).

Given the above, we don't think we need to make any changes right now for this, and we don't support diversified addresses yet. Does the above satisfy your review comment? We could open an issue about GetNullifiersForAddresses and have review for v2.0.2?

This comment has been minimized.

@daira

daira Sep 27, 2018

Contributor

You're right; for z_listreceivedbyaddress I had missed that the loop at line 3342 is over the saplingEntries found by GetFilteredNotes, not GetNullifiersForAddresses.

I don't understand the argument for z_listunspent but that may just be because I'm tired right now.

This comment has been minimized.

@daira

daira Sep 27, 2018

Contributor

In any case, please add a comment to GetNullifiersForAddresses explaining the current behaviour and why it is ok for now. That will be sufficient to address my review.

This comment has been minimized.

@LarryRuane

LarryRuane Sep 27, 2018

Contributor

@daira, I'm sorry, I'm not seeing what comment to make in GetNullifiersForAddresses(). Let me try to go through this with an example. Suppose GetNullifiersForAddresses() is called with 4 addresses, za1, za2, za7, za8. Let's say four addresses, za1-za4 are diversified from the same spending key, so they have the same ivk, which has value ivk_1234. So the first loop will create:

    ivkMap[ivk_1234]: (za1, za2)
    ivkMap[ivk_7]:    (za7)
    ivkMap[ivk_8]:    (za8)

The third (last) loop iterates all the notes in the wallet, each of which has a nullifier and an ivk:

    if (nullifier && ivkMap.count(ivk)) {
        for (const auto & addr : ivkMap[ivk]) {
            nullifierSet.insert(std::make_pair(addr, nullifier.get()));
        }
    }

After this loops runs, nullifierSet will contain four entries, za1, za2, za7, za8, the first two of which will have the same nullifier. If and when this loop encounters notes for za3 and za4, they will not be added to nullifierSet, even though they have the same nullifier as za1 and za2, because ivkMap[] doesn't contain za3 and za4. Am I missing something?

This comment has been minimized.

@daira

daira Sep 28, 2018

Contributor

What needs to be documented is that this function does not consider the specific address that each note was sent to; it just finds nullifiers associated with equivalent addresses to the ones passed in. That might be correct –it depends on how GetNullifiersForAddresses is used– but it certainly isn't obvious.

This comment has been minimized.

@daira

daira Sep 28, 2018

Contributor

In any case, since this PR is merged, let's just track this as part of #3547.

@LarryRuane LarryRuane force-pushed the arcalinea:3379_z_listreceivedbyaddress branch from 69c9257 to ce8cfad Sep 25, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 25, 2018

Force-pushed (squashed) changes @daira requested except a fix for multiple output entries for a single note (see @bitcartel's comment/question above).

@daira

daira approved these changes Sep 27, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov) modulo a minor comment improvement (s/0.4/0.4-fee/) and addition of the comment about diversified address behaviour in GetNullifiersForAddresses.


assert_equal(txid, r[0]['txid'])
assert_equal(Decimal('0.4')-fee, r[0]['amount'])
assert_true(r[0]['change'], "Note valued at 0.4 should be change")

This comment has been minimized.

@daira

daira Sep 27, 2018

Contributor

s/0.4/0.4-fee/

This comment has been minimized.

@bitcartel

bitcartel Sep 27, 2018

Contributor

@LarryRuane ^ "Note valued at 0.4-fee should be change"

assert_true(r[0]['change'], "Note valued at 0.4 should be change")
assert_equal(expected_memo, r[0]['memo'])

# The old note still exists (it's immutable), even though it is spent

This comment has been minimized.

@daira

daira Sep 27, 2018

Contributor

Hmm, should the z_listreceivedbyaddress output have a field for each note indicating whether it is known to have been spent? Maybe file a ticket for that.

This comment has been minimized.

This comment has been minimized.

@daira

daira Sep 28, 2018

Contributor

I meant a ticket; project cards don't support comments and are too easy to miss (I barely look at projects except in planning meetings).

@Eirik0 Eirik0 referenced this pull request Sep 27, 2018

Open

Look for TODOs and FIXME in code. #3546

20 of 25 tasks complete

@LarryRuane LarryRuane force-pushed the arcalinea:3379_z_listreceivedbyaddress branch from 86dc9df to e4f0d6a Sep 27, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 27, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 27, 2018

📌 Commit e4f0d6a has been approved by bitcartel

zkbot added a commit that referenced this pull request Sep 27, 2018

Auto merge of #3499 - arcalinea:3379_z_listreceivedbyaddress, r=bitca…
…rtel

Sapling support for z_listreceivedbyaddress

Closes #3379.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 27, 2018

⌛️ Testing commit e4f0d6a with merge 2558943...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 28, 2018

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

@zkbot zkbot merged commit e4f0d6a into zcash:master Sep 28, 2018

1 check passed

homu Test successful
Details

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

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