From 2fd52ada5121d5eb70d4a2e5943b7436fb483e58 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 17 Jan 2023 19:17:56 -0700 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Daira Hopwood --- qa/rpc-tests/wallet_persistence.py | 45 +++++++++++++++--------------- src/coins.cpp | 1 - src/main.cpp | 2 +- src/primitives/transaction.cpp | 4 +++ src/rpc/blockchain.cpp | 24 ++++++++-------- 5 files changed, 40 insertions(+), 36 deletions(-) diff --git a/qa/rpc-tests/wallet_persistence.py b/qa/rpc-tests/wallet_persistence.py index 23cfa8e2a..d7ab8cd9a 100755 --- a/qa/rpc-tests/wallet_persistence.py +++ b/qa/rpc-tests/wallet_persistence.py @@ -60,19 +60,20 @@ class WalletPersistenceTest (BitcoinTestFramework): addresses = self.nodes[0].z_listaddresses() assert_true(sapling_addr in addresses, "Should contain address before restart") - def check_chain_value(pool, expected_value): + def check_chain_value(pool, expected_id, expected_value): + assert_equal(pool.get('id', None), expected_id) + assert_equal(pool['monitored'], True) assert_equal(pool['chainValue'], expected_value) assert_equal(pool['chainValueZat'], expected_value * COIN) # Verify size of pools chainInfo = self.nodes[0].getblockchaininfo() - print(str(chainInfo)) - assert_equal(chainInfo['chainSupply']['chainValue'], expected_supply) # Supply pools = chainInfo['valuePools'] - check_chain_value(pools[0], expected_supply) # Transparent - check_chain_value(pools[1], Decimal('0')) # Sprout - check_chain_value(pools[2], Decimal('0')) # Sapling - check_chain_value(pools[3], Decimal('0')) # Orchard + check_chain_value(chainInfo['chainSupply'], None, expected_supply) + check_chain_value(pools[0], 'transparent', expected_supply) + check_chain_value(pools[1], 'sprout', Decimal('0')) + check_chain_value(pools[2], 'sapling', Decimal('0')) + check_chain_value(pools[3], 'orchard', Decimal('0')) # Restart the nodes stop_nodes(self.nodes) @@ -87,11 +88,11 @@ class WalletPersistenceTest (BitcoinTestFramework): chainInfo = self.nodes[0].getblockchaininfo() pools = chainInfo['valuePools'] # Reenable these test in v5.4.0-rc1 - # check_chain_value(chainInfo['chainSupply'], expected_supply) # Supply - # check_chain_value(pools[0], expected_supply) # Transparent - check_chain_value(pools[1], Decimal('0')) # Sprout - check_chain_value(pools[2], Decimal('0')) # Sapling - check_chain_value(pools[3], Decimal('0')) # Orchard + # check_chain_value(chainInfo['chainSupply'], None, expected_supply) # Supply + # check_chain_value(pools[0], 'transparent', expected_supply) + check_chain_value(pools[1], 'sprout', Decimal('0')) + check_chain_value(pools[2], 'sapling', Decimal('0')) + check_chain_value(pools[3], 'orchard', Decimal('0')) # Node 0 shields funds to Sapling address taddr0 = get_coinbase_address(self.nodes[0]) @@ -112,11 +113,11 @@ class WalletPersistenceTest (BitcoinTestFramework): chainInfo = self.nodes[0].getblockchaininfo() pools = chainInfo['valuePools'] # Reenable these tests in v5.4.0-rc1 - # check_chain_value(chainInfo['chainSupply'], expected_supply) # Supply - # check_chain_value(pools[0], expected_supply - Decimal('20')) # Transparent - check_chain_value(pools[1], Decimal('0')) # Sprout - check_chain_value(pools[2], Decimal('20')) # Sapling - check_chain_value(pools[3], Decimal('0')) # Orchard + # check_chain_value(chainInfo['chainSupply'], None, expected_supply) # Supply + # check_chain_value(pools[0], 'transparent', expected_supply - Decimal('20')) # Transparent + check_chain_value(pools[1], 'sprout', Decimal('0')) + check_chain_value(pools[2], 'sapling', Decimal('20')) + check_chain_value(pools[3], 'orchard', Decimal('0')) # Restart the nodes stop_nodes(self.nodes) @@ -127,11 +128,11 @@ class WalletPersistenceTest (BitcoinTestFramework): chainInfo = self.nodes[0].getblockchaininfo() pools = chainInfo['valuePools'] # Reenable these tests in v5.4.0-rc1 - # check_chain_value(chainInfo['chainSupply'], expected_supply) # Supply - # check_chain_value(pools[0], expected_supply - Decimal('20')) # Transparent - check_chain_value(pools[1], Decimal('0')) # Sprout - check_chain_value(pools[2], Decimal('20')) # Sapling - check_chain_value(pools[3], Decimal('0')) # Orchard + # check_chain_value(chainInfo['chainSupply'], None, expected_supply) # Supply + # check_chain_value(pools[0], 'transparent', expected_supply - Decimal('20')) # Transparent + check_chain_value(pools[1], 'sprout', Decimal('0')) + check_chain_value(pools[2], 'sapling', Decimal('20')) + check_chain_value(pools[3], 'orchard', Decimal('0')) # Node 0 sends some shielded funds to Node 1 dest_addr = self.nodes[1].z_getnewaddress('sapling') diff --git a/src/coins.cpp b/src/coins.cpp index a52dcf943..4cca98a0a 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -994,7 +994,6 @@ CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const return GetTransparentValueIn(tx) + tx.GetShieldedValueIn(); } -// TODO: remove this if it ends up unused CAmount CCoinsViewCache::GetTransparentValueIn(const CTransaction& tx) const { if (tx.IsCoinBase()) diff --git a/src/main.cpp b/src/main.cpp index 96d0da200..48c4c3a8c 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4687,7 +4687,7 @@ bool ReceivedBlockTransactions( // place that we have a valid coins view with which to compute // the transparent input value and fees. - // calculate the block's effect on the chain's net Sprout value + // Calculate the block's effect on the Sprout chain value pool balance. if (pindex->pprev->nChainSproutValue && pindex->nSproutValue) { pindex->nChainSproutValue = *pindex->pprev->nChainSproutValue + *pindex->nSproutValue; } else { diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 42473362e..b2c3768c0 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -352,6 +352,10 @@ CAmount CTransaction::GetShieldedValueIn() const } } + if (IsCoinBase() && nValue != 0) { + throw std::runtime_error("CTransaction::GetShieldedValueIn(): shielded value of inputs must be zero in coinbase transactions."); + } + return nValue; } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 416d3a449..d1d9a1ba1 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -747,19 +747,19 @@ UniValue getblock(const UniValue& params, bool fHelp) " \"difficulty\" : x.xxx, (numeric) The difficulty\n" " \"chainSupply\": { (object) information about the total supply\n" " \"monitored\": xx, (boolean) true if the total supply is being monitored\n" - " \"chainValue\": xxxxxx, (numeric, optional) total chain supply\n" - " \"chainValueZat\": xxxxxx, (numeric, optional) total chain supply in satoshis\n" - " \"valueDelta\": xxxxxx, (numeric, optional) change to the chain supply produced by this block\n" - " \"valueDeltaZat\": xxxxxx, (numeric, optional) change to the chain supply produced by this block, in satoshis\n" + " \"chainValue\": xxxxxx, (numeric, optional) total chain supply after this block, in " + CURRENCY_UNIT + "\n" + " \"chainValueZat\": xxxxxx, (numeric, optional) total chain supply after this block, in " + MINOR_CURRENCY_UNIT + "\n" + " \"valueDelta\": xxxxxx, (numeric, optional) change to the chain supply produced by this block, in " + CURRENCY_UNIT + "\n" + " \"valueDeltaZat\": xxxxxx, (numeric, optional) change to the chain supply produced by this block, in " + MINOR_CURRENCY_UNIT + "\n" " }\n" " \"valuePools\": [ (array) information about each value pool\n" " {\n" " \"id\": \"xxxx\", (string) name of the pool\n" " \"monitored\": xx, (boolean) true if the pool is being monitored\n" - " \"chainValue\": xxxxxx, (numeric, optional) total amount in the pool\n" - " \"chainValueZat\": xxxxxx, (numeric, optional) total amount in the pool in satoshis\n" - " \"valueDelta\": xxxxxx, (numeric, optional) change to the amount in the pool produced by this block\n" - " \"valueDeltaZat\": xxxxxx, (numeric, optional) change to the amount in the pool produced by this block, in satoshis\n" + " \"chainValue\": xxxxxx, (numeric, optional) total amount in the pool, in " + CURRENCY_UNIT + "\n" + " \"chainValueZat\": xxxxxx, (numeric, optional) total amount in the pool, in " + MINOR_CURRENCY_UNIT + "\n" + " \"valueDelta\": xxxxxx, (numeric, optional) change to the amount in the pool produced by this block, in " + CURRENCY_UNIT + "\n" + " \"valueDeltaZat\": xxxxxx, (numeric, optional) change to the amount in the pool produced by this block, in " + MINOR_CURRENCY_UNIT + "\n" " }, ...\n" " ]\n" " \"previousblockhash\" : \"hash\", (string) The hash of the previous block\n" @@ -1058,15 +1058,15 @@ UniValue getblockchaininfo(const UniValue& params, bool fHelp) " \"commitments\": xxxxxx, (numeric) the current number of note commitments in the commitment tree\n" " \"chainSupply\": { (object) information about the total supply\n" " \"monitored\": xx, (boolean) true if the total supply is being monitored\n" - " \"chainValue\": xxxxxx, (numeric, optional) total chain supply\n" - " \"chainValueZat\": xxxxxx, (numeric, optional) total chain supply in satoshis\n" + " \"chainValue\": xxxxxx, (numeric, optional) total chain supply after this block, in " + CURRENCY_UNIT + "\n" + " \"chainValueZat\": xxxxxx, (numeric, optional) total chain supply after this block, in " + MINOR_CURRENCY_UNIT + "\n" " }\n" " \"valuePools\": [ (array) information about each value pool\n" " {\n" " \"id\": \"xxxx\", (string) name of the pool\n" " \"monitored\": xx, (boolean) true if the pool is being monitored\n" - " \"chainValue\": xxxxxx, (numeric, optional) total amount in the pool\n" - " \"chainValueZat\": xxxxxx, (numeric, optional) total amount in the pool in satoshis\n" + " \"chainValue\": xxxxxx, (numeric, optional) total amount in the pool, in " + CURRENCY_UNIT + "\n" + " \"chainValueZat\": xxxxxx, (numeric, optional) total amount in the pool, in " + MINOR_CURRENCY_UNIT + "\n" " }, ...\n" " ]\n" " \"softforks\": [ (array) status of softforks in progress\n"