From 81a45d6984ef656f3b82829b20717a0c9d497d5b Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Mon, 18 Mar 2019 20:33:02 -0600 Subject: [PATCH 1/7] Add rpc to enable and disable Sprout to Sapling migration --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/sprout_sapling_migration.py | 96 +++++++++ qa/rpc-tests/test_framework/util.py | 13 +- src/Makefile.am | 2 + src/main.cpp | 5 + src/main.h | 1 + src/validationinterface.cpp | 3 + src/validationinterface.h | 3 + .../asyncrpcoperation_saplingmigration.cpp | 196 ++++++++++++++++++ .../asyncrpcoperation_saplingmigration.h | 31 +++ src/wallet/rpcwallet.cpp | 22 ++ src/wallet/wallet.cpp | 47 +++++ src/wallet/wallet.h | 6 + 13 files changed, 421 insertions(+), 5 deletions(-) create mode 100755 qa/rpc-tests/sprout_sapling_migration.py create mode 100644 src/wallet/asyncrpcoperation_saplingmigration.cpp create mode 100644 src/wallet/asyncrpcoperation_saplingmigration.h diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 19c65a0b5..d072898b2 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -71,6 +71,7 @@ testScripts=( 'p2p_node_bloom.py' 'regtest_signrawtransaction.py' 'finalsaplingroot.py' + 'sprout_sapling_migration.py' 'turnstile.py' ); testScriptsExt=( diff --git a/qa/rpc-tests/sprout_sapling_migration.py b/qa/rpc-tests/sprout_sapling_migration.py new file mode 100755 index 000000000..726e5e2aa --- /dev/null +++ b/qa/rpc-tests/sprout_sapling_migration.py @@ -0,0 +1,96 @@ +#!/usr/bin/env python +# Copyright (c) 2019 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +import sys; assert sys.version_info < (3,), ur"This script does not run under Python 3. Please use Python 2.7.x." + +from decimal import Decimal +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal, assert_true, get_coinbase_address, \ + initialize_chain_clean, start_nodes, wait_and_assert_operationid_status + + +class SproutSaplingMigration(BitcoinTestFramework): + def setup_nodes(self): + return start_nodes(4, self.options.tmpdir, [[ + '-nuparams=5ba81b19:100', # Overwinter + '-nuparams=76b809bb:100', # Sapling + ]] * 4) + + def setup_chain(self): + print("Initializing test directory " + self.options.tmpdir) + initialize_chain_clean(self.options.tmpdir, 4) + + def run_test(self): + print "Mining blocks..." + self.nodes[0].generate(101) + self.sync_all() + + # Send some ZEC to a Sprout address + tAddr = get_coinbase_address(self.nodes[0]) + sproutAddr = self.nodes[0].z_getnewaddress('sprout') + saplingAddr = self.nodes[0].z_getnewaddress('sapling') + + opid = self.nodes[0].z_sendmany(tAddr, [{"address": sproutAddr, "amount": Decimal('10')}], 1, 0) + wait_and_assert_operationid_status(self.nodes[0], opid) + self.nodes[0].generate(1) + self.sync_all() + + assert_equal(self.nodes[0].z_getbalance(sproutAddr), Decimal('10')) + assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('0')) + + # Migrate + assert_equal(True, self.nodes[0].z_setmigration(True)) + print "Mining to block 494..." + self.nodes[0].generate(392) # 102 -> 494 + self.sync_all() + + # At 494 we should have no async operations + assert_equal(0, len(self.nodes[0].z_getoperationstatus()), "num async operations at 494") + + self.nodes[0].generate(1) + self.sync_all() + + # At 495 we should have an async operation + operationstatus = self.nodes[0].z_getoperationstatus() + assert_equal(1, len(operationstatus), "num async operations at 495") + assert_equal('saplingmigration', operationstatus[0]['method']) + assert_equal(500, operationstatus[0]['target_height']) + + print "migration operation: {}".format(operationstatus) + migration_opid = operationstatus[0]['id'] + result = wait_and_assert_operationid_status(self.nodes[0], migration_opid) + print "result: {}".format(result) + assert_equal(0, len(self.nodes[0].getrawmempool()), "mempool size at 495") + + self.nodes[0].generate(3) + self.sync_all() + + # At 498 the mempool will be empty and no funds will have moved + assert_equal(0, len(self.nodes[0].getrawmempool()), "mempool size at 498") + assert_equal(self.nodes[0].z_getbalance(sproutAddr), Decimal('10')) + assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('0')) + + self.nodes[0].generate(1) + self.sync_all() + + # At 499 there will be a transaction in the mempool and the note will be locked + assert_equal(1, len(self.nodes[0].getrawmempool()), "mempool size at 499") + assert_equal(self.nodes[0].z_getbalance(sproutAddr), Decimal('0')) + assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('0')) + assert_true(self.nodes[0].z_getbalance(saplingAddr, 0) > Decimal('0'), "Unconfirmed sapling") + + self.nodes[0].generate(1) + self.sync_all() + + # At 500 funds will have moved + sprout_balance = self.nodes[0].z_getbalance(sproutAddr) + sapling_balance = self.nodes[0].z_getbalance(saplingAddr) + print "sprout balance: {}, sapling balance: {}".format(sprout_balance, sapling_balance) + assert_true(sprout_balance < Decimal('10'), "Should have less Sprout funds") + assert_true(sapling_balance > Decimal('0'), "Should have more Sapling funds") + + +if __name__ == '__main__': + SproutSaplingMigration().main() diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 77abec55d..7d15ec063 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -399,26 +399,29 @@ def wait_and_assert_operationid_status(node, myopid, in_status='success', in_err assert_true(result is not None, "timeout occured") status = result['status'] - txid = None + ret = None errormsg = None if status == "failed": errormsg = result['error']['message'] elif status == "success": - txid = result['result']['txid'] + if type(result['result']) is dict and result['result'].get('txid'): + ret = result['result']['txid'] + else: + ret = result['result'] if os.getenv("PYTHON_DEBUG", ""): print('...returned status: {}'.format(status)) if errormsg is not None: print('...returned error: {}'.format(errormsg)) - + assert_equal(in_status, status, "Operation returned mismatched status. Error Message: {}".format(errormsg)) if errormsg is not None: assert_true(in_errormsg is not None, "No error retured. Expected: {}".format(errormsg)) assert_true(in_errormsg in errormsg, "Error returned: {}. Error expected: {}".format(errormsg, in_errormsg)) - return result # if there was an error return the result + return result # if there was an error return the result else: - return txid # otherwise return the txid + return ret # otherwise return the txid # Find a coinbase address on the node, filtering by the number of UTXOs it has. # If no filter is provided, returns the coinbase address on the node containing diff --git a/src/Makefile.am b/src/Makefile.am index 16951f9d2..5924abfb6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -215,6 +215,7 @@ BITCOIN_CORE_H = \ validationinterface.h \ version.h \ wallet/asyncrpcoperation_mergetoaddress.h \ + wallet/asyncrpcoperation_saplingmigration.h \ wallet/asyncrpcoperation_sendmany.h \ wallet/asyncrpcoperation_shieldcoinbase.h \ wallet/crypter.h \ @@ -304,6 +305,7 @@ libbitcoin_wallet_a_SOURCES = \ zcbenchmarks.cpp \ zcbenchmarks.h \ wallet/asyncrpcoperation_mergetoaddress.cpp \ + wallet/asyncrpcoperation_saplingmigration.cpp \ wallet/asyncrpcoperation_sendmany.cpp \ wallet/asyncrpcoperation_shieldcoinbase.cpp \ wallet/crypter.cpp \ diff --git a/src/main.cpp b/src/main.cpp index 72744b61a..782ab69b0 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -78,6 +78,7 @@ bool fCoinbaseEnforcedProtectionEnabled = true; size_t nCoinCacheUsage = 5000 * 300; uint64_t nPruneTarget = 0; bool fAlerts = DEFAULT_ALERTS; +bool fSaplingMigrationEnabled = false; /* If the tip is older than this (in seconds), the node is considered to be in initial block download. */ int64_t nMaxTipAge = DEFAULT_MAX_TIP_AGE; @@ -3104,6 +3105,10 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock * EnforceNodeDeprecation(pindexNew->nHeight); + if (fSaplingMigrationEnabled) { + GetMainSignals().RunSaplingMigration(pindexNew->nHeight); + } + int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; LogPrint("bench", " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint("bench", "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); diff --git a/src/main.h b/src/main.h index 3b8c6aff9..0e1fa6694 100644 --- a/src/main.h +++ b/src/main.h @@ -154,6 +154,7 @@ extern bool fCoinbaseEnforcedProtectionEnabled; extern size_t nCoinCacheUsage; extern CFeeRate minRelayTxFee; extern bool fAlerts; +extern bool fSaplingMigrationEnabled; extern int64_t nMaxTipAge; /** Best header we've seen so far (used for getheaders queries' starting points). */ diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 1dc3351ab..bb3f800fc 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -18,6 +18,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.EraseTransaction.connect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4, _5)); + g_signals.RunSaplingMigration.connect(boost::bind(&CValidationInterface::RunSaplingMigration, pwalletIn, _1)); g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); @@ -33,6 +34,7 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4, _5)); + g_signals.RunSaplingMigration.disconnect(boost::bind(&CValidationInterface::RunSaplingMigration, pwalletIn, _1)); g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.EraseTransaction.disconnect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); @@ -47,6 +49,7 @@ void UnregisterAllValidationInterfaces() { g_signals.Broadcast.disconnect_all_slots(); g_signals.Inventory.disconnect_all_slots(); g_signals.ChainTip.disconnect_all_slots(); + g_signals.RunSaplingMigration.disconnect_all_slots(); g_signals.SetBestChain.disconnect_all_slots(); g_signals.UpdatedTransaction.disconnect_all_slots(); g_signals.EraseTransaction.disconnect_all_slots(); diff --git a/src/validationinterface.h b/src/validationinterface.h index 7b02bd9da..9e443d134 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -37,6 +37,7 @@ protected: virtual void SyncTransaction(const CTransaction &tx, const CBlock *pblock) {} virtual void EraseFromWallet(const uint256 &hash) {} virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree, bool added) {} + virtual void RunSaplingMigration(int blockHeight) {} virtual void SetBestChain(const CBlockLocator &locator) {} virtual void UpdatedTransaction(const uint256 &hash) {} virtual void Inventory(const uint256 &hash) {} @@ -60,6 +61,8 @@ struct CMainSignals { boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a change to the tip of the active block chain. */ boost::signals2::signal ChainTip; + /** Notifies listeners that they may need to update the status of the Sprout->Sapling migration */ + boost::signals2::signal RunSaplingMigration; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Notifies listeners about an inventory item being seen on the network. */ diff --git a/src/wallet/asyncrpcoperation_saplingmigration.cpp b/src/wallet/asyncrpcoperation_saplingmigration.cpp new file mode 100644 index 000000000..6a043f9bf --- /dev/null +++ b/src/wallet/asyncrpcoperation_saplingmigration.cpp @@ -0,0 +1,196 @@ +#include "assert.h" +#include "boost/variant/static_visitor.hpp" +#include "asyncrpcoperation_saplingmigration.h" +#include "init.h" +#include "rpc/protocol.h" +#include "random.h" +#include "sync.h" +#include "tinyformat.h" +#include "transaction_builder.h" +#include "util.h" +#include "wallet.h" + +const CAmount FEE = 10000; + +AsyncRPCOperation_saplingmigration::AsyncRPCOperation_saplingmigration(int targetHeight) : targetHeight_(targetHeight) {} + +AsyncRPCOperation_saplingmigration::~AsyncRPCOperation_saplingmigration() {} + +void AsyncRPCOperation_saplingmigration::main() { + if (isCancelled()) + return; + + set_state(OperationStatus::EXECUTING); + start_execution_clock(); + + bool success = false; + + try { + success = main_impl(); + } catch (const UniValue& objError) { + int code = find_value(objError, "code").get_int(); + std::string message = find_value(objError, "message").get_str(); + set_error_code(code); + set_error_message(message); + } catch (const runtime_error& e) { + set_error_code(-1); + set_error_message("runtime error: " + string(e.what())); + } catch (const logic_error& e) { + set_error_code(-1); + set_error_message("logic error: " + string(e.what())); + } catch (const exception& e) { + set_error_code(-1); + set_error_message("general exception: " + string(e.what())); + } catch (...) { + set_error_code(-2); + set_error_message("unknown error"); + } + + stop_execution_clock(); + + if (success) { + set_state(OperationStatus::SUCCESS); + } else { + set_state(OperationStatus::FAILED); + } + + std::string s = strprintf("%s: Sprout->Sapling transactions sent. (status=%s", getId(), getStateAsString()); + if (success) { + s += strprintf(", success)\n"); + } else { + s += strprintf(", error=%s)\n", getErrorMessage()); + } + + LogPrintf("%s", s); +} + +bool AsyncRPCOperation_saplingmigration::main_impl() { + std::vector sproutEntries; + std::vector saplingEntries; + { + LOCK2(cs_main, pwalletMain->cs_wallet); + // Consider, should notes be sorted? + pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, "", 0); + } + if (sproutEntries.empty()) { // Consider, should the migration remain enabled? + fSaplingMigrationEnabled = false; + return true; + } + CAmount availableFunds = 0; + for (const CSproutNotePlaintextEntry& sproutEntry : sproutEntries) { + availableFunds = sproutEntry.plaintext.value(); + } + // If the remaining amount to be migrated is less than 0.01 ZEC, end the migration. + if (availableFunds < CENT) { + fSaplingMigrationEnabled = false; + return true; + } + + HDSeed seed; + if (!pwalletMain->GetHDSeed(seed)) { + throw JSONRPCError( + RPC_WALLET_ERROR, + "AsyncRPCOperation_AsyncRPCOperation_saplingmigration: HD seed not found"); + } + + libzcash::SaplingPaymentAddress migrationDestAddress = getMigrationDestAddress(seed); + + auto consensusParams = Params().GetConsensus(); + + // Up to the limit of 5, as many transactions are sent as are needed to migrate the remaining funds + int numTxCreated = 0; + int noteIndex = 0; + do { + CAmount amountToSend = chooseAmount(availableFunds); + auto builder = TransactionBuilder(consensusParams, targetHeight_, pwalletMain, pzcashParams); + std::vector fromNotes; + CAmount fromNoteAmount = 0; + while (fromNoteAmount < amountToSend) { + auto sproutEntry = sproutEntries[noteIndex++]; + fromNotes.push_back(sproutEntry); + fromNoteAmount += sproutEntry.plaintext.value(); + } + availableFunds -= fromNoteAmount; + for (const CSproutNotePlaintextEntry& sproutEntry : fromNotes) { + libzcash::SproutNote sproutNote = sproutEntry.plaintext.note(sproutEntry.address); + libzcash::SproutSpendingKey sproutSk; + pwalletMain->GetSproutSpendingKey(sproutEntry.address, sproutSk); + std::vector vOutPoints = {sproutEntry.jsop}; + // Each migration transaction SHOULD specify an anchor at height N-10 + // for each Sprout JoinSplit description + // TODO: the above functionality (in comment) is not implemented in zcashd + uint256 inputAnchor; + std::vector> vInputWitnesses; + pwalletMain->GetSproutNoteWitnesses(vOutPoints, vInputWitnesses, inputAnchor); + builder.AddSproutInput(sproutSk, sproutNote, vInputWitnesses[0].get()); + } + // The amount chosen *includes* the 0.0001 ZEC fee for this transaction, i.e. + // the value of the Sapling output will be 0.0001 ZEC less. + builder.SetFee(FEE); + builder.AddSaplingOutput(ovkForShieldingFromTaddr(seed), migrationDestAddress, amountToSend - FEE); + CTransaction tx = builder.Build().GetTxOrThrow(); + if (isCancelled()) { + break; + } + pwalletMain->AddPendingSaplingMigrationTx(tx); + ++numTxCreated; + } while (numTxCreated < 5 && availableFunds > CENT); + + UniValue res(UniValue::VOBJ); + res.push_back(Pair("num_tx_created", numTxCreated)); + set_result(res); + return true; +} + +CAmount AsyncRPCOperation_saplingmigration::chooseAmount(const CAmount& availableFunds) { + CAmount amount = 0; + do { + // 1. Choose an integer exponent uniformly in the range 6 to 8 inclusive. + int exponent = GetRand(3) + 6; + // 2. Choose an integer mantissa uniformly in the range 1 to 99 inclusive. + uint64_t mantissa = GetRand(99) + 1; + // 3. Calculate amount := (mantissa * 10^exponent) zatoshi. + int pow = std::pow(10, exponent); + amount = mantissa * pow; + // 4. If amount is greater than the amount remaining to send, repeat from step 1. + } while (amount > availableFunds); + return amount; +} + +// Unless otherwise specified, the migration destination address is the address for Sapling account 0 +libzcash::SaplingPaymentAddress AsyncRPCOperation_saplingmigration::getMigrationDestAddress(const HDSeed& seed) { + // Derive the address for Sapling account 0 + auto m = libzcash::SaplingExtendedSpendingKey::Master(seed); + uint32_t bip44CoinType = Params().BIP44CoinType(); + + // We use a fixed keypath scheme of m/32'/coin_type'/account' + // Derive m/32' + auto m_32h = m.Derive(32 | ZIP32_HARDENED_KEY_LIMIT); + // Derive m/32'/coin_type' + auto m_32h_cth = m_32h.Derive(bip44CoinType | ZIP32_HARDENED_KEY_LIMIT); + + // Derive account key at next index, skip keys already known to the wallet + libzcash::SaplingExtendedSpendingKey xsk = m_32h_cth.Derive(0 | ZIP32_HARDENED_KEY_LIMIT); + + libzcash::SaplingPaymentAddress toAddress = xsk.DefaultAddress(); + + // Refactor: this is similar logic as in the visitor HaveSpendingKeyForPaymentAddress and is used elsewhere + libzcash::SaplingIncomingViewingKey ivk; + libzcash::SaplingFullViewingKey fvk; + if (!(pwalletMain->GetSaplingIncomingViewingKey(toAddress, ivk) && + pwalletMain->GetSaplingFullViewingKey(ivk, fvk) && + pwalletMain->HaveSaplingSpendingKey(fvk))) { + // Sapling account 0 must be the first address returned by GenerateNewSaplingZKey + assert(pwalletMain->GenerateNewSaplingZKey() == toAddress); + } + + return toAddress; +} + +UniValue AsyncRPCOperation_saplingmigration::getStatus() const { + UniValue v = AsyncRPCOperation::getStatus(); + UniValue obj = v.get_obj(); + obj.push_back(Pair("method", "saplingmigration")); + obj.push_back(Pair("target_height", targetHeight_)); + return obj; +} diff --git a/src/wallet/asyncrpcoperation_saplingmigration.h b/src/wallet/asyncrpcoperation_saplingmigration.h new file mode 100644 index 000000000..b520db408 --- /dev/null +++ b/src/wallet/asyncrpcoperation_saplingmigration.h @@ -0,0 +1,31 @@ +#include "amount.h" +#include "asyncrpcoperation.h" +#include "univalue.h" +#include "zcash/Address.hpp" +#include "zcash/zip32.h" + +class AsyncRPCOperation_saplingmigration : public AsyncRPCOperation +{ +public: + AsyncRPCOperation_saplingmigration(int targetHeight); + virtual ~AsyncRPCOperation_saplingmigration(); + + // We don't want to be copied or moved around + AsyncRPCOperation_saplingmigration(AsyncRPCOperation_saplingmigration const&) = delete; // Copy construct + AsyncRPCOperation_saplingmigration(AsyncRPCOperation_saplingmigration&&) = delete; // Move construct + AsyncRPCOperation_saplingmigration& operator=(AsyncRPCOperation_saplingmigration const&) = delete; // Copy assign + AsyncRPCOperation_saplingmigration& operator=(AsyncRPCOperation_saplingmigration&&) = delete; // Move assign + + virtual void main(); + + virtual UniValue getStatus() const; + +private: + int targetHeight_; + + bool main_impl(); + + CAmount chooseAmount(const CAmount& availableFunds); + + libzcash::SaplingPaymentAddress getMigrationDestAddress(const HDSeed& seed); +}; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4275f897f..7cf5429a7 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3906,6 +3906,27 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) return operationId; } +UniValue z_setmigration(const UniValue& params, bool fHelp) { + if (!EnsureWalletIsAvailable(fHelp)) + return NullUniValue; + if (fHelp || params.size() != 1) + throw runtime_error( + "z_setmigration enabled\n" + "When enabled the Sprout to Sapling migration will attempt to migrate all funds from this wallet’s\n" + "Sprout addresses to either the address for Sapling account 0 or one specified by the parameter\n" + "'-migrationdestaddress'. This migration is designed to minimize information leakage. As a result,\n" + "for wallets with a significant Sprout balance, this process may take several weeks. The migration\n" + "works by sending, up to 5, as many transactions as possible whenever the blockchain reaches a height\n" + "equal to 499 modulo 500. The transaction amounts are picked according to the random distribution\n" + "specified in ZIP 308. The migration will end once the wallet’s Sprout balance is below 0.01 " + CURRENCY_UNIT + ".\n" + "\nArguments:\n" + "1. enabled (boolean, required) 'true' or 'false' to enable or disable respectively.\n" + "\nResult:\n" + "enabled (boolean) Whether or not migration is enabled (echos the argument).\n" + ); + fSaplingMigrationEnabled = params[0].get_bool(); + return fSaplingMigrationEnabled; +} /** When estimating the number of coinbase utxos we can shield in a single transaction: @@ -4660,6 +4681,7 @@ static const CRPCCommand commands[] = { "wallet", "z_gettotalbalance", &z_gettotalbalance, false }, { "wallet", "z_mergetoaddress", &z_mergetoaddress, false }, { "wallet", "z_sendmany", &z_sendmany, false }, + { "wallet", "z_setmigration", &z_setmigration, false }, { "wallet", "z_shieldcoinbase", &z_shieldcoinbase, false }, { "wallet", "z_getoperationstatus", &z_getoperationstatus, true }, { "wallet", "z_getoperationresult", &z_getoperationresult, true }, diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6dfa15025..4ddf5a073 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5,8 +5,10 @@ #include "wallet/wallet.h" +#include "asyncrpcqueue.h" #include "checkpoints.h" #include "coincontrol.h" +#include "core_io.h" #include "consensus/upgrades.h" #include "consensus/validation.h" #include "consensus/consensus.h" @@ -15,12 +17,14 @@ #include "main.h" #include "net.h" #include "rpc/protocol.h" +#include "rpc/server.h" #include "script/script.h" #include "script/sign.h" #include "timedata.h" #include "utilmoneystr.h" #include "zcash/Note.hpp" #include "crypter.h" +#include "wallet/asyncrpcoperation_saplingmigration.h" #include "zcash/zip32.h" #include @@ -32,6 +36,8 @@ using namespace std; using namespace libzcash; +extern UniValue sendrawtransaction(const UniValue& params, bool fHelp); + /** * Settings */ @@ -566,6 +572,47 @@ void CWallet::ChainTip(const CBlockIndex *pindex, UpdateSaplingNullifierNoteMapForBlock(pblock); } +void CWallet::RunSaplingMigration(int blockHeight) { + if (!NetworkUpgradeActive(blockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + return; + } + LOCK(cs_wallet); + // The migration transactions to be sent in a particular batch can take + // significant time to generate, and this time depends on the speed of the user's + // computer. If they were generated only after a block is seen at the target + // height minus 1, then this could leak information. Therefore, for target + // height N, implementations SHOULD start generating the transactions at around + // height N-5 + if (blockHeight % 500 == 495) { + if (saplingMigrationOperation != nullptr) { + saplingMigrationOperation->cancel(); + } + pendingSaplingMigrationTxs.clear(); + std::shared_ptr q = getAsyncRPCQueue(); + std::shared_ptr operation(new AsyncRPCOperation_saplingmigration(blockHeight + 5)); + saplingMigrationOperation = operation; + q->addOperation(operation); + } else if (blockHeight % 500 == 499) { + for (const CTransaction& transaction : pendingSaplingMigrationTxs) { + // The following is taken from z_sendmany/z_mergetoaddress + // Send the transaction + // TODO: Use CWallet::CommitTransaction instead of sendrawtransaction + auto signedtxn = EncodeHexTx(transaction); + UniValue params = UniValue(UniValue::VARR); + params.push_back(signedtxn); + UniValue sendResultValue = sendrawtransaction(params, false); + if (sendResultValue.isNull()) { + throw JSONRPCError(RPC_WALLET_ERROR, "sendrawtransaction did not return an error or a txid."); + } + } + pendingSaplingMigrationTxs.clear(); + } +} + +void CWallet::AddPendingSaplingMigrationTx(const CTransaction& tx) { + pendingSaplingMigrationTxs.push_back(tx); +} + void CWallet::SetBestChain(const CBlockLocator& loc) { CWalletDB walletdb(strWalletFile); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3f43e434e..9c2d60226 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -7,6 +7,7 @@ #define BITCOIN_WALLET_WALLET_H #include "amount.h" +#include "asyncrpcoperation.h" #include "coins.h" #include "key.h" #include "keystore.h" @@ -755,6 +756,9 @@ private: TxNullifiers mapTxSproutNullifiers; TxNullifiers mapTxSaplingNullifiers; + std::vector pendingSaplingMigrationTxs; + std::shared_ptr saplingMigrationOperation = nullptr; + void AddToTransparentSpends(const COutPoint& outpoint, const uint256& wtxid); void AddToSproutSpends(const uint256& nullifier, const uint256& wtxid); void AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid); @@ -1183,6 +1187,8 @@ public: CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const; CAmount GetChange(const CTransaction& tx) const; void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree, bool added); + void RunSaplingMigration(int blockHeight); + void AddPendingSaplingMigrationTx(const CTransaction& tx); /** Saves witness caches and best block locator to disk. */ void SetBestChain(const CBlockLocator& loc); std::set> GetNullifiersForAddresses(const std::set & addresses); From 162bfc3a1edac35bb803438dcc2f72e183122da9 Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Tue, 16 Apr 2019 19:05:15 -0600 Subject: [PATCH 2/7] Move migration logic to ChainTip --- src/main.cpp | 5 ----- src/main.h | 1 - src/validationinterface.cpp | 3 --- src/validationinterface.h | 3 --- .../asyncrpcoperation_saplingmigration.cpp | 4 ++-- src/wallet/rpcwallet.cpp | 5 +++-- src/wallet/wallet.cpp | 19 ++++++++++++++++--- src/wallet/wallet.h | 2 ++ 8 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 782ab69b0..72744b61a 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -78,7 +78,6 @@ bool fCoinbaseEnforcedProtectionEnabled = true; size_t nCoinCacheUsage = 5000 * 300; uint64_t nPruneTarget = 0; bool fAlerts = DEFAULT_ALERTS; -bool fSaplingMigrationEnabled = false; /* If the tip is older than this (in seconds), the node is considered to be in initial block download. */ int64_t nMaxTipAge = DEFAULT_MAX_TIP_AGE; @@ -3105,10 +3104,6 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock * EnforceNodeDeprecation(pindexNew->nHeight); - if (fSaplingMigrationEnabled) { - GetMainSignals().RunSaplingMigration(pindexNew->nHeight); - } - int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; LogPrint("bench", " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint("bench", "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); diff --git a/src/main.h b/src/main.h index 0e1fa6694..3b8c6aff9 100644 --- a/src/main.h +++ b/src/main.h @@ -154,7 +154,6 @@ extern bool fCoinbaseEnforcedProtectionEnabled; extern size_t nCoinCacheUsage; extern CFeeRate minRelayTxFee; extern bool fAlerts; -extern bool fSaplingMigrationEnabled; extern int64_t nMaxTipAge; /** Best header we've seen so far (used for getheaders queries' starting points). */ diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index bb3f800fc..1dc3351ab 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -18,7 +18,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.EraseTransaction.connect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4, _5)); - g_signals.RunSaplingMigration.connect(boost::bind(&CValidationInterface::RunSaplingMigration, pwalletIn, _1)); g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); @@ -34,7 +33,6 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4, _5)); - g_signals.RunSaplingMigration.disconnect(boost::bind(&CValidationInterface::RunSaplingMigration, pwalletIn, _1)); g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.EraseTransaction.disconnect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); @@ -49,7 +47,6 @@ void UnregisterAllValidationInterfaces() { g_signals.Broadcast.disconnect_all_slots(); g_signals.Inventory.disconnect_all_slots(); g_signals.ChainTip.disconnect_all_slots(); - g_signals.RunSaplingMigration.disconnect_all_slots(); g_signals.SetBestChain.disconnect_all_slots(); g_signals.UpdatedTransaction.disconnect_all_slots(); g_signals.EraseTransaction.disconnect_all_slots(); diff --git a/src/validationinterface.h b/src/validationinterface.h index 9e443d134..7b02bd9da 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -37,7 +37,6 @@ protected: virtual void SyncTransaction(const CTransaction &tx, const CBlock *pblock) {} virtual void EraseFromWallet(const uint256 &hash) {} virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree, bool added) {} - virtual void RunSaplingMigration(int blockHeight) {} virtual void SetBestChain(const CBlockLocator &locator) {} virtual void UpdatedTransaction(const uint256 &hash) {} virtual void Inventory(const uint256 &hash) {} @@ -61,8 +60,6 @@ struct CMainSignals { boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a change to the tip of the active block chain. */ boost::signals2::signal ChainTip; - /** Notifies listeners that they may need to update the status of the Sprout->Sapling migration */ - boost::signals2::signal RunSaplingMigration; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Notifies listeners about an inventory item being seen on the network. */ diff --git a/src/wallet/asyncrpcoperation_saplingmigration.cpp b/src/wallet/asyncrpcoperation_saplingmigration.cpp index 6a043f9bf..fd40fb74a 100644 --- a/src/wallet/asyncrpcoperation_saplingmigration.cpp +++ b/src/wallet/asyncrpcoperation_saplingmigration.cpp @@ -73,7 +73,7 @@ bool AsyncRPCOperation_saplingmigration::main_impl() { pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, "", 0); } if (sproutEntries.empty()) { // Consider, should the migration remain enabled? - fSaplingMigrationEnabled = false; + pwalletMain->fSaplingMigrationEnabled = false; return true; } CAmount availableFunds = 0; @@ -82,7 +82,7 @@ bool AsyncRPCOperation_saplingmigration::main_impl() { } // If the remaining amount to be migrated is less than 0.01 ZEC, end the migration. if (availableFunds < CENT) { - fSaplingMigrationEnabled = false; + pwalletMain->fSaplingMigrationEnabled = false; return true; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7cf5429a7..ea70b0a91 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3924,8 +3924,9 @@ UniValue z_setmigration(const UniValue& params, bool fHelp) { "\nResult:\n" "enabled (boolean) Whether or not migration is enabled (echos the argument).\n" ); - fSaplingMigrationEnabled = params[0].get_bool(); - return fSaplingMigrationEnabled; + LOCK(pwalletMain->cs_wallet); + pwalletMain->fSaplingMigrationEnabled = params[0].get_bool(); + return pwalletMain->fSaplingMigrationEnabled; } /** diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4ddf5a073..9c3107436 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -558,6 +558,15 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; } +void CWallet::ChainTipAdded(const CBlockIndex *pindex, + const CBlock *pblock, + SproutMerkleTree sproutTree, + SaplingMerkleTree saplingTree) +{ + IncrementNoteWitnesses(pindex, pblock, sproutTree, saplingTree); + UpdateSaplingNullifierNoteMapForBlock(pblock); +} + void CWallet::ChainTip(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, @@ -565,11 +574,12 @@ void CWallet::ChainTip(const CBlockIndex *pindex, bool added) { if (added) { - IncrementNoteWitnesses(pindex, pblock, sproutTree, saplingTree); + ChainTipAdded(pindex, pblock, sproutTree, saplingTree); + RunSaplingMigration(pindex->nHeight); } else { DecrementNoteWitnesses(pindex); + UpdateSaplingNullifierNoteMapForBlock(pblock); } - UpdateSaplingNullifierNoteMapForBlock(pblock); } void CWallet::RunSaplingMigration(int blockHeight) { @@ -577,6 +587,9 @@ void CWallet::RunSaplingMigration(int blockHeight) { return; } LOCK(cs_wallet); + if (!fSaplingMigrationEnabled) { + return; + } // The migration transactions to be sent in a particular batch can take // significant time to generate, and this time depends on the speed of the user's // computer. If they were generated only after a block is seen at the target @@ -2491,7 +2504,7 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) } } // Increment note witness caches - ChainTip(pindex, &block, sproutTree, saplingTree, true); + ChainTipAdded(pindex, &block, sproutTree, saplingTree); pindex = chainActive.Next(pindex); if (GetTime() >= nNow + 60) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9c2d60226..7cd808b62 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -771,6 +771,7 @@ public: * incremental witness cache in any transaction in mapWallet. */ int64_t nWitnessCacheSize; + bool fSaplingMigrationEnabled = false; void ClearNoteWitnessCache(); @@ -836,6 +837,7 @@ protected: private: template void SyncMetaData(std::pair::iterator, typename TxSpendMap::iterator>); + void ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree); protected: bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx); From 699288b4b4330996dec6d356d405774e6751a9b2 Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Tue, 16 Apr 2019 19:12:53 -0600 Subject: [PATCH 3/7] Documentation cleanup --- qa/rpc-tests/sprout_sapling_migration.py | 2 +- .../asyncrpcoperation_saplingmigration.cpp | 2 +- src/wallet/rpcwallet.cpp | 18 +++++++++--------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/qa/rpc-tests/sprout_sapling_migration.py b/qa/rpc-tests/sprout_sapling_migration.py index 726e5e2aa..b888fb8e2 100755 --- a/qa/rpc-tests/sprout_sapling_migration.py +++ b/qa/rpc-tests/sprout_sapling_migration.py @@ -41,7 +41,7 @@ class SproutSaplingMigration(BitcoinTestFramework): assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('0')) # Migrate - assert_equal(True, self.nodes[0].z_setmigration(True)) + self.nodes[0].z_setmigration(True) print "Mining to block 494..." self.nodes[0].generate(392) # 102 -> 494 self.sync_all() diff --git a/src/wallet/asyncrpcoperation_saplingmigration.cpp b/src/wallet/asyncrpcoperation_saplingmigration.cpp index fd40fb74a..22157c682 100644 --- a/src/wallet/asyncrpcoperation_saplingmigration.cpp +++ b/src/wallet/asyncrpcoperation_saplingmigration.cpp @@ -169,7 +169,7 @@ libzcash::SaplingPaymentAddress AsyncRPCOperation_saplingmigration::getMigration // Derive m/32'/coin_type' auto m_32h_cth = m_32h.Derive(bip44CoinType | ZIP32_HARDENED_KEY_LIMIT); - // Derive account key at next index, skip keys already known to the wallet + // Derive m/32'/coin_type'/0' libzcash::SaplingExtendedSpendingKey xsk = m_32h_cth.Derive(0 | ZIP32_HARDENED_KEY_LIMIT); libzcash::SaplingPaymentAddress toAddress = xsk.DefaultAddress(); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ea70b0a91..347d65b18 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3913,20 +3913,20 @@ UniValue z_setmigration(const UniValue& params, bool fHelp) { throw runtime_error( "z_setmigration enabled\n" "When enabled the Sprout to Sapling migration will attempt to migrate all funds from this wallet’s\n" - "Sprout addresses to either the address for Sapling account 0 or one specified by the parameter\n" - "'-migrationdestaddress'. This migration is designed to minimize information leakage. As a result,\n" - "for wallets with a significant Sprout balance, this process may take several weeks. The migration\n" - "works by sending, up to 5, as many transactions as possible whenever the blockchain reaches a height\n" - "equal to 499 modulo 500. The transaction amounts are picked according to the random distribution\n" - "specified in ZIP 308. The migration will end once the wallet’s Sprout balance is below 0.01 " + CURRENCY_UNIT + ".\n" + "Sprout addresses to either the address for Sapling account 0 or the address specified by the parameter\n" + "'-migrationdestaddress'.\n" + "\n" + "This migration is designed to minimize information leakage. As a result for wallets with a significant\n" + "Sprout balance, this process may take several weeks. The migration works by sending, up to 5, as many\n" + "transactions as possible whenever the blockchain reaches a height equal to 499 modulo 500. The transaction\n" + "amounts are picked according to the random distribution specified in ZIP 308. The migration will end once\n" + "the wallet’s Sprout balance is below" + strprintf("%s %s", FormatMoney(CENT), CURRENCY_UNIT) + ".\n" "\nArguments:\n" "1. enabled (boolean, required) 'true' or 'false' to enable or disable respectively.\n" - "\nResult:\n" - "enabled (boolean) Whether or not migration is enabled (echos the argument).\n" ); LOCK(pwalletMain->cs_wallet); pwalletMain->fSaplingMigrationEnabled = params[0].get_bool(); - return pwalletMain->fSaplingMigrationEnabled; + return NullUniValue; } /** From 7c4ad6e2986907e621496cd992010b264b37c63c Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Tue, 16 Apr 2019 19:35:04 -0600 Subject: [PATCH 4/7] Additional locking and race condition prevention --- src/wallet/wallet.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9c3107436..07fafe42f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -606,6 +606,9 @@ void CWallet::RunSaplingMigration(int blockHeight) { saplingMigrationOperation = operation; q->addOperation(operation); } else if (blockHeight % 500 == 499) { + if (saplingMigrationOperation != nullptr) { + saplingMigrationOperation->cancel(); + } for (const CTransaction& transaction : pendingSaplingMigrationTxs) { // The following is taken from z_sendmany/z_mergetoaddress // Send the transaction @@ -623,6 +626,7 @@ void CWallet::RunSaplingMigration(int blockHeight) { } void CWallet::AddPendingSaplingMigrationTx(const CTransaction& tx) { + LOCK(cs_wallet); pendingSaplingMigrationTxs.push_back(tx); } From d2a584e35a1f85e28919b4b74bb97a6da3abaa9d Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Thu, 18 Apr 2019 15:41:27 -0600 Subject: [PATCH 5/7] Refactor wait_and_assert_operationid_status to allow returning the result --- qa/rpc-tests/sprout_sapling_migration.py | 12 +++++--- qa/rpc-tests/test_framework/util.py | 35 ++++++++++++------------ qa/rpc-tests/wallet_protectcoinbase.py | 4 +-- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/qa/rpc-tests/sprout_sapling_migration.py b/qa/rpc-tests/sprout_sapling_migration.py index b888fb8e2..0e606fb5e 100755 --- a/qa/rpc-tests/sprout_sapling_migration.py +++ b/qa/rpc-tests/sprout_sapling_migration.py @@ -8,7 +8,8 @@ import sys; assert sys.version_info < (3,), ur"This script does not run under Py from decimal import Decimal from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_true, get_coinbase_address, \ - initialize_chain_clean, start_nodes, wait_and_assert_operationid_status + initialize_chain_clean, start_nodes, wait_and_assert_operationid_status, \ + wait_and_assert_operationid_status_result class SproutSaplingMigration(BitcoinTestFramework): @@ -54,14 +55,17 @@ class SproutSaplingMigration(BitcoinTestFramework): # At 495 we should have an async operation operationstatus = self.nodes[0].z_getoperationstatus() + print "migration operation: {}".format(operationstatus) assert_equal(1, len(operationstatus), "num async operations at 495") assert_equal('saplingmigration', operationstatus[0]['method']) assert_equal(500, operationstatus[0]['target_height']) - print "migration operation: {}".format(operationstatus) - migration_opid = operationstatus[0]['id'] - result = wait_and_assert_operationid_status(self.nodes[0], migration_opid) + result = wait_and_assert_operationid_status_result(self.nodes[0], operationstatus[0]['id']) print "result: {}".format(result) + assert_equal('saplingmigration', result['method']) + assert_equal(500, result['target_height']) + assert_equal(1, result['result']['num_tx_created']) + assert_equal(0, len(self.nodes[0].getrawmempool()), "mempool size at 495") self.nodes[0].generate(3) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 7d15ec063..590cb2d48 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -385,8 +385,9 @@ def assert_raises(exc, fun, *args, **kwds): def fail(message=""): raise AssertionError(message) -# Returns txid if operation was a success or None -def wait_and_assert_operationid_status(node, myopid, in_status='success', in_errormsg=None, timeout=300): + +# Returns an async operation result +def wait_and_assert_operationid_status_result(node, myopid, in_status='success', in_errormsg=None, timeout=300): print('waiting for async operation {}'.format(myopid)) result = None for _ in xrange(1, timeout): @@ -399,29 +400,29 @@ def wait_and_assert_operationid_status(node, myopid, in_status='success', in_err assert_true(result is not None, "timeout occured") status = result['status'] - ret = None + debug = os.getenv("PYTHON_DEBUG", "") + if debug: + print('...returned status: {}'.format(status)) + errormsg = None if status == "failed": errormsg = result['error']['message'] - elif status == "success": - if type(result['result']) is dict and result['result'].get('txid'): - ret = result['result']['txid'] - else: - ret = result['result'] - - if os.getenv("PYTHON_DEBUG", ""): - print('...returned status: {}'.format(status)) - if errormsg is not None: + if debug: print('...returned error: {}'.format(errormsg)) + assert_equal(in_errormsg, errormsg) assert_equal(in_status, status, "Operation returned mismatched status. Error Message: {}".format(errormsg)) - if errormsg is not None: - assert_true(in_errormsg is not None, "No error retured. Expected: {}".format(errormsg)) - assert_true(in_errormsg in errormsg, "Error returned: {}. Error expected: {}".format(errormsg, in_errormsg)) - return result # if there was an error return the result + return result + + +# Returns txid if operation was a success or None +def wait_and_assert_operationid_status(node, myopid, in_status='success', in_errormsg=None, timeout=300): + result = wait_and_assert_operationid_status_result(node, myopid, in_status, in_errormsg, timeout) + if result['status'] == "success": + return result['result']['txid'] else: - return ret # otherwise return the txid + return None # Find a coinbase address on the node, filtering by the number of UTXOs it has. # If no filter is provided, returns the coinbase address on the node containing diff --git a/qa/rpc-tests/wallet_protectcoinbase.py b/qa/rpc-tests/wallet_protectcoinbase.py index 851099311..5eaed7c3a 100755 --- a/qa/rpc-tests/wallet_protectcoinbase.py +++ b/qa/rpc-tests/wallet_protectcoinbase.py @@ -10,7 +10,7 @@ from test_framework.authproxy import JSONRPCException from test_framework.mininode import COIN from test_framework.util import assert_equal, initialize_chain_clean, \ start_nodes, connect_nodes_bi, wait_and_assert_operationid_status, \ - get_coinbase_address + wait_and_assert_operationid_status_result, get_coinbase_address import sys import timeit @@ -92,7 +92,7 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): recipients.append({"address":myzaddr, "amount":Decimal('1.23456789')}) myopid = self.nodes[0].z_sendmany(mytaddr, recipients) - error_result = wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "wallet does not allow any change", 10) + error_result = wait_and_assert_operationid_status_result(self.nodes[0], myopid, "failed", "wallet does not allow any change", 10) # Test that the returned status object contains a params field with the operation's input parameters assert_equal(error_result["method"], "z_sendmany") From 5519d16997c07c02932804e17dc23625be656e8b Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Thu, 25 Apr 2019 13:43:23 -0600 Subject: [PATCH 6/7] Set min depth when selecting notes to migrate --- qa/rpc-tests/sprout_sapling_migration.py | 1 + .../asyncrpcoperation_saplingmigration.cpp | 18 ++++++++++-------- .../asyncrpcoperation_saplingmigration.h | 2 ++ 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/qa/rpc-tests/sprout_sapling_migration.py b/qa/rpc-tests/sprout_sapling_migration.py index 0e606fb5e..13dc21890 100755 --- a/qa/rpc-tests/sprout_sapling_migration.py +++ b/qa/rpc-tests/sprout_sapling_migration.py @@ -94,6 +94,7 @@ class SproutSaplingMigration(BitcoinTestFramework): print "sprout balance: {}, sapling balance: {}".format(sprout_balance, sapling_balance) assert_true(sprout_balance < Decimal('10'), "Should have less Sprout funds") assert_true(sapling_balance > Decimal('0'), "Should have more Sapling funds") + assert_true(sprout_balance + sapling_balance, Decimal('9.9999')) if __name__ == '__main__': diff --git a/src/wallet/asyncrpcoperation_saplingmigration.cpp b/src/wallet/asyncrpcoperation_saplingmigration.cpp index 22157c682..85962f75a 100644 --- a/src/wallet/asyncrpcoperation_saplingmigration.cpp +++ b/src/wallet/asyncrpcoperation_saplingmigration.cpp @@ -54,7 +54,7 @@ void AsyncRPCOperation_saplingmigration::main() { set_state(OperationStatus::FAILED); } - std::string s = strprintf("%s: Sprout->Sapling transactions sent. (status=%s", getId(), getStateAsString()); + std::string s = strprintf("%s: Sprout->Sapling transactions created. (status=%s", getId(), getStateAsString()); if (success) { s += strprintf(", success)\n"); } else { @@ -69,12 +69,10 @@ bool AsyncRPCOperation_saplingmigration::main_impl() { std::vector saplingEntries; { LOCK2(cs_main, pwalletMain->cs_wallet); + // We set minDepth to 11 to avoid unconfirmed notes and in anticipation of specifying + // an anchor at height N-10 for each Sprout JoinSplit description // Consider, should notes be sorted? - pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, "", 0); - } - if (sproutEntries.empty()) { // Consider, should the migration remain enabled? - pwalletMain->fSaplingMigrationEnabled = false; - return true; + pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, "", 11); } CAmount availableFunds = 0; for (const CSproutNotePlaintextEntry& sproutEntry : sproutEntries) { @@ -82,7 +80,7 @@ bool AsyncRPCOperation_saplingmigration::main_impl() { } // If the remaining amount to be migrated is less than 0.01 ZEC, end the migration. if (availableFunds < CENT) { - pwalletMain->fSaplingMigrationEnabled = false; + setMigrationResult(0); return true; } @@ -136,10 +134,14 @@ bool AsyncRPCOperation_saplingmigration::main_impl() { ++numTxCreated; } while (numTxCreated < 5 && availableFunds > CENT); + setMigrationResult(numTxCreated); + return true; +} + +void AsyncRPCOperation_saplingmigration::setMigrationResult(int numTxCreated) { UniValue res(UniValue::VOBJ); res.push_back(Pair("num_tx_created", numTxCreated)); set_result(res); - return true; } CAmount AsyncRPCOperation_saplingmigration::chooseAmount(const CAmount& availableFunds) { diff --git a/src/wallet/asyncrpcoperation_saplingmigration.h b/src/wallet/asyncrpcoperation_saplingmigration.h index b520db408..af26281f6 100644 --- a/src/wallet/asyncrpcoperation_saplingmigration.h +++ b/src/wallet/asyncrpcoperation_saplingmigration.h @@ -25,6 +25,8 @@ private: bool main_impl(); + void setMigrationResult(int numTxCreated); + CAmount chooseAmount(const CAmount& availableFunds); libzcash::SaplingPaymentAddress getMigrationDestAddress(const HDSeed& seed); From 6acb37bcee606aac23a8895ef4a7b2bbc32b3634 Mon Sep 17 00:00:00 2001 From: Eirik0 Date: Fri, 26 Apr 2019 19:01:23 -0600 Subject: [PATCH 7/7] Check for full failure message in test case --- qa/rpc-tests/wallet_protectcoinbase.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/wallet_protectcoinbase.py b/qa/rpc-tests/wallet_protectcoinbase.py index 5eaed7c3a..e7a3b9e5a 100755 --- a/qa/rpc-tests/wallet_protectcoinbase.py +++ b/qa/rpc-tests/wallet_protectcoinbase.py @@ -84,7 +84,7 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): recipients= [{"address":myzaddr, "amount": Decimal('1')}] myopid = self.nodes[3].z_sendmany(mytaddr, recipients) - wait_and_assert_operationid_status(self.nodes[3], myopid, "failed", "no UTXOs found for taddr from address", 10) + wait_and_assert_operationid_status(self.nodes[3], myopid, "failed", "Insufficient funds, no UTXOs found for taddr from address.", 10) # This send will fail because our wallet does not allow any change when protecting a coinbase utxo, # as it's currently not possible to specify a change address in z_sendmany. @@ -92,7 +92,9 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): recipients.append({"address":myzaddr, "amount":Decimal('1.23456789')}) myopid = self.nodes[0].z_sendmany(mytaddr, recipients) - error_result = wait_and_assert_operationid_status_result(self.nodes[0], myopid, "failed", "wallet does not allow any change", 10) + error_result = wait_and_assert_operationid_status_result(self.nodes[0], myopid, "failed", ("Change 8.76533211 not allowed. " + "When shielding coinbase funds, the wallet does not allow any change " + "as there is currently no way to specify a change address in z_sendmany."), 10) # Test that the returned status object contains a params field with the operation's input parameters assert_equal(error_result["method"], "z_sendmany")