Fix logic issues with randomization of outputs in [re]issueasset_base; minor refactoring.

src/rpc/rawtransaction.cpp

@@ -2583,9 +2583,13 @@ struct RawIssuanceDetails
 };
 
 // Appends a single issuance to the first input that doesn't have one, and includes
-// a single output per asset type in shuffled positions.
+// a single output per asset type in shuffled positions. Requires at least one output
+// to exist (the fee output, which must be last).
 void issueasset_base(CMutableTransaction& mtx, RawIssuanceDetails& issuance_details, const CAmount asset_amount, const CAmount token_amount, const CTxDestination& asset_dest, const CTxDestination& token_dest, const bool blind_issuance, const uint256& contract_hash)
 {
+    assert(asset_amount > 0 || token_amount > 0);
+    assert(mtx.vout.size() > 0);
+
     CScript asset_script = GetScriptForDestination(asset_dest);
     CScript token_script = GetScriptForDestination(token_dest);
 
@@ -2616,13 +2620,10 @@ void issueasset_base(CMutableTransaction& mtx, RawIssuanceDetails& issuance_deta
 
     mtx.vin[issuance_input_index].assetIssuance.assetEntropy = contract_hash;
 
-    // Place assets into randomly placed output slots, just insert in place
-    // -1 due to fee output being at the end no matter what.
-    int asset_place = GetRandInt(mtx.vout.size()-1);
-    int token_place = GetRandInt(mtx.vout.size()); // Don't bias insertion
-
-    assert(asset_amount > 0 || token_amount > 0);
     if (asset_amount > 0) {
+        // Fee output is required to be last. We will insert _before_ the selected position, which preserves that.
+        int asset_place = GetRandInt(mtx.vout.size());
+
         CTxOut asset_out(asset, asset_amount, asset_script);
         // If blinded address, insert the pubkey into the nonce field for later substitution by blinding
         if (IsBlindDestination(asset_dest)) {
@@ -2638,6 +2639,9 @@ void issueasset_base(CMutableTransaction& mtx, RawIssuanceDetails& issuance_deta
     }
 
     if (token_amount > 0) {
+        // Calculate this _after_ we conditionally insert the asset output, which changes mtx.vout.size().
+        int token_place = GetRandInt(mtx.vout.size());
+
         CTxOut token_out(token, token_amount, token_script);
         // If blinded address, insert the pubkey into the nonce field for later substitution by blinding
         if (IsBlindDestination(token_dest)) {
@@ -2650,17 +2654,16 @@ void issueasset_base(CMutableTransaction& mtx, RawIssuanceDetails& issuance_deta
     }
 }
 
-// Appends a single reissuance to the specified input if none exists,
+// Appends a single reissuance to the specified input if none exists, and the
-// and the corresponding output in a shuffled position. Errors otherwise.
+// corresponding output in a shuffled position. Errors otherwise. Requires at
-void reissueasset_base(CMutableTransaction& mtx, int& issuance_input_index, const CAmount asset_amount, const CTxDestination& asset_dest, const uint256& asset_blinder, const uint256& entropy)
+// least one output to exist (the fee output, which must be last).
+void reissueasset_base(CMutableTransaction& mtx, size_t issuance_input_index, const CAmount asset_amount, const CTxDestination& asset_dest, const uint256& asset_blinder, const uint256& entropy)
 {
-    CScript asset_script = GetScriptForDestination(asset_dest);
+    assert(mtx.vout.size() > 0);
+    assert(asset_amount > 0);
+    assert(mtx.vin[issuance_input_index].assetIssuance.IsNull());
 
-    // Check if issuance already exists, error if already exists
+    CScript asset_script = GetScriptForDestination(asset_dest);
-    if ((size_t)issuance_input_index >= mtx.vin.size() || !mtx.vin[issuance_input_index].assetIssuance.IsNull()) {
-        issuance_input_index = -1;
-        return;
-    }
 
     CAsset asset;
     CalculateAsset(asset, entropy);
@@ -2670,8 +2673,7 @@ void reissueasset_base(CMutableTransaction& mtx, int& issuance_input_index, cons
     mtx.vin[issuance_input_index].assetIssuance.nAmount = asset_amount;
 
     // Place assets into randomly placed output slots, before change output, inserted in place
-    assert(mtx.vout.size() >= 1);
+    int asset_place = GetRandInt(mtx.vout.size());
-    int asset_place = GetRandInt(mtx.vout.size()-1);
 
     CTxOut asset_out(asset, asset_amount, asset_script);
     // If blinded address, insert the pubkey into the nonce field for later substitution by blinding
@@ -2679,7 +2681,6 @@ void reissueasset_base(CMutableTransaction& mtx, int& issuance_input_index, cons
         CPubKey asset_blind = GetDestinationBlindingKey(asset_dest);
         asset_out.nNonce.vchCommitment = std::vector<unsigned char>(asset_blind.begin(), asset_blind.end());
     }
-    assert(asset_amount > 0);
     mtx.vout.insert(mtx.vout.begin()+asset_place, asset_out);
     mtx.vin[issuance_input_index].assetIssuance.nAmount = asset_amount;
 }
@@ -2734,6 +2735,19 @@ UniValue rawissueasset(const JSONRPCRequest& request)
     // Count issuances, only append hex to final one
     unsigned int issuances_til_now = 0;
 
+    // Validate fee output location, required by the implementation of issueasset_base
+    if (mtx.vout.size() == 0){
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction must have at least one output.");
+    }
+    if (!mtx.vout[mtx.vout.size() - 1].IsFee()) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction must have exactly one fee output, which must be last");
+    }
+    for (size_t i = 0; i < mtx.vout.size() - 1; i++) {
+        if (mtx.vout[i].IsFee()) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction must have exactly one fee output, which must be last");
+        }
+    }
+
     for (unsigned int idx = 0; idx < issuances.size(); idx++) {
         const UniValue& issuance = issuances[idx];
         const UniValue& issuance_o = issuance.get_obj();
@@ -2852,9 +2866,17 @@ UniValue rawreissueasset(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction must have at least one output.");
     }
 
-    UniValue issuances = request.params[1].get_array();
+    // Validate fee output location, required by the implementation of reissueasset_base
+    if (!mtx.vout[mtx.vout.size() - 1].IsFee()) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction must have exactly one fee output, which must be last");
+    }
+    for (size_t i = 0; i < mtx.vout.size() - 1; i++) {
+        if (mtx.vout[i].IsFee()) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction must have exactly one fee output, which must be last");
+        }
+    }
 
-    unsigned int num_issuances = 0;
+    UniValue issuances = request.params[1].get_array();
 
     for (unsigned int idx = 0; idx < issuances.size(); idx++) {
         const UniValue& issuance = issuances[idx];
@@ -2880,31 +2902,24 @@ UniValue rawreissueasset(const JSONRPCRequest& request)
             throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid asset address provided: %s", asset_address_uni.get_str()));
         }
 
-        int input_index = -1;
+        size_t input_index = -1;
         const UniValue& input_index_o = issuance_o["input_index"];
         if (input_index_o.isNum()) {
             input_index = input_index_o.get_int();
             if (input_index < 0) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Input index must be non-negative.");
+            } else if (input_index >= mtx.vin.size()) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Input index must exist in transaction.");
+            } else if (!mtx.vin[input_index].assetIssuance.IsNull()) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected transaction input already has issuance data.");
             }
         } else {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Input indexes for all reissuances are required.");
         }
 
         uint256 asset_blinder = ParseHashV(issuance_o["asset_blinder"], "asset_blinder");
-
         uint256 entropy = ParseHashV(issuance_o["entropy"], "entropy");
-
         reissueasset_base(mtx, input_index, asset_amount, asset_dest, asset_blinder, entropy);
-        if (input_index == -1) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected transaction input already has issuance data.");
-        }
-
-        num_issuances++;
-    }
-
-    if (num_issuances != issuances.size()) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Failed to find enough blank inputs for listed issuances.");
     }
 
     UniValue ret(UniValue::VOBJ);

test/functional/feature_issuance.py

@@ -290,10 +290,20 @@ class IssuanceTest(BitcoinTestFramework):
         # Default "blind" value is true, omitting explicit argument for last
         process_raw_issuance(self.nodes[0], [{"asset_amount": 1, "asset_address": nonblind_addr, "token_amount":2, "token_address":blind_addr, "blind":True}, {"asset_amount":3, "asset_address":nonblind_addr, "blind":True}, {"asset_amount":4, "asset_address":nonblind_addr, "token_amount":5, "token_address":blind_addr, "blind":True}, {"asset_amount":6, "asset_address":nonblind_addr, "token_amount":7, "token_address":blind_addr, "blind":True}, {"asset_amount":8, "asset_address":nonblind_addr, "token_amount":9, "token_address":blind_addr}])
 
+
+        # Make sure that fee is checked
+        valid_addr = self.nodes[0].getnewaddress()
+        raw_tx = self.nodes[0].createrawtransaction([], {})
+        assert_raises_rpc_error(-8, "Transaction must have at least one output.",
+                self.nodes[0].rawissueasset, raw_tx, [{"asset_amount": 1, "asset_address": valid_addr}])
+        raw_tx = self.nodes[0].createrawtransaction([], {valid_addr: Decimal("1")})
+        assert_raises_rpc_error(-8, "Transaction must have exactly one fee output, which must be last",
+                self.nodes[0].rawissueasset, raw_tx, [{"asset_amount": 1, "asset_address": valid_addr}])
+
         # Make sure that invalid addresses are rejected.
         valid_addr = self.nodes[0].getnewaddress()
         raw_tx = self.nodes[0].createrawtransaction([], {valid_addr: Decimal("1")})
-        funded_tx = raw_tx #self.nodes[0].fundrawtransaction(raw_tx, {"feeRate": Decimal('0.00050000')})['hex']
+        funded_tx = self.nodes[0].fundrawtransaction(raw_tx, {"feeRate": Decimal('0.00050000')})['hex']
         assert_raises_rpc_error(-8, "Invalid asset address provided: foobar",
                 self.nodes[0].rawissueasset, funded_tx, [{"asset_amount": 1, "asset_address": "foobar"}])
         assert_raises_rpc_error(-8, "Invalid token address provided: foobar",