Apply suggestions from code review

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: str4d <thestr4d@gmail.com>
This commit is contained in:
Kris Nuttycombe 2022-02-07 16:35:06 -07:00 committed by Kris Nuttycombe
parent 13ee447a42
commit cf271473eb
13 changed files with 97 additions and 103 deletions

View File

@ -25,31 +25,17 @@ class WalletZSendmanyTest(BitcoinTestFramework):
# Check we only have balances in the expected pools.
# Remember that empty pools are omitted from the output.
def check_account_balance(self, node, account, expected, minconf=None):
if minconf is None:
actual = self.nodes[node].z_getbalanceforaccount(account)
else:
actual = self.nodes[node].z_getbalanceforaccount(account, minconf)
assert_equal(set(expected), set(actual['pools']))
for pool in expected:
assert_equal(expected[pool] * COIN, actual['pools'][pool]['valueZat'])
assert_equal(actual['minimum_confirmations'], 1 if minconf is None else minconf)
# Check we only have balances in the expected pools.
# Remember that empty pools are omitted from the output.
def check_address_balance(self, node, address, expected, minconf=None):
if minconf is None:
actual = self.nodes[node].z_getbalanceforaddress(address)
else:
actual = self.nodes[node].z_getbalanceforaddress(address, minconf)
def _check_balance_for_rpc(self, rpcmethod, node, account, expected, minconf):
rpc = getattr(self.nodes[node], rpcmethod)
actual = rpc(account) if minconf is None else rpc(account, minconf)
assert_equal(set(expected), set(actual['pools']))
for pool in expected:
assert_equal(expected[pool] * COIN, actual['pools'][pool]['valueZat'])
assert_equal(actual['minimum_confirmations'], 1 if minconf is None else minconf)
def check_balance(self, node, account, address, expected, minconf=None):
self.check_account_balance(node, account, expected, minconf)
self.check_address_balance(node, address, expected, minconf)
self._check_balance_for_rpc('z_getbalanceforaccount', node, account, expected, minconf)
self._check_balance_for_rpc('z_getbalanceforaddress', node, address, expected, minconf)
def run_test(self):
# z_sendmany is expected to fail if tx size breaks limit
@ -170,10 +156,10 @@ class WalletZSendmanyTest(BitcoinTestFramework):
n0account0 = self.nodes[0].z_getnewaccount()['account']
n0ua0 = self.nodes[0].z_getaddressforaccount(n0account0)['unifiedaddress']
# Change went to a fresh address, so use `ANY_TADDR` which
# Change went to a fresh address, so use `ANY_TADDR` which
# should hold the rest of our transparent funds.
recipients = []
recipients.append({"address":n0ua0, "amount":10})
recipients.append({"address":n0ua0, "amount":10})
opid = self.nodes[2].z_sendmany('ANY_TADDR', recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[2], opid)
@ -188,7 +174,7 @@ class WalletZSendmanyTest(BitcoinTestFramework):
# Send some funds to a specific legacy taddr that we can spend from
recipients = []
recipients.append({"address":mytaddr, "amount":5})
recipients.append({"address":mytaddr, "amount":5})
opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[0], opid)
@ -201,7 +187,7 @@ class WalletZSendmanyTest(BitcoinTestFramework):
# Send some funds to a legacy sapling address that we can spend from
recipients = []
recipients.append({"address":myzaddr, "amount":3})
recipients.append({"address":myzaddr, "amount":3})
opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[0], opid)
@ -214,7 +200,7 @@ class WalletZSendmanyTest(BitcoinTestFramework):
# Send funds back from the legacy taddr to the UA
recipients = []
recipients.append({"address":n0ua0, "amount":4})
recipients.append({"address":n0ua0, "amount":4})
opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[2], opid)
@ -227,7 +213,7 @@ class WalletZSendmanyTest(BitcoinTestFramework):
# Send funds back from the legacy zaddr to the UA
recipients = []
recipients.append({"address":n0ua0, "amount":2})
recipients.append({"address":n0ua0, "amount":2})
opid = self.nodes[2].z_sendmany(myzaddr, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[2], opid)

View File

@ -260,9 +260,6 @@ TEST(KeystoreTests, StoreAndRetrieveSaplingSpendingKey) {
// Sanity-check: we can't get a full viewing key we haven't added
EXPECT_FALSE(keyStore.HaveSaplingFullViewingKey(ivk));
EXPECT_FALSE(keyStore.GetSaplingFullViewingKey(ivk, extfvkOut));
// Sanity-check: we can't get an incoming viewing key we haven't added
EXPECT_FALSE(keyStore.HaveSaplingIncomingViewingKey(addr));
EXPECT_FALSE(keyStore.GetSaplingIncomingViewingKey(addr, ivkOut));
// When we specify the default address, we get the full mapping
keyStore.AddSaplingSpendingKey(sk);
@ -270,6 +267,11 @@ TEST(KeystoreTests, StoreAndRetrieveSaplingSpendingKey) {
EXPECT_TRUE(keyStore.GetSaplingSpendingKey(extfvk, skOut));
EXPECT_TRUE(keyStore.HaveSaplingFullViewingKey(ivk));
EXPECT_TRUE(keyStore.GetSaplingFullViewingKey(ivk, extfvkOut));
// We can't get an incoming viewing key for an address we haven't added
EXPECT_FALSE(keyStore.HaveSaplingIncomingViewingKey(addr));
EXPECT_FALSE(keyStore.GetSaplingIncomingViewingKey(addr, ivkOut));
keyStore.AddSaplingPaymentAddress(ivk, addr);
EXPECT_TRUE(keyStore.HaveSaplingIncomingViewingKey(addr));
EXPECT_TRUE(keyStore.GetSaplingIncomingViewingKey(addr, ivkOut));
@ -310,9 +312,11 @@ TEST(KeystoreTests, StoreAndRetrieveSaplingFullViewingKey) {
EXPECT_TRUE(keyStore.GetSaplingFullViewingKey(ivk, extfvkOut));
EXPECT_EQ(extfvk, extfvkOut);
// We should still not have the spending key...
// We should still not have the spending key or
// be able to retrieve the IVK by the default address...
EXPECT_FALSE(keyStore.HaveSaplingSpendingKey(extfvk));
EXPECT_FALSE(keyStore.GetSaplingSpendingKey(extfvk, skOut));
EXPECT_FALSE(keyStore.HaveSaplingIncomingViewingKey(addr));
// The IVK must be manually associated with the address...
keyStore.AddSaplingPaymentAddress(ivk, addr);

View File

@ -313,8 +313,8 @@ extern "C" {
);
/**
* Derive the Sapling FVK for an internal BIP44 chain from the
* corresponding external chain FVK.
* Derive the Sapling internal FVK corresponding to the given
* Sapling external FVK.
*/
void librustzcash_zip32_sapling_derive_internal_fvk(
const unsigned char *fvk,

View File

@ -13,7 +13,6 @@
#include "script/script.h"
#include "script/standard.h"
#include "uint256.h"
#include "util/match.h"
#include "zcash/Address.hpp"
#include "zcash/IncrementalMerkleTree.hpp"
#include "zcash/JoinSplit.hpp"

View File

@ -264,10 +264,10 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n",
getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(fee_));
LogPrint("zrpc", "%s: transparent input: %s (to choose from)\n", getId(), FormatMoney(t_inputs_total));
LogPrint("zrpcunsafe", "%s: private input: %s (to choose from)\n", getId(), FormatMoney(z_inputs_total));
LogPrint("zrpc", "%s: transparent output: %s\n", getId(), FormatMoney(txOutputAmounts_.t_outputs_total));
LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(txOutputAmounts_.sapling_outputs_total));
LogPrint("zrpc", "%s: total transparent input: %s (to choose from)\n", getId(), FormatMoney(t_inputs_total));
LogPrint("zrpcunsafe", "%s: total shielded input: %s (to choose from)\n", getId(), FormatMoney(z_inputs_total));
LogPrint("zrpc", "%s: total transparent output: %s\n", getId(), FormatMoney(txOutputAmounts_.t_outputs_total));
LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(), FormatMoney(txOutputAmounts_.sapling_outputs_total));
LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(fee_));
auto ovks = this->SelectOVKs(spendable);

View File

@ -59,7 +59,7 @@ TEST(WalletZkeysTest, StoreAndLoadSaplingZkeys) {
wallet.GetSaplingSpendingKey(extfvk, keyOut);
ASSERT_EQ(sk, keyOut);
// verify there is still only one key; adding the spending
// verify there is still only one address; adding the spending
// key no longer adds the default address
wallet.GetSaplingPaymentAddresses(addrs);
EXPECT_EQ(1, addrs.size());
@ -472,6 +472,8 @@ TEST(WalletZkeysTest, WriteCryptedSaplingZkeyDirectToDb) {
wallet.Unlock(strWalletPass);
auto address2 = wallet.GenerateNewLegacySaplingZKey();
// wallet should have three addresses: the default addresses for the two
// generated keys, and the added diversified address.
wallet.GetSaplingPaymentAddresses(addrs);
ASSERT_EQ(3, addrs.size());

View File

@ -3169,7 +3169,7 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp)
params[2].getValStr());
break;
case WalletUAGenerationError::WalletEncrypted:
// By construction, we should never see these errors; this case is included
// By construction, we should never see this error; this case is included
// only for future-proofing.
strErr = tfm::format("Error: wallet is encrypted.");
}
@ -4409,7 +4409,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
if (selectorAccount.has_value() && selectorAccount.value() != ZCASH_LEGACY_ACCOUNT) {
throw JSONRPCError(
RPC_INVALID_ADDRESS_OR_KEY,
"Invalid from address, bare address does not correspond the legacy account.");
"Invalid from address: is a bare receiver from a Unified Address in this wallet. Provide the UA as returned by z_getaddressforaccount instead.");
}
}
}, decoded.value());

View File

@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet)
LOCK2(cs_main, pwalletMain->cs_wallet);
CPubKey demoPubkey = pwalletMain->GenerateNewKey(false);
CPubKey demoPubkey = pwalletMain->GenerateNewKey(true);
CTxDestination demoAddress(CTxDestination(demoPubkey.GetID()));
UniValue retValue;
string strPurpose = "receive";
@ -143,7 +143,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet)
pwalletMain->SetAddressBook(demoPubkey.GetID(), "", strPurpose);
});
CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey(false);
CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey(true);
CTxDestination setaccountDemoAddress(CTxDestination(setaccountDemoPubkey.GetID()));
/*********************************
@ -1325,7 +1325,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling)
KeyIO keyIO(Params());
// add keys manually
auto taddr = pwalletMain->GenerateNewKey(false).GetID();
auto taddr = pwalletMain->GenerateNewKey(true).GetID();
std::string taddr1 = keyIO.EncodeDestination(taddr);
auto pa = pwalletMain->GenerateNewLegacySaplingZKey();
@ -1987,7 +1987,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_internals)
void TestWTxStatus(const Consensus::Params consensusParams, const int delta) {
auto AddTrx = [&consensusParams]() {
auto taddr = pwalletMain->GenerateNewKey(false).GetID();
auto taddr = pwalletMain->GenerateNewKey(true).GetID();
CMutableTransaction mtx = CreateNewContextualCMutableTransaction(consensusParams, 1);
CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(taddr) << OP_EQUALVERIFY << OP_CHECKSIG;
mtx.vout.push_back(CTxOut(5 * COIN, scriptPubKey));

View File

@ -232,8 +232,6 @@ bool CWallet::AddSaplingPaymentAddress(
}
return CWalletDB(strWalletFile).WriteSaplingPaymentAddress(addr, ivk);
return true;
}
@ -259,10 +257,9 @@ bool CWallet::AddSproutZKey(const libzcash::SproutSpendingKey &key)
return true;
}
CPubKey CWallet::GenerateNewKey(bool isChangeKey)
CPubKey CWallet::GenerateNewKey(bool external)
{
AssertLockHeld(cs_wallet); // mapKeyMetadata
bool external = !isChangeKey;
if (!mnemonicHDChain.has_value()) {
throw std::runtime_error(
@ -679,7 +676,7 @@ WalletUAGenerationResult CWallet::GenerateUnifiedAddress(
assert(mapUfvkAddressMetadata[ufvkid].SetReceivers(address.second, receiverTypes));
if (hasTransparent) {
// We must construct the and add transparent spending key associated
// We must construct and add the transparent spending key associated
// with the external and internal transparent child addresses to the
// transparent keystore.
auto usk = GenerateUnifiedSpendingKeyForAccount(accountId).value();
@ -1635,7 +1632,7 @@ std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
// we're able to generate.
for (libzcash::ChangeType t : changeOptions) {
if (t == libzcash::ChangeType::Transparent && accountId == ZCASH_LEGACY_ACCOUNT) {
return GenerateNewKey(true).GetID();
return GenerateNewKey(false).GetID();
} else {
auto ufvk = this->GetUnifiedFullViewingKeyByAccount(accountId);
if (ufvk.has_value()) {
@ -4966,7 +4963,7 @@ bool CWallet::NewKeyPool()
for (int i = 0; i < nKeys; i++)
{
int64_t nIndex = i+1;
walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(false)));
walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(true)));
setKeyPool.insert(nIndex);
}
LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys);
@ -4996,7 +4993,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
int64_t nEnd = 1;
if (!setKeyPool.empty())
nEnd = *(--setKeyPool.end()) + 1;
if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(false))))
if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(true))))
throw runtime_error("TopUpKeyPool(): writing generated key failed");
setKeyPool.insert(nEnd);
LogPrintf("keypool added key %d, size=%u\n", nEnd, setKeyPool.size());
@ -5063,7 +5060,7 @@ std::optional<CPubKey> CWallet::GetKeyFromPool()
if (nIndex == -1)
{
if (IsLocked()) return std::nullopt;
return GenerateNewKey(false);
return GenerateNewKey(true);
}
KeepKey(nIndex);
return keypool.vchPubKey;
@ -6275,38 +6272,36 @@ KeyAddResult AddSpendingKeyToWallet::operator()(const libzcash::SaplingExtendedS
auto ivk = extfvk.ToIncomingViewingKey();
auto addr = extfvk.DefaultAddress();
KeyIO keyIO(Params());
{ // TODO: why is this extra scope here?
if (log){
LogPrint("zrpc", "Importing zaddr %s...\n", keyIO.EncodePaymentAddress(addr));
if (log){
LogPrint("zrpc", "Importing zaddr %s...\n", keyIO.EncodePaymentAddress(addr));
}
// Don't throw error in case a key is already there
if (m_wallet->HaveSaplingSpendingKey(extfvk)) {
return KeyAlreadyExists;
} else {
if (!(
m_wallet->AddSaplingZKey(sk) &&
(!addDefaultAddress || m_wallet->AddSaplingPaymentAddress(ivk, addr))
)) {
return KeyNotAdded;
}
// Don't throw error in case a key is already there
if (m_wallet->HaveSaplingSpendingKey(extfvk)) {
return KeyAlreadyExists;
} else {
if (!(
m_wallet->AddSaplingZKey(sk) &&
(!addDefaultAddress || m_wallet->AddSaplingPaymentAddress(ivk, addr))
)) {
return KeyNotAdded;
}
// Sapling addresses can't have been used in transactions prior to activation.
if (params.vUpgrades[Consensus::UPGRADE_SAPLING].nActivationHeight == Consensus::NetworkUpgrade::ALWAYS_ACTIVE) {
m_wallet->mapSaplingZKeyMetadata[ivk].nCreateTime = nTime;
} else {
// 154051200 seconds from epoch is Friday, 26 October 2018 00:00:00 GMT - definitely before Sapling activates
m_wallet->mapSaplingZKeyMetadata[ivk].nCreateTime = std::max((int64_t) 154051200, nTime);
}
if (hdKeypath.has_value()) {
m_wallet->mapSaplingZKeyMetadata[ivk].hdKeypath = hdKeypath.value();
}
if (seedFpStr.has_value()) {
uint256 seedFp;
seedFp.SetHex(seedFpStr.value());
m_wallet->mapSaplingZKeyMetadata[ivk].seedFp = seedFp;
}
return KeyAdded;
// Sapling addresses can't have been used in transactions prior to activation.
if (params.vUpgrades[Consensus::UPGRADE_SAPLING].nActivationHeight == Consensus::NetworkUpgrade::ALWAYS_ACTIVE) {
m_wallet->mapSaplingZKeyMetadata[ivk].nCreateTime = nTime;
} else {
// 154051200 seconds from epoch is Friday, 26 October 2018 00:00:00 GMT - definitely before Sapling activates
m_wallet->mapSaplingZKeyMetadata[ivk].nCreateTime = std::max((int64_t) 154051200, nTime);
}
if (hdKeypath.has_value()) {
m_wallet->mapSaplingZKeyMetadata[ivk].hdKeypath = hdKeypath.value();
}
if (seedFpStr.has_value()) {
uint256 seedFp;
seedFp.SetHex(seedFpStr.value());
m_wallet->mapSaplingZKeyMetadata[ivk].seedFp = seedFp;
}
return KeyAdded;
}
}

View File

@ -1254,9 +1254,17 @@ public:
std::optional<libzcash::AccountId> FindAccountForSelector(const ZTXOSelector& paymentSource) const;
/**
* Generate a change address for the specified account. If a transparent change
* address is requested, this will generate a fresh diversified unified address,
* Generate a change address for the specified account.
*
* If a shielded change address is requested, this will return the default
* unified address for the internal unified full viewing key.
*
* If a transparent change address is requested, this will generate a fresh
* diversified unified address from the internal unified full viewing key,
* and return the associated transparent change address.
*
* Returns `std::nullopt` if the account does not have an internal spending
* key matching the requested `ChangeType`.
*/
std::optional<libzcash::RecipientAddress> GenerateChangeAddressForAccount(
libzcash::AccountId accountId,
@ -1297,7 +1305,7 @@ public:
* keystore implementation
* Generate a new key
*/
CPubKey GenerateNewKey(bool isChangeKey);
CPubKey GenerateNewKey(bool external);
//! Adds a key to the store, and saves it to disk.
bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey);
//! Adds a key to the store, without saving it to disk (used by LoadWallet)
@ -1365,10 +1373,10 @@ public:
//! Generates new Sapling key, stores the newly generated spending
//! key to the wallet, and returns the default address for the newly generated key.
libzcash::SaplingPaymentAddress GenerateNewLegacySaplingZKey();
//! Generates Sapling key at the specified address index, and stores the newly generated
//! spending key to the wallet if it has not alreay been persisted.
//! Returns the newly created key, and a flag distinguishing
//! whether or not the key was already known by the wallet.
//! Generates Sapling key at the specified address index, and stores that
//! key to the wallet if it has not already been persisted. Returns the
//! default address for the key, and a flag that is true when the key
//! was newly generated (not already in the wallet).
std::pair<libzcash::SaplingPaymentAddress, bool> GenerateLegacySaplingZKey(uint32_t addrIndex);
//! Adds Sapling spending key to the store, and saves it to disk
bool AddSaplingZKey(const libzcash::SaplingExtendedSpendingKey &key);

View File

@ -17,7 +17,6 @@ const size_t SerializedSaplingPaymentAddressSize = 43;
const size_t SerializedSaplingFullViewingKeySize = 96;
const size_t SerializedSaplingExpandedSpendingKeySize = 96;
const size_t SerializedSaplingSpendingKeySize = 32;
const size_t SerializedSaplingDiversifierKeySize = 32;
typedef std::array<unsigned char, ZC_DIVERSIFIER_SIZE> diversifier_t;

View File

@ -122,17 +122,14 @@ UnifiedAddressGenerationResult ZcashdUnifiedFullViewingKey::FindAddress(
const std::set<ReceiverType>& receiverTypes) const {
diversifier_index_t j0(j);
bool hasTransparent = HasTransparent(receiverTypes);
auto addr = Address(j0, receiverTypes);
while (addr == UnifiedAddressGenerationResult(UnifiedAddressGenerationError::NoAddressForDiversifier)) {
if (!j0.increment())
return UnifiedAddressGenerationError::DiversifierSpaceExhausted;
do {
auto addr = Address(j0, receiverTypes);
if (addr != UnifiedAddressGenerationResult(UnifiedAddressGenerationError::NoAddressForDiversifier)) {
return addr;
}
} while (j0.increment());
if (hasTransparent && !j0.ToTransparentChildIndex().has_value())
return UnifiedAddressGenerationError::InvalidTransparentChildIndex;
addr = Address(j0, receiverTypes);
}
return addr;
return UnifiedAddressGenerationError::DiversifierSpaceExhausted;
}
UnifiedAddressGenerationResult ZcashdUnifiedFullViewingKey::FindAddress(
@ -152,7 +149,7 @@ std::optional<RecipientAddress> ZcashdUnifiedFullViewingKey::GetChangeAddress(co
}
},
[&](const SaplingChangeRequest& req) {
// currently true by construction, as a UFVK must have a shielded component
// currently true by construction, as a UFVK must have a supported shielded component
if (saplingKey.has_value()) {
addr = saplingKey.value().GetChangeAddress();
}

View File

@ -175,8 +175,8 @@ public:
* `UnifiedAddressGenerationError::InvalidTransparentChildIndex` if a transparent
* receiver was requested but the specified diversifier was out of range.
*
* If successful in deriving an address, this method returns a pair consisting of the
* newly derived address and the provided value `j`.
* If successful in deriving an address, this method returns a `UnifiedAddressGenerationResult`
* holding a pair consisting of the newly derived address and the provided value `j`.
*/
UnifiedAddressGenerationResult Address(
const diversifier_index_t& j,
@ -206,7 +206,11 @@ public:
* Return the change address for this UFVK, given the provided
* set of receiver types for pools involved in this transaction.
* If the provided set is empty, return the change address
* corresponding to the most-preferred pool.
* corresponding to the most-preferred pool. Returns `std::nullopt`
* if the request cannot be satisfied; for example, if a transparent
* change address is requested but derivation fails for the requested
* child index, or if the set of requested protocols does not intersect
* with those supported by the this UFVKs constituent keys.
*/
std::optional<RecipientAddress> GetChangeAddress(const ChangeRequest& req) const;