From cd10562c73a773e7926118d677e7fafb34ff1b87 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 11 Oct 2018 21:25:53 -0700 Subject: [PATCH 1/7] Resolves Sapling nullifier persistence issue when importing a key. During a rescan, a CWalletTx was persisted to disk before it had its note data set. This meant that upon restart, the CWalletTx would potentially be missing its nullifiers causing the wallet's balance to include notes which had already been spent. The resolution is to ensure that after a rescan, a CWalletTx is persisted after it has had its nullifiers set correctly. Co-authored-by: Eirik Ogilvie-Wigley --- src/wallet/wallet.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3690ed166..b9da65454 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2398,6 +2398,9 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) const CChainParams& chainParams = Params(); CBlockIndex* pindex = pindexStart; + + std::vector myTxHashes; + { LOCK2(cs_main, cs_wallet); @@ -2418,8 +2421,10 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) ReadBlockFromDisk(block, pindex); BOOST_FOREACH(CTransaction& tx, block.vtx) { - if (AddToWalletIfInvolvingMe(tx, &block, fUpdate)) + if (AddToWalletIfInvolvingMe(tx, &block, fUpdate)) { + myTxHashes.push_back(tx.GetHash()); ret++; + } } SproutMerkleTree sproutTree; @@ -2441,6 +2446,19 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex)); } } + + // After rescanning, persist Sapling note data that might have changed, e.g. nullifiers. + // Do not flush the wallet here for performance reasons. + CWalletDB walletdb(strWalletFile, "r+", false); + for (auto hash : myTxHashes) { + CWalletTx wtx = mapWallet[hash]; + if (!wtx.mapSaplingNoteData.empty()) { + if (!wtx.WriteToDisk(&walletdb)) { + LogPrintf("Rescanning... WriteToDisk failed to update Sapling note data for: %s\n", hash.ToString()); + } + } + } + ShowProgress(_("Rescanning..."), 100); // hide progress dialog in GUI } return ret; From 01918e8742028846259f0b1389b17179a8df92f9 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 11 Oct 2018 21:52:56 -0700 Subject: [PATCH 2/7] Update test to verify Sapling nullifiers and witnesses persist correctly. --- qa/rpc-tests/wallet_persistence.py | 36 +++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/wallet_persistence.py b/qa/rpc-tests/wallet_persistence.py index 612226275..f6c227d2a 100755 --- a/qa/rpc-tests/wallet_persistence.py +++ b/qa/rpc-tests/wallet_persistence.py @@ -16,15 +16,16 @@ class WalletPersistenceTest (BitcoinTestFramework): def setup_chain(self): print("Initializing test directory " + self.options.tmpdir) - initialize_chain_clean(self.options.tmpdir, 2) + initialize_chain_clean(self.options.tmpdir, 3) def setup_network(self, split=False): - self.nodes = start_nodes(2, self.options.tmpdir, + self.nodes = start_nodes(3, self.options.tmpdir, extra_args=[[ '-nuparams=5ba81b19:100', # Overwinter '-nuparams=76b809bb:201', # Sapling - ]] * 2) + ]] * 3) connect_nodes_bi(self.nodes,0,1) + connect_nodes_bi(self.nodes,1,2) self.is_network_split=False self.sync_all() @@ -97,5 +98,34 @@ class WalletPersistenceTest (BitcoinTestFramework): assert_equal(self.nodes[0].z_getbalance(sapling_addr), Decimal('5')) assert_equal(self.nodes[1].z_getbalance(dest_addr), Decimal('15')) + # Verify importing a spending key will update and persist the nullifiers and witnesses correctly + sk0 = self.nodes[0].z_exportkey(sapling_addr) + self.nodes[2].z_importkey(sk0, "yes") + assert_equal(self.nodes[2].z_getbalance(sapling_addr), Decimal('5')) + + # Restart the nodes + stop_nodes(self.nodes) + wait_bitcoinds() + self.setup_network() + + # Verify nullifiers persisted correctly by checking balance + # Prior to PR #3590, there will be an error as spent notes are considered unspent: + # Assertion failed: expected: <25.00000000> but was: <5> + assert_equal(self.nodes[2].z_getbalance(sapling_addr), Decimal('5')) + + # Verity witnesses persisted correctly by sending shielded funds + recipients = [] + recipients.append({"address": dest_addr, "amount": Decimal('1')}) + myopid = self.nodes[2].z_sendmany(sapling_addr, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[2], myopid) + + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + # Verify balances + assert_equal(self.nodes[2].z_getbalance(sapling_addr), Decimal('4')) + assert_equal(self.nodes[1].z_getbalance(dest_addr), Decimal('16')) + if __name__ == '__main__': WalletPersistenceTest().main() \ No newline at end of file From 0917c84d9ad7478a6619eef7c1cc817a8e7a9791 Mon Sep 17 00:00:00 2001 From: Eirik Ogilvie-Wigley Date: Fri, 12 Oct 2018 14:12:21 -0600 Subject: [PATCH 3/7] Better error message when sending to both sprout and sapling Co-authored-by mdr0id --- qa/rpc-tests/wallet_sapling.py | 14 ++++++++++++++ src/wallet/rpcwallet.cpp | 13 +++++++++++++ 2 files changed, 27 insertions(+) diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py index c3b17af50..1aecba5d3 100755 --- a/qa/rpc-tests/wallet_sapling.py +++ b/qa/rpc-tests/wallet_sapling.py @@ -4,6 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. from test_framework.test_framework import BitcoinTestFramework +from test_framework.authproxy import JSONRPCException from test_framework.util import ( assert_equal, start_nodes, @@ -144,5 +145,18 @@ class WalletSaplingTest(BitcoinTestFramework): self.nodes[2].z_importkey(sk1, "yes") assert_equal(self.nodes[2].z_getbalance(saplingAddr1), Decimal('5')) + # Make sure we get a useful error when trying to send to both sprout and sapling + node4_sproutaddr = self.nodes[3].z_getnewaddress('sprout') + node4_saplingaddr = self.nodes[3].z_getnewaddress('sapling') + try: + self.nodes[1].z_sendmany( + taddr1, + [{'address': node4_sproutaddr, 'amount': 2.5}, {'address': node4_saplingaddr, 'amount': 2.4999}], + 1, 0.0001 + ) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Cannot send to both Sprout and Sapling addresses using z_sendmany", e.error['message']) + if __name__ == '__main__': WalletSaplingTest().main() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b85ab8f89..4bbf6271c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3687,6 +3687,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) std::vector zaddrRecipients; CAmount nTotalOut = 0; + bool containsSproutOutput = false; + bool containsSaplingOutput = false; + for (const UniValue& o : outputs.getValues()) { if (!o.isObject()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected object"); @@ -3710,6 +3713,16 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) bool toSprout = !toSapling; noSproutAddrs = noSproutAddrs && toSapling; + containsSproutOutput |= toSprout; + containsSaplingOutput |= toSapling; + + // Sending to both Sprout and Sapling is currently unsupported using z_sendmany + if (containsSproutOutput && containsSaplingOutput) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Cannot send to both Sprout and Sapling addresses using z_sendmany"); + } + // If we are sending from a shielded address, all recipient // shielded addresses must be of the same type. if (fromSprout && toSapling) { From de1b86a429204f87dc184f3f75a6beebe9bb6f57 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 12 Oct 2018 22:21:00 -0700 Subject: [PATCH 4/7] For #3359. RPCs transferring funds return error if Sapling addresses are used before Sapling activation. --- qa/rpc-tests/wallet_sapling.py | 31 +++++++++++++++++++++++++++++-- src/wallet/rpcwallet.cpp | 20 ++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py index 1aecba5d3..3c8809a1d 100755 --- a/qa/rpc-tests/wallet_sapling.py +++ b/qa/rpc-tests/wallet_sapling.py @@ -19,17 +19,44 @@ class WalletSaplingTest(BitcoinTestFramework): def setup_nodes(self): return start_nodes(4, self.options.tmpdir, [[ '-nuparams=5ba81b19:201', # Overwinter - '-nuparams=76b809bb:201', # Sapling + '-nuparams=76b809bb:203', # Sapling ]] * 4) def run_test(self): # Sanity-check the test harness assert_equal(self.nodes[0].getblockcount(), 200) - # Activate Sapling + # Activate Overwinter self.nodes[2].generate(1) self.sync_all() + # Verify RPCs disallow Sapling value transfer if Sapling is not active + tmp_taddr = self.nodes[3].getnewaddress() + tmp_zaddr = self.nodes[3].z_getnewaddress('sapling') + try: + recipients = [] + recipients.append({"address": tmp_zaddr, "amount": Decimal('20')}) + self.nodes[3].z_sendmany(tmp_taddr, recipients, 1, 0) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + try: + recipients = [] + recipients.append({"address": tmp_taddr, "amount": Decimal('20')}) + self.nodes[3].z_sendmany(tmp_zaddr, recipients, 1, 0) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + try: + self.nodes[3].z_shieldcoinbase(tmp_taddr, tmp_zaddr) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + + # Activate Sapling + self.nodes[2].generate(2) + self.sync_all() + taddr0 = self.nodes[0].getnewaddress() # Skip over the address containing node 1's coinbase self.nodes[1].getnewaddress() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4bbf6271c..508c6dacc 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3795,6 +3795,13 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } } + // If Sapling is not active, do not allow sending from or sending to Sapling addresses. + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + if (fromSapling || containsSaplingOutput) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated"); + } + } + // As a sanity check, estimate and verify that the size of the transaction will be valid. // Depending on the input notes, the actual tx size may turn out to be larger and perhaps invalid. size_t txsize = 0; @@ -3984,6 +3991,19 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } + // If Sapling is not active, do not allow sending to a Sapling address. + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + auto res = DecodePaymentAddress(destaddress); + if (IsValidPaymentAddress(res)) { + bool toSapling = boost::get(&res) != nullptr; + if (toSapling) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated"); + } + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); + } + } + // Prepare to get coinbase utxos std::vector inputs; CAmount shieldedValue = 0; From 61caa4661913c2971e6348ce9354bad9d7f88b55 Mon Sep 17 00:00:00 2001 From: Simon Date: Sat, 13 Oct 2018 08:10:10 -0700 Subject: [PATCH 5/7] For #3359. Return error if Sapling addresses passed to RPC z_mergetoaddress. RPC z_mergetoaddress does not support Sapling yet but the existing error reporting was not clear to users. --- qa/rpc-tests/wallet_sapling.py | 13 +++++++++++++ src/wallet/rpcwallet.cpp | 26 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py index 3c8809a1d..1abb1e8dc 100755 --- a/qa/rpc-tests/wallet_sapling.py +++ b/qa/rpc-tests/wallet_sapling.py @@ -20,6 +20,7 @@ class WalletSaplingTest(BitcoinTestFramework): return start_nodes(4, self.options.tmpdir, [[ '-nuparams=5ba81b19:201', # Overwinter '-nuparams=76b809bb:203', # Sapling + '-experimentalfeatures', '-zmergetoaddress', ]] * 4) def run_test(self): @@ -53,6 +54,18 @@ class WalletSaplingTest(BitcoinTestFramework): except JSONRPCException as e: assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + # Verify z_mergetoaddress RPC does not support Sapling yet + try: + self.nodes[3].z_mergetoaddress([tmp_taddr], tmp_zaddr) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling is not supported yet by z_mergetoadress", e.error['message']) + try: + self.nodes[3].z_mergetoaddress([tmp_zaddr], tmp_taddr) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling is not supported yet by z_mergetoadress", e.error['message']) + # Activate Sapling self.nodes[2].generate(2) self.sync_all() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 508c6dacc..d6940aeb7 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4204,6 +4204,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) std::set setAddress; // Sources + bool containsSaplingZaddrSource = false; for (const UniValue& o : addresses.getValues()) { if (!o.isStr()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected string"); @@ -4229,6 +4230,9 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) if (!(useAny || useAnyNote)) { zaddrs.insert(zaddr); } + // Check if z-addr is Sapling + bool isSapling = boost::get(&zaddr) != nullptr; + containsSaplingZaddrSource |= isSapling; } else { throw JSONRPCError( RPC_INVALID_PARAMETER, @@ -4245,10 +4249,19 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) // Validate the destination address auto destaddress = params[1].get_str(); bool isToZaddr = false; + bool isToSaplingZaddr = false; CTxDestination taddr = DecodeDestination(destaddress); if (!IsValidDestination(taddr)) { if (IsValidPaymentAddressString(destaddress)) { isToZaddr = true; + + // Is this a Sapling address? + auto res = DecodePaymentAddress(destaddress); + if (IsValidPaymentAddress(res)) { + isToSaplingZaddr = boost::get(&res) != nullptr; + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); + } } else { throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); } @@ -4302,6 +4315,19 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } + // This RPC does not support Sapling yet. + if (isToSaplingZaddr || containsSaplingZaddrSource) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling is not supported yet by z_mergetoadress"); + } + + // If this RPC does support Sapling... + // If Sapling is not active, do not allow sending from or sending to Sapling addresses. + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + if (isToSaplingZaddr || containsSaplingZaddrSource) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated"); + } + } + // Prepare to get UTXOs and notes std::vector utxoInputs; std::vector noteInputs; From 85809c75ad61197085590baff80952d2e489a406 Mon Sep 17 00:00:00 2001 From: Simon Date: Sat, 13 Oct 2018 08:55:38 -0700 Subject: [PATCH 6/7] Update mainnet checkpoint for block 410100. --- src/chainparams.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 8ebf04c6f..7cf888fa7 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -180,11 +180,12 @@ public: (180000, uint256S("0x000000001205b742eac4a1b3959635bdf8aeada078d6a996df89740f7b54351d")) (222222, uint256S("0x000000000cafb9e56445a6cabc8057b57ee6fcc709e7adbfa195e5c7fac61343")) (270000, uint256S("0x00000000025c1cfa0258e33ab050aaa9338a3d4aaa3eb41defefc887779a9729")) - (304600, uint256S("0x00000000028324e022a45014c4a4dc51e95d41e6bceb6ad554c5b65d5cea3ea5")), - 1523506583, // * UNIX timestamp of last checkpoint block - 2824682, // * total number of transactions between genesis and last checkpoint + (304600, uint256S("0x00000000028324e022a45014c4a4dc51e95d41e6bceb6ad554c5b65d5cea3ea5")) + (410100, uint256S("0x0000000002c565958f783a24a4ac17cde898ff525e75ed9baf66861b0b9fcada")), + 1539405939, // * UNIX timestamp of last checkpoint block + 3954156, // * total number of transactions between genesis and last checkpoint // (the tx=... number in the SetBestChain debug.log lines) - 5341 // * estimated number of transactions per day after checkpoint + 5553 // * estimated number of transactions per day after checkpoint // total number of tx / (checkpoint block height / (24 * 24)) }; From cfcea81238c5acd039b208c572e847937190fe92 Mon Sep 17 00:00:00 2001 From: Simon Date: Sun, 14 Oct 2018 09:23:49 -0700 Subject: [PATCH 7/7] Update release notes for v2.0.1 --- doc/release-notes.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index 6cdd058ac..0ae3ea63a 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -4,6 +4,16 @@ release-notes at release time) Notable changes =============== +Enabled Sapling features for mainnet +------------------------------------ +This release adds significant support for Sapling to the wallet and RPC interface. Sapling will activate at block 419200, which is expected to be mined on the 28th of October 2018. Users running v2.0.0 nodes (which are consensus-compatible with Sapling) will follow the network upgrade, but must upgrade to v2.0.1 in order to send Sapling shielded transactions. + +Minimum Difficulty Blocks allowed on testnet +-------------------------------------------- +Sapling activated on testnet at block 280000. Users running v2.0.1 nodes no longer need to specify `-experimentalfeatures` and `-developersapling` to use Sapling functionality on testnet. Users running v2.0.0 nodes should upgrade to v2.0.1 which introduces a consensus rule change to allow minimum difficulty blocks to be mined from block 299188, thereby splitting the chain. + +[Pull request](https://github.com/zcash/zcash/pull/3559) + Hierarchical Deterministic Key Generation for Sapling ----------------------------------------------------- All Sapling addresses will use hierarchical deterministic key generation @@ -17,3 +27,9 @@ Regular backups are still necessary, however, in order to ensure that transparent and Sprout addresses are not lost. [Pull request](https://github.com/zcash/zcash/pull/3492), [ZIP 32](https://github.com/zcash/zips/blob/master/zip-0032.mediawiki) + +Fix Signing Raw Transactions with Unsynced Offline Nodes +-------------------------------------------------------- +With v2.0.0, in `signrawtransaction` the consensus branch ID (which is used to construct the transaction) was estimated using the chain height. With v2.0.1 this has been improved by also considering the `APPROX_RELEASE_HEIGHT` of the release, and a new parameter to allow the caller to manually override the consensus branch ID that zcashd will use. + +[Pull request](https://github.com/zcash/zcash/pull/3520) \ No newline at end of file