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

Jump to bottom

Sapling support for z_listreceivedbyaddress #3499

Merged
merged 1 commit into from Sep 28, 2018
Merged

Sapling support for z_listreceivedbyaddress #3499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions qa/pull-tester/rpc-tests.sh
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ testScripts=(
'wallet_import_export.py'
'wallet_protectcoinbase.py'
'wallet_shieldcoinbase.py'
'wallet_listreceived.py'
'wallet_mergetoaddress.py'
'wallet.py'
'wallet_overwintertx.py'
Expand Down
101 changes: 101 additions & 0 deletions qa/rpc-tests/wallet_listreceived.py
View file
Open in desktop
@@ -0,0 +1,101 @@
#!/usr/bin/env python2
# Copyright (c) 2018 The Zcash developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal, assert_true, assert_false
from test_framework.util import start_nodes, wait_and_assert_operationid_status
from decimal import Decimal

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

no_memo = 'f6' + ('0'*1022) # see section 5.5 of the protocol spec
# sapling generates zero_memo, but this may be fixed soon (to no_memo)
# then this test can be simplified
zero_memo = '0'*1024

fee = Decimal('0.0001')

class ListReceivedTest (BitcoinTestFramework):

def setup_nodes(self):
return start_nodes(4, self.options.tmpdir, [[
"-nuparams=5ba81b19:201", # Overwinter
"-nuparams=76b809bb:204", # Sapling
]] * 4)

def generate_and_sync(self, new_height):
self.nodes[0].generate(1)
self.sync_all()
assert_equal(new_height, self.nodes[0].getblockcount())

def run_test_release(self, release, expected_memo, height):
self.generate_and_sync(height+1)
taddr = self.nodes[1].getnewaddress()
zaddr1 = self.nodes[1].z_getnewaddress(release)

self.nodes[0].sendtoaddress(taddr, 2.0)
self.generate_and_sync(height+2)

# Send 1 ZEC to zaddr1
opid = self.nodes[1].z_sendmany(taddr,
[{'address': zaddr1, 'amount': 1, 'memo': my_memo}])
txid = wait_and_assert_operationid_status(self.nodes[1], opid)
self.sync_all()
r = self.nodes[1].z_listreceivedbyaddress(zaddr1)
assert_equal(0, len(r), "Should have received no confirmed note")

# No confirmation required, one note should be present
r = self.nodes[1].z_listreceivedbyaddress(zaddr1, 0)
assert_equal(1, len(r), "Should have received one (unconfirmed) note")
assert_equal(txid, r[0]['txid'])
assert_equal(1, r[0]['amount'])
assert_false(r[0]['change'], "Note should not be change")
assert_equal(my_memo, r[0]['memo'])

# Confirm transaction (1 ZEC from taddr to zaddr1)
self.generate_and_sync(height+3)

# Require one confirmation, note should be present
assert_equal(r, self.nodes[1].z_listreceivedbyaddress(zaddr1))

# Generate some change by sending part of zaddr1 to zaddr2
zaddr2 = self.nodes[1].z_getnewaddress(release)
opid = self.nodes[1].z_sendmany(zaddr1,
[{'address': zaddr2, 'amount': 0.6, 'memo': my_memo}])
txid = wait_and_assert_operationid_status(self.nodes[1], opid)
self.sync_all()
self.generate_and_sync(height+4)

# zaddr1 should have a note with change
r = self.nodes[1].z_listreceivedbyaddress(zaddr1, 0)
r = sorted(r, key = lambda received: received['amount'])
assert_equal(2, len(r), "zaddr1 Should have received 2 notes")

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-fee) should be change")
assert_equal(expected_memo, r[0]['memo'])

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done https://github.com/zcash/zcash/projects/23#card-13380398

Copy link
Contributor

@daira daira Sep 28, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

assert_equal(Decimal('1.0'), r[1]['amount'])
assert_false(r[1]['change'], "Note valued at 1.0 should not be change")
assert_equal(expected_memo, r[0]['memo'])

# zaddr2 should not have change
r = self.nodes[1].z_listreceivedbyaddress(zaddr2, 0)
r = sorted(r, key = lambda received: received['amount'])
assert_equal(1, len(r), "zaddr2 Should have received 1 notes")
assert_equal(txid, r[0]['txid'])
assert_equal(Decimal('0.6'), r[0]['amount'])
assert_false(r[0]['change'], "Note valued at 0.6 should not be change")
assert_equal(my_memo, r[0]['memo'])

def run_test(self):
self.run_test_release('sprout', no_memo, 200)
self.run_test_release('sapling', zero_memo, 204)

if __name__ == '__main__':
ListReceivedTest().main()
55 changes: 35 additions & 20 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Expand Up @@ -2579,7 +2579,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
obj.push_back(Pair("memo", HexStr(data)));
if (hasSproutSpendingKey) {
obj.push_back(Pair("change", pwalletMain->IsNoteChange(nullifierSet, entry.address, entry.jsop)));
obj.push_back(Pair("change", pwalletMain->IsNoteSproutChange(nullifierSet, entry.address, entry.jsop)));
}
results.push_back(obj);
}
Expand Down Expand Up @@ -3307,35 +3307,50 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp)
if (!IsValidPaymentAddress(zaddr)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid zaddr.");
}
// TODO: Add Sapling support. For now, ensure we can freely convert.
assert(boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr);
auto sproutzaddr = boost::get<libzcash::SproutPaymentAddress>(zaddr);

bool hasSproutSpendingKey = pwalletMain->HaveSproutSpendingKey(sproutzaddr);
if (!(hasSproutSpendingKey || pwalletMain->HaveSproutViewingKey(sproutzaddr))) {
// Visitor to support Sprout and Sapling addrs
if (!boost::apply_visitor(PaymentAddressBelongsToWallet(pwalletMain), zaddr)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "From address does not belong to this node, zaddr spending key or viewing key not found.");
}

UniValue result(UniValue::VARR);
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>>();
for (CSproutNotePlaintextEntry & entry : sproutEntries) {
UniValue obj(UniValue::VOBJ);
obj.push_back(Pair("txid", entry.jsop.hash.ToString()));
obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.plaintext.value()))));
std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
obj.push_back(Pair("memo", HexStr(data)));
// (txid, jsindex, jsoutindex) is needed to globally identify a note
obj.push_back(Pair("jsindex", entry.jsop.js));
obj.push_back(Pair("jsoutindex", entry.jsop.n));
if (hasSproutSpendingKey) {
obj.push_back(Pair("change", pwalletMain->IsNoteChange(nullifierSet, entry.address, entry.jsop)));

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

if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr) {
for (CSproutNotePlaintextEntry & entry : sproutEntries) {
UniValue obj(UniValue::VOBJ);
obj.push_back(Pair("txid", entry.jsop.hash.ToString()));
obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.plaintext.value()))));
std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
obj.push_back(Pair("memo", HexStr(data)));
obj.push_back(Pair("jsindex", entry.jsop.js));
obj.push_back(Pair("jsoutindex", entry.jsop.n));
if (hasSpendingKey) {
obj.push_back(Pair("change", pwalletMain->IsNoteSproutChange(nullifierSet, entry.address, entry.jsop)));
}
result.push_back(obj);
}
} else if (boost::get<libzcash::SaplingPaymentAddress>(&zaddr) != nullptr) {
for (SaplingNoteEntry & entry : saplingEntries) {
UniValue obj(UniValue::VOBJ);
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));
if (hasSpendingKey) {
obj.push_back(Pair("change", pwalletMain->IsNoteSaplingChange(nullifierSet, entry.address, entry.op)));
}
result.push_back(obj);
}
result.push_back(obj);
}
// TODO: Sapling
return result;
}

Expand Down
58 changes: 54 additions & 4 deletions src/wallet/wallet.cpp
View file
Open in desktop
Expand Up @@ -512,20 +512,50 @@ void CWallet::SetBestChain(const CBlockLocator& loc)
SetBestChainINTERNAL(walletdb, loc);
}

std::set<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersForAddresses(const std::set<libzcash::PaymentAddress> & addresses)
std::set<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersForAddresses(
const std::set<libzcash::PaymentAddress> & addresses)
{
std::set<std::pair<libzcash::PaymentAddress, uint256>> nullifierSet;
// Sapling ivk -> list of addrs map
LarryRuane marked this conversation as resolved.
Show resolved Hide resolved
// (There may be more than one diversified address for a given ivk.)
std::map<libzcash::SaplingIncomingViewingKey, std::vector<libzcash::SaplingPaymentAddress>> ivkMap;
Copy link
Collaborator

@LarryRuane LarryRuane Sep 20, 2018 •
edited by daira

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

for (const auto & addr : addresses) {
auto saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(&addr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

if (saplingAddr != nullptr) {
libzcash::SaplingIncomingViewingKey ivk;
this->GetSaplingIncomingViewingKey(*saplingAddr, ivk);
ivkMap[ivk].push_back(*saplingAddr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
for (const auto & txPair : mapWallet) {
// Sprout
for (const auto & noteDataPair : txPair.second.mapSproutNoteData) {
if (noteDataPair.second.nullifier && addresses.count(noteDataPair.second.address)) {
nullifierSet.insert(std::make_pair(noteDataPair.second.address, noteDataPair.second.nullifier.get()));
auto & noteData = noteDataPair.second;
auto & nullifier = noteData.nullifier;
auto & address = noteData.address;
if (nullifier && addresses.count(address)) {
nullifierSet.insert(std::make_pair(address, nullifier.get()));
}
}
// Sapling
for (const auto & noteDataPair : txPair.second.mapSaplingNoteData) {
auto & noteData = noteDataPair.second;
auto & nullifier = noteData.nullifier;
auto & ivk = noteData.ivk;
if (nullifier && ivkMap.count(ivk)) {
for (const auto & addr : ivkMap[ivk]) {
nullifierSet.insert(std::make_pair(addr, nullifier.get()));
}
Copy link
Contributor

@daira daira Sep 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@daira daira Sep 27, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@daira daira Sep 28, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
return nullifierSet;
}

bool CWallet::IsNoteChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const PaymentAddress & address, const JSOutPoint & jsop)
bool CWallet::IsNoteSproutChange(
const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet,
const PaymentAddress & address,
const JSOutPoint & jsop)
{
// A Note is marked as "change" if the address that received it
// also spent Notes in the same transaction. This will catch,
Expand All @@ -546,6 +576,26 @@ 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)
{
// A Note is marked as "change" if the address that received it
// also spent Notes in the same transaction. This will catch,
// for instance:
// - Change created by spending fractions of Notes (because
// z_sendmany sends change to the originating z-address).
// - Notes created by consolidation transactions (e.g. using
// z_mergetoaddress).
// - Notes sent from one address to itself.
for (const SpendDescription &spend : mapWallet[op.hash].vShieldedSpend) {
if (nullifierSet.count(std::make_pair(address, spend.nullifier))) {
return true;
}
}
return false;
}

bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool fExplicit)
{
LOCK(cs_wallet); // nWalletVersion
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Expand Up @@ -1161,7 +1161,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
/** Saves witness caches and best block locator to disk. */
void SetBestChain(const CBlockLocator& loc);
std::set<std::pair<libzcash::PaymentAddress, uint256>> GetNullifiersForAddresses(const std::set<libzcash::PaymentAddress> & addresses);
bool IsNoteChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const JSOutPoint & entry);
bool IsNoteSproutChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const JSOutPoint & entry);
bool IsNoteSaplingChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const SaplingOutPoint & entry);

DBErrors LoadWallet(bool& fFirstRunRet);
DBErrors ZapWalletTx(std::vector<CWalletTx>& vWtx);
Expand Down