Merge pull request #143 from instagibbs/explicit0

fixup disallowing 0-value spendable outputs

Author: instagibbs

qa/rpc-tests/getchaintips.py

@@ -27,6 +27,8 @@ def run_test (self):
         # Split the network and build two chains of different lengths.
         self.split_network ()
         self.nodes[0].generate(10)
+        # Need a send to split the chains since empty blocks are identical
+        self.nodes[2].sendtoaddress(self.nodes[2].getnewaddress(), 1)
         self.nodes[2].generate(20)
         self.sync_all ()
 

qa/rpc-tests/wallet.py

@@ -101,11 +101,11 @@ def run_test (self):
         assert_equal(self.nodes[0].getbalance("", 0, False, "bitcoin"), 21000000-21)
         assert_equal(self.nodes[2].getbalance("", 0, False, "bitcoin"), 21)
 
-        # Node0 should have three non-zero unspent outputs and 101 from generate.
+        # Node0 should have three spendable outputs since 0-value coinbase outputs will be OP_RETURN.
         # Create a couple of transactions to send them to node2, submit them through
         # node1, and make sure both node0 and node2 pick them up properly:
         node0utxos = self.nodes[0].listunspent(1, 9999999, [], "bitcoin")
-        assert_equal(len(node0utxos), 104)
+        assert_equal(len(node0utxos), 3)
 
         # create both transactions
         txns_to_send = []

src/Makefile.test.include

@@ -94,7 +94,8 @@ BITCOIN_TESTS =\
   test/versionbits_tests.cpp \
   test/uint256_tests.cpp \
   test/univalue_tests.cpp \
-  test/util_tests.cpp
+  test/util_tests.cpp \
+  test/verify_amounts_tests.cpp
 
 if ENABLE_WALLET
 BITCOIN_TESTS += \

src/blind.cpp

@@ -164,11 +164,11 @@ void BlindAsset(CConfidentialAsset& confAsset, secp256k1_generator& gen, const C
     assert(ret != 0);
 }
 
-void CreateValueCommitment(CConfidentialValue& confValue, secp256k1_pedersen_commitment& commit, const std::vector<unsigned char*> blindptrs, const secp256k1_generator& gen, const CAmount amount)
+void CreateValueCommitment(CConfidentialValue& confValue, secp256k1_pedersen_commitment& commit, const unsigned char* blindptr, const secp256k1_generator& gen, const CAmount amount)
 {
     int ret;
     confValue.vchCommitment.resize(CConfidentialValue::nCommittedSize);
-    ret = secp256k1_pedersen_commit(secp256k1_blind_context, &commit, (unsigned char*)blindptrs.back(), amount, &gen);
+    ret = secp256k1_pedersen_commit(secp256k1_blind_context, &commit, blindptr, amount, &gen);
     assert(ret != 0);
     secp256k1_pedersen_commitment_serialize(secp256k1_blind_context, &confValue.vchCommitment[0], &commit);
     assert(confValue.IsValid());
@@ -416,7 +416,7 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
                 BlindAsset(confAsset, gen, asset, assetblindptrs.back());
 
                 // Create value commitment
-                CreateValueCommitment(confValue, commit, blindptrs, gen, amount);
+                CreateValueCommitment(confValue, commit, blindptrs.back(), gen, amount);
 
                 // nonce should just be blinding key
                 uint256 nonce = nPseudo ? uint256(std::vector<unsigned char>(vBlindIssuanceToken[nIn].begin(), vBlindIssuanceToken[nIn].end())) : uint256(std::vector<unsigned char>(vBlindIssuanceAsset[nIn].begin(), vBlindIssuanceAsset[nIn].end()));
@@ -485,7 +485,7 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
             BlindAsset(confAsset, gen, asset, assetblindptrs.back());
 
             // Create value commitment
-            CreateValueCommitment(confValue, commit, blindptrs, gen, amount);
+            CreateValueCommitment(confValue, commit, blindptrs.back(), gen, amount);
 
             // Generate nonce for rewind by owner
             uint256 nonce = GenerateOutputRangeproofNonce(out, output_pubkeys[nOut]);

src/blind.h

@@ -17,7 +17,7 @@ uint256 GenerateOutputRangeproofNonce(CTxOut& out, const CPubKey output_pubkey);
 
 void BlindAsset(CConfidentialAsset& confAsset, secp256k1_generator& gen, const CAsset& asset, const unsigned char* assetblindptr);
 
-void CreateValueCommitment(CConfidentialValue& value, secp256k1_pedersen_commitment& commit, const std::vector<unsigned char*> blindptrs, const secp256k1_generator& gen, const CAmount amount);
+void CreateValueCommitment(CConfidentialValue& value, secp256k1_pedersen_commitment& commit, const unsigned char* blindptr, const secp256k1_generator& gen, const CAmount amount);
 
 /*
  * blinding_key is used to create the nonce to rewind the rangeproof in conjunction with the nNonce commitment. In the case of a 0-length nNonce, the blinding key is directly used as the nonce.

src/main.cpp

@@ -1299,8 +1299,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
                 if (!MoneyRange(val.GetAmount()))
                     return false;
 
-                if (val.GetAmount() == 0)
-                    continue;
+                assert(val.GetAmount() != 0);
 
                 if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, val.GetAmount(), &gen) != 1)
                     return false;
@@ -1474,8 +1473,14 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
             if (!MoneyRange(val.GetAmount()))
                 return false;
 
-            if (val.GetAmount() == 0)
-              continue;
+            if (val.GetAmount() == 0) {
+                if (tx.vout[i].scriptPubKey.IsUnspendable()) {
+                    continue;
+                } else {
+                    // No spendable 0-value outputs
+                    return false;
+                }
+            }
 
             if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, val.GetAmount(), &gen) != 1)
                 return false;
@@ -1559,9 +1564,14 @@ bool VerifyCoinbaseAmount(const CTransaction& tx, const CAmountMap& mapFees)
     assert(tx.IsCoinBase());
     CAmountMap remaining = mapFees;
     for (unsigned int i = 0; i < tx.vout.size(); i++) {
-        if (!tx.vout[i].nValue.IsExplicit() || !tx.vout[i].nAsset.IsExplicit())
+        const CTxOut& out = tx.vout[i];
+        if (!out.nValue.IsExplicit() || !out.nAsset.IsExplicit()) {
+            return false;
+        }
+        if (!MoneyRange(out.nValue.GetAmount()) || (out.nValue.GetAmount() == 0 && !out.scriptPubKey.IsUnspendable())) {
             return false;
-        remaining[tx.vout[i].nAsset.GetAsset()] -= tx.vout[i].nValue.GetAmount();
+        }
+        remaining[out.nAsset.GetAsset()] -= out.nValue.GetAmount();
     }
     return MoneyRange(remaining);
 }

src/miner.cpp

@@ -175,7 +175,7 @@ CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
     coinbaseTx.vin.resize(1);
     coinbaseTx.vin[0].prevout.SetNull();
     coinbaseTx.vout.resize(1);
-    coinbaseTx.vout[0].scriptPubKey = scriptPubKeyIn;
+    coinbaseTx.vout[0].scriptPubKey = nFees ? scriptPubKeyIn : CScript() << OP_RETURN;
     coinbaseTx.vout[0].nValue = nFees;
     coinbaseTx.vout[0].nAsset = policyAsset;
     coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;

src/test/blind_tests.cpp

@@ -337,6 +337,10 @@ BOOST_AUTO_TEST_CASE(naive_blinding_test)
         BOOST_CHECK(BlindTransaction(input_blinds, input_asset_blinds, input_assets, input_amounts, output_blinds, output_asset_blinds, output_pubkeys, vDummy, vDummy, txtemp) == 4);
         BOOST_CHECK(VerifyAmounts(cache, txtemp));
 
+        // Transaction may not have spendable 0-value output
+        txtemp.vout.push_back(CTxOut(CAsset(), 0, CScript() << OP_TRUE));
+        BOOST_CHECK(!VerifyAmounts(cache, txtemp));
+
         // Create imbalance by removing fees, should still be able to blind
         txtemp = tx5;
         txtemp.vout.resize(5);

src/test/verify_amounts_tests.cpp

@@ -0,0 +1,93 @@
+// Copyright (c) 2017-2017 Blockstream Inc.
+// Distributed under the MIT/X11 software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "arith_uint256.h"
+#include "blind.h"
+#include "uint256.h"
+#include "main.h"
+
+#include "test/test_bitcoin.h"
+
+#include <boost/test/unit_test.hpp>
+
+
+BOOST_FIXTURE_TEST_SUITE(amount_tests, TestingSetup)
+
+BOOST_AUTO_TEST_CASE(verify_coinbase_test)
+{
+    // Generate some asset types and feemap
+    std::vector<CAsset> types;
+    CAmountMap mapFees;
+    for (unsigned int i = 1; i < 4; i++) {
+        types.emplace_back(GetRandHash());
+        mapFees[types.back()] = i;
+    }
+
+    // Make coinbase transaction
+    CMutableTransaction tx;
+    tx.vin.push_back(CTxIn(COutPoint(), CScript(), 0));
+
+    // Blank amount check
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, CAmountMap()));
+
+    // Check against non-null fee, surplus is ok
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // Add non-fee outputs to coinbase
+    for (unsigned int i = 1; i < 4; i++) {
+        BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+        tx.vout.push_back(CTxOut(types[i-1], i, CScript() << OP_TRUE));
+    }
+    // All outputs added, should still balance
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // Adding 0-value unspendable output should not change balance
+    tx.vout.push_back(CTxOut(types.back(), 0, CScript() << OP_RETURN));
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // But you cannot add spendable 0-value output
+    tx.vout.push_back(CTxOut(types.back(), 0, CScript() << OP_TRUE));
+    BOOST_CHECK(!VerifyCoinbaseAmount(tx, mapFees));
+    tx.vout.pop_back();
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // Adding values outside MAX_MONEY also causes failure
+    mapFees[CAsset()] = MAX_MONEY+1;
+    tx.vout.push_back(CTxOut(CAsset(), MAX_MONEY+1, CScript() << OP_RETURN));
+    BOOST_CHECK(!VerifyCoinbaseAmount(tx, mapFees));
+    tx.vout.pop_back();
+    mapFees[CAsset()] = 0;
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // Change the unspendable 0-value output to have asset commitment
+    secp256k1_generator gen;
+    const unsigned char blind[32] = {0};
+    CAsset asset = tx.vout.back().nAsset.GetAsset();
+
+    BlindAsset(tx.vout.back().nAsset, gen, asset, blind);
+
+    BOOST_CHECK(!VerifyCoinbaseAmount(tx, mapFees));
+    tx.vout.back().nAsset = types.back();
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // Change the same output's value to an unblinded commitment to non-zero value (unblinded zero is blank commitment)
+    secp256k1_pedersen_commitment commit;
+    CreateValueCommitment(tx.vout.back().nValue, commit, blind, gen, 1);
+    BOOST_CHECK(!VerifyCoinbaseAmount(tx, mapFees));
+
+    tx.vout.pop_back();
+    BOOST_CHECK(VerifyCoinbaseAmount(tx, mapFees));
+
+    // Negative output
+    tx.vout.push_back(CTxOut(CAsset(), -1, CScript() << OP_RETURN));
+    BOOST_CHECK(!VerifyCoinbaseAmount(tx, mapFees));
+    tx.vout.pop_back();
+
+    // Transaction claiming too much fees
+    tx.vout.push_back(CTxOut(CAsset(), 1, CScript() << OP_RETURN));
+    BOOST_CHECK(!VerifyCoinbaseAmount(tx, mapFees));
+    tx.vout.pop_back();
+}
+
+BOOST_AUTO_TEST_SUITE_END()