From 46f24162349735db7f7b5b81fd0d70a7f59df99c Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Fri, 18 May 2018 17:52:05 +0200 Subject: [PATCH 1/7] Remove old testing code --- src/primitives/transaction.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index bd568342da..c1e92f2ecb 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -43,8 +43,6 @@ public: return; /* Explicit value */ case 1: - /* Trust-me! asset generation */ - case 0xff: vchCommitment.resize(nExplicitSize); break; /* Confidential commitment */ @@ -79,8 +77,7 @@ public: bool IsValid() const { - return IsNull() || IsExplicit() || IsCommitment() - || (vchCommitment.size()==nExplicitSize && vchCommitment[0]==0xff); + return IsNull() || IsExplicit() || IsCommitment(); } friend bool operator==(const CConfidentialCommitment& a, const CConfidentialCommitment& b) From bef2760352bc4846e7eb930d9f01e1c38a502769 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Fri, 18 May 2018 19:57:25 +0200 Subject: [PATCH 2/7] Assert return value of secp256k1_pedersen_commit() for issuances secp256k1_pedersen_commit() is expected to succeed because the value and the blinding factor are both 0. --- src/validation.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 4958b0db41..0c50c82f0c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -825,9 +825,9 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve continue; } - if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nAmount.GetAmount(), &gen) != 1) { - return false; - } + ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nAmount.GetAmount(), &gen); + // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. + assert(ret == 1); } else if (issuance.nAmount.IsCommitment()) { if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &issuance.nAmount.vchCommitment[0]) != 1) { @@ -867,9 +867,10 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve continue; } - if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nInflationKeys.GetAmount(), &gen) != 1) { - return false; - } + // Here we have issuance.nAmount.IsCommitment() == true + ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nInflationKeys.GetAmount(), &gen); + // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. + assert(ret == 1); } else if (issuance.nInflationKeys.IsCommitment()) { if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &issuance.nInflationKeys.vchCommitment[0]) != 1) { @@ -928,8 +929,9 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve } } - if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, val.GetAmount(), &gen) != 1) - return false; + ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, val.GetAmount(), &gen); + // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. + assert(ret == 1); } else if (val.IsCommitment()) { if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &val.vchCommitment[0]) != 1) From e1838685163abab0362fdf9b7a864ec705a5cc00 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Fri, 18 May 2018 20:12:57 +0200 Subject: [PATCH 3/7] Forbid explicit issuance of 0 asset units The proper way to issue 0 units of an asset is a null commitment but not an explicit commitment to 0. This commit eliminates the issue that some checks in the verification of the issuance of re-issuance tokes were skipped when issuing 0 asset units explicitly. This commit also simplifies the control flow of the relevant code locations. --- src/validation.cpp | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 0c50c82f0c..7c34c18cab 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -805,46 +805,44 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve } // Process issuance of asset - if (!issuance.nAmount.IsNull()) { + if (!issuance.nAmount.IsValid()) { + return false; + } + if (!issuance.nAmount.IsNull()) { // Generate asset generator and add to list of surjection targets ret = secp256k1_generator_generate(secp256k1_ctx_verify_amounts, &gen, assetID.begin()); assert(ret == 1); - CConfidentialAsset issuanceAsset; - issuanceAsset.vchCommitment.resize(CConfidentialAsset::nCommittedSize); - secp256k1_generator_serialize(secp256k1_ctx_verify_amounts, &issuanceAsset.vchCommitment[0], &gen); targetGenerators.push_back(gen); // Build value commitment and add to tally if (issuance.nAmount.IsExplicit()) { - if (!MoneyRange(issuance.nAmount.GetAmount())) { + if (!MoneyRange(issuance.nAmount.GetAmount()) || issuance.nAmount.GetAmount() == 0) { return false; } - if (issuance.nAmount.GetAmount() == 0) { - continue; - } - ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nAmount.GetAmount(), &gen); // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. assert(ret == 1); } - else if (issuance.nAmount.IsCommitment()) { + else { + assert(issuance.nAmount.IsCommitment()); + // Verify range proof + std::vector vchAssetCommitment(CConfidentialAsset::nExplicitSize); + secp256k1_generator_serialize(secp256k1_ctx_verify_amounts, &vchAssetCommitment[0], &gen); + if (QueueCheck(pvChecks, new CRangeCheck(&issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, vchAssetCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) { + return false; + } + + // Here we have issuance.nAmount.IsCommitment() == true if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &issuance.nAmount.vchCommitment[0]) != 1) { return false; } - } else { - return false; } vData.push_back(commit); vpCommitsIn.push_back(p); p++; - - // Rangecheck must be done for blinded amount - if (issuance.nAmount.IsCommitment() && QueueCheck(pvChecks, new CRangeCheck(&issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, issuanceAsset.vchCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) { - return false; - } } // Only initial issuance can have reissuance tokens From d7abf1870a4c1c77516e57e60d5a7ccd32012c15 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 19 May 2018 09:56:37 +0200 Subject: [PATCH 4/7] Extract helper function from VerifyAmount() --- src/validation.cpp | 76 ++++++++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 29 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7c34c18cab..7c31489dec 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -697,6 +697,50 @@ size_t GetNumIssuances(const CTransaction& tx) return numIssuances; } +// Helper function for VerifyAmount(), not exported +static bool VerifyIssuanceAmount(secp256k1_pedersen_commitment& commit, secp256k1_generator& gen, + const CAsset& asset, const CConfidentialValue& value, const std::vector& vchRangeproof, + std::vector* pvChecks, const bool cacheStore) +{ + // This is used to add in the explicit values + unsigned char explBlinds[32]; + memset(explBlinds, 0, sizeof(explBlinds)); + int ret; + + assert(value.IsValid()); + + // Generate asset generator + ret = secp256k1_generator_generate(secp256k1_ctx_verify_amounts, &gen, asset.begin()); + assert(ret == 1); + + // Build value commitment + if (value.IsExplicit()) { + if (!MoneyRange(value.GetAmount()) || value.GetAmount() == 0) { + return false; + } + + ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, value.GetAmount(), &gen); + // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. + assert(ret == 1); + } + else { + assert(value.IsCommitment()); + // Verify range proof + std::vector vchAssetCommitment(CConfidentialAsset::nExplicitSize); + secp256k1_generator_serialize(secp256k1_ctx_verify_amounts, &vchAssetCommitment[0], &gen); + if (QueueCheck(pvChecks, new CRangeCheck(&value, vchRangeproof, vchAssetCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) { + return false; + } + + // Here we have value.IsCommitment() == true + if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &value.vchCommitment[0]) != 1) { + return false; + } + } + + return true; +} + bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::vector* pvChecks, const bool cacheStore) { assert(!tx.IsCoinBase()); @@ -810,36 +854,10 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve return false; } if (!issuance.nAmount.IsNull()) { - // Generate asset generator and add to list of surjection targets - ret = secp256k1_generator_generate(secp256k1_ctx_verify_amounts, &gen, assetID.begin()); - assert(ret == 1); + if (!VerifyIssuanceAmount(commit, gen, assetID, issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, pvChecks, cacheStore)) { + return false; + } targetGenerators.push_back(gen); - - // Build value commitment and add to tally - if (issuance.nAmount.IsExplicit()) { - if (!MoneyRange(issuance.nAmount.GetAmount()) || issuance.nAmount.GetAmount() == 0) { - return false; - } - - ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nAmount.GetAmount(), &gen); - // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. - assert(ret == 1); - } - else { - assert(issuance.nAmount.IsCommitment()); - // Verify range proof - std::vector vchAssetCommitment(CConfidentialAsset::nExplicitSize); - secp256k1_generator_serialize(secp256k1_ctx_verify_amounts, &vchAssetCommitment[0], &gen); - if (QueueCheck(pvChecks, new CRangeCheck(&issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, vchAssetCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) { - return false; - } - - // Here we have issuance.nAmount.IsCommitment() == true - if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &issuance.nAmount.vchCommitment[0]) != 1) { - return false; - } - } - vData.push_back(commit); vpCommitsIn.push_back(p); p++; From 670229e0608bb3d78421e17a1e089d1e668c59ac Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 19 May 2018 10:06:54 +0200 Subject: [PATCH 5/7] Use new helper function to verify issuance of re-issuance tokens This forbids the explicit issuance of 0 re-issuance tokens (like for the issuance of normal assets. The proper way to issue no re-issuance tokens is to use a null commitment for the amount. --- src/validation.cpp | 50 ++++++++++++++-------------------------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7c31489dec..76577c9a26 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -863,51 +863,31 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve p++; } - // Only initial issuance can have reissuance tokens - if (issuance.assetBlindingNonce.IsNull() && !issuance.nInflationKeys.IsNull()) { + if (!issuance.nAmount.IsValid()) { + return false; + } - ret = secp256k1_generator_generate(secp256k1_ctx_verify_amounts, &gen, assetTokenID.begin()); - assert(ret == 1); - CConfidentialAsset tokenAsset(assetTokenID); - tokenAsset.vchCommitment.resize(CConfidentialAsset::nCommittedSize); - secp256k1_generator_serialize(secp256k1_ctx_verify_amounts, &tokenAsset.vchCommitment[0], &gen); + // Process issuance of reissuance tokens - targetGenerators.push_back(gen); - - if (issuance.nInflationKeys.IsExplicit()) { - if (!MoneyRange(issuance.nInflationKeys.GetAmount())) { - return false; - } - - if (issuance.nInflationKeys.GetAmount() == 0) { - continue; - } - - // Here we have issuance.nAmount.IsCommitment() == true - ret = secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, issuance.nInflationKeys.GetAmount(), &gen); - // The explBlinds are all 0, and the amount is not 0. So secp256k1_pedersen_commit does not fail. - assert(ret == 1); - } - else if (issuance.nInflationKeys.IsCommitment()) { - if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &issuance.nInflationKeys.vchCommitment[0]) != 1) { - return false; - } - } else { + if (!issuance.nInflationKeys.IsValid()) { + return false; + } + if (!issuance.nInflationKeys.IsNull()) { + // Only initial issuance can have reissuance tokens + if (!issuance.assetBlindingNonce.IsNull()) { return false; } + if (!VerifyIssuanceAmount(commit, gen, assetTokenID, issuance.nInflationKeys, tx.wit.vtxinwit[i].vchInflationKeysRangeproof, pvChecks, cacheStore)) { + return false; + } + targetGenerators.push_back(gen); vData.push_back(commit); vpCommitsIn.push_back(p); p++; - - if (issuance.nInflationKeys.IsCommitment() && QueueCheck(pvChecks, new CRangeCheck(&issuance.nInflationKeys, tx.wit.vtxinwit[i].vchInflationKeysRangeproof, tokenAsset.vchCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) { - return false; - } - } else if (!issuance.nInflationKeys.IsNull()) { - // Token amount field must be null for reissuance - return false; } } + for (size_t i = 0; i < tx.vout.size(); ++i) { const CConfidentialValue& val = tx.vout[i].nValue; From 42b62ac28543dd862758f8b97fc181e49c8dddad Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Tue, 22 May 2018 10:11:23 +0200 Subject: [PATCH 6/7] Ensure that input witness is present before accessing range proofs --- src/validation.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 76577c9a26..9abb28917a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -854,6 +854,9 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve return false; } if (!issuance.nAmount.IsNull()) { + if (i >= tx.wit.vtxinwit.size()) { + return false; + } if (!VerifyIssuanceAmount(commit, gen, assetID, issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, pvChecks, cacheStore)) { return false; } @@ -878,6 +881,9 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve return false; } + if (i >= tx.wit.vtxinwit.size()) { + return false; + } if (!VerifyIssuanceAmount(commit, gen, assetTokenID, issuance.nInflationKeys, tx.wit.vtxinwit[i].vchInflationKeysRangeproof, pvChecks, cacheStore)) { return false; } From 05602edf6fa981bb8f36cb7363b1a750d32322d7 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 23 May 2018 15:21:16 +0200 Subject: [PATCH 7/7] Improve comments for VerifyAmount() --- src/script/script.h | 2 ++ src/validation.cpp | 8 +++++--- src/validation.h | 9 ++++++--- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/script/script.h b/src/script/script.h index fa4dd176a5..21264b9584 100644 --- a/src/script/script.h +++ b/src/script/script.h @@ -657,6 +657,8 @@ public: * Returns whether the script is guaranteed to fail at execution, * regardless of the initial stack. This allows outputs to be pruned * instantly when entering the UTXO set. This includes fee outputs. + * + * This is consensus-critical because it is called by VerifyAmounts(). */ bool IsUnspendable() const { diff --git a/src/validation.cpp b/src/validation.cpp index 9abb28917a..95d54dfa55 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -793,6 +793,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve if (!MoneyRange(val.GetAmount())) return false; + // Fails if val.GetAmount() == 0 if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explBlinds, val.GetAmount(), &gen) != 1) return false; } else if (val.IsCommitment()) { @@ -831,8 +832,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve // Null nAmount is considered explicit 0, so just check for commitment CalculateReissuanceToken(assetTokenID, entropy, issuance.nAmount.IsCommitment()); } else { - //Re-issuance - + // Re-issuance // hashAssetIdentifier doubles as the entropy on reissuance CalculateAsset(assetID, issuance.assetEntropy); CalculateReissuanceToken(assetTokenID, issuance.assetEntropy, issuance.nAmount.IsCommitment()); @@ -927,6 +927,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve continue; } else { // No spendable 0-value outputs + // Reason: A spendable output of 0 reissuance tokens would allow reissuance without reissuance tokens. return false; } } @@ -977,11 +978,12 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve } } + // Surjection proofs for (size_t i = 0; i < tx.vout.size(); i++) { const CConfidentialAsset& asset = tx.vout[i].nAsset; const CTxOutWitness* ptxoutwit = tx.wit.vtxoutwit.size() <= i? NULL: &tx.wit.vtxoutwit[i]; - //No need for surjective proof + // No need for surjection proof if (asset.IsExplicit()) { if (ptxoutwit && !ptxoutwit->vchSurjectionproof.empty()) { return false; diff --git a/src/validation.h b/src/validation.h index 0a1542ccc3..61d3425b8d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -399,11 +399,14 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins /** * Verify the transaction's outputs spend exactly what its inputs provide, plus some excess amount. * + * This also checks rangeproofs, surjection proofs, and issuances of assets and re-issuance tokens. + * The function assumes that IsValidPeginWitness() returns true on all peg-in inputs. + * * @param[in] view CCoinsViewCache to find necessary outputs * @param[in] tx transaction for which we are checking totals - * @param[in] pvChecks multithreaded rangeproof and commitment checker - * @param[in] cacheStore signal if rangeproof verification should be cached - * @return True if totals are identical + * @param[in] pvChecks multithreaded rangeproof, surjection proof and commitment checker + * @param[in] cacheStore signal if rangeproof and surjection proof verification should be cached + * @return True if verification was not aborted and totals are identical */ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::vector* pvChecks = NULL, const bool cacheStore = false);