From b14e739227b8c11308ae57de708c30dc00f9db99 Mon Sep 17 00:00:00 2001 From: Steven Roose Date: Tue, 1 Aug 2023 23:25:39 +0100 Subject: [PATCH] Improve error reporting from VerifyAmounts The famous "bad-txns-in-ne-out" error that could mean anything! --- src/confidential_validation.cpp | 77 +++++++++++++++++---------------- src/confidential_validation.h | 2 +- src/consensus/tx_verify.cpp | 8 +++- src/rpc/rawtransaction.cpp | 1 + src/wallet/wallet.cpp | 4 +- 5 files changed, 50 insertions(+), 42 deletions(-) diff --git a/src/confidential_validation.cpp b/src/confidential_validation.cpp index efc7b753c4..ab3d3bcfa5 100644 --- a/src/confidential_validation.cpp +++ b/src/confidential_validation.cpp @@ -131,7 +131,7 @@ static bool VerifyIssuanceAmount(secp256k1_pedersen_commitment& value_commit, se return true; } -bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, std::vector* checks, const bool store_result) { +const std::string VerifyAmounts(const std::vector& inputs, const CTransaction& tx, std::vector* checks, const bool store_result) { assert(!tx.IsCoinBase()); assert(inputs.size() == tx.vin.size()); @@ -161,7 +161,7 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st const CConfidentialAsset& asset = inputs[i].nAsset; if (val.IsNull() || asset.IsNull()) - return false; + return "input val.IsNull or asset.IsNull"; if (asset.IsExplicit()) { ret = secp256k1_generator_generate(secp256k1_ctx_verify_amounts, &gen, asset.GetAsset().begin()); @@ -169,26 +169,26 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st } else if (asset.IsCommitment()) { if (secp256k1_generator_parse(secp256k1_ctx_verify_amounts, &gen, &asset.vchCommitment[0]) != 1) - return false; + return "input commitment parse failed"; } else { - return false; + return "input asset not explicit not commitment"; } target_generators.push_back(gen); if (val.IsExplicit()) { if (!MoneyRange(val.GetAmount())) - return false; + return "input value not in moneyrange"; // Fails if val.GetAmount() == 0 if (secp256k1_pedersen_commit(secp256k1_ctx_verify_amounts, &commit, explicit_blinds, val.GetAmount(), &gen) != 1) - return false; + return "input explicit value pedersen commit failed"; } else if (val.IsCommitment()) { if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &val.vchCommitment[0]) != 1) - return false; + return "input commitment pedersen commit failed"; } else { - return false; + return "input value not explicit not commitment"; } vData.push_back(commit); @@ -228,7 +228,7 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st // Must check that prevout is the blinded issuance token // prevout's asset tag = assetTokenID + assetBlindingNonce if (secp256k1_generator_generate_blinded(secp256k1_ctx_verify_amounts, &gen, assetTokenID.begin(), issuance.assetBlindingNonce.begin()) != 1) { - return false; + return "reissuance generate blinded generator failed"; } // Serialize the generator for direct comparison unsigned char derived_generator[33]; @@ -236,29 +236,30 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st // Belt-and-suspenders: Check that asset commitment from issuance input is correct size if (asset.vchCommitment.size() != sizeof(derived_generator)) { - return false; + return "reissuance asset commitment size is not derived generator size"; } - // We have already checked the outputs' generator commitment for general validity, so directly compare serialized bytes + // We have already checked the outputs' generator commitment for general validity, + // so directly compare serialized bytes if (memcmp(asset.vchCommitment.data(), derived_generator, sizeof(derived_generator))) { - return false; + return "reissuance asset commitment not equal to derived generator"; } } // Process issuance of asset if (!issuance.nAmount.IsValid()) { - return false; + return "issuance amount not valid"; } if (!issuance.nAmount.IsNull()) { // Note: This check disallows issuances in transactions with *no* witness data. // This can be relaxed in a future update as a HF by passing in an empty rangeproof // to `VerifyIssuanceAmount` instead. if (i >= tx.witness.vtxinwit.size()) { - return false; + return "no witness for issuance input"; } if (!VerifyIssuanceAmount(commit, gen, assetID, issuance.nAmount, tx.witness.vtxinwit[i].vchIssuanceAmountRangeproof, checks, store_result)) { - return false; + return "VerifyIssuanceAmount failed"; } target_generators.push_back(gen); vData.push_back(commit); @@ -269,22 +270,22 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st // Process issuance of reissuance tokens if (!issuance.nInflationKeys.IsValid()) { - return false; + return "issuance of reissuance tokens: inflatioinskeys invalid"; } if (!issuance.nInflationKeys.IsNull()) { // Only initial issuance can have reissuance tokens if (!issuance.assetBlindingNonce.IsNull()) { - return false; + return "only initial issuance can have reissuance tokens"; } // Note: This check disallows issuances in transactions with *no* witness data. // This can be relaxed in a future update as a HF by passing in an empty rangeproof // to `VerifyIssuanceAmount` instead. if (i >= tx.witness.vtxinwit.size()) { - return false; + return "no witness for reissuance input"; } if (!VerifyIssuanceAmount(commit, gen, assetTokenID, issuance.nInflationKeys, tx.witness.vtxinwit[i].vchInflationKeysRangeproof, checks, store_result)) { - return false; + return "reissuance VerifyIssuanceAmount failed"; } target_generators.push_back(gen); vData.push_back(commit); @@ -298,11 +299,11 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st const CConfidentialValue& val = tx.vout[i].nValue; const CConfidentialAsset& asset = tx.vout[i].nAsset; if (!asset.IsValid()) - return false; + return "output asset invalid"; if (!val.IsValid()) - return false; + return "output value invalid"; if (!tx.vout[i].nNonce.IsValid()) - return false; + return "output nonce invalid"; if (asset.IsExplicit()) { ret = secp256k1_generator_generate(secp256k1_ctx_verify_amounts, &gen, asset.GetAsset().begin()); @@ -310,15 +311,15 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st } else if (asset.IsCommitment()) { if (secp256k1_generator_parse(secp256k1_ctx_verify_amounts, &gen, &asset.vchCommitment[0]) != 1) - return false; + return "output asset commitment parse failed"; } else { - return false; + return "output asset is not explicit not commitment"; } if (val.IsExplicit()) { if (!MoneyRange(val.GetAmount())) - return false; + return "output value out of money range"; if (val.GetAmount() == 0) { if (tx.vout[i].scriptPubKey.IsUnspendable()) { @@ -326,7 +327,7 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st } else { // No spendable 0-value outputs // Reason: A spendable output of 0 reissuance tokens would allow reissuance without reissuance tokens. - return false; + return "attempted spendable 0-value output"; } } @@ -336,9 +337,9 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st } else if (val.IsCommitment()) { if (secp256k1_pedersen_commitment_parse(secp256k1_ctx_verify_amounts, &commit, &val.vchCommitment[0]) != 1) - return false; + return "output value pedersen commit parse failed"; } else { - return false; + return "output value is not explicit not commitment"; } vData.push_back(commit); @@ -348,7 +349,7 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st // Check balance if (QueueCheck(checks, new CBalanceCheck(vData, vpCommitsIn, vpCommitsOut)) != SCRIPT_ERR_OK) { - return false; + return "CBalanceCheck failed"; } // Range proofs @@ -360,7 +361,7 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st if (val.IsExplicit()) { if (ptxoutwit && !ptxoutwit->vchRangeproof.empty()) - return false; + return "explicit output value non-empty range proof"; continue; } if (asset.IsExplicit()) { @@ -369,10 +370,10 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st secp256k1_generator_serialize(secp256k1_ctx_verify_amounts, &vchAssetCommitment[0], &gen); } if (!ptxoutwit) { - return false; + return "blinded output value without output witness"; } if (QueueCheck(checks, new CRangeCheck(&val, ptxoutwit->vchRangeproof, vchAssetCommitment, tx.vout[i].scriptPubKey, store_result)) != SCRIPT_ERR_OK) { - return false; + return "CRangeCheck failed"; } } @@ -384,25 +385,25 @@ bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, st // No need for surjection proof if (asset.IsExplicit()) { if (ptxoutwit && !ptxoutwit->vchSurjectionproof.empty()) { - return false; + return "explicit output asset non-empty surjection proof"; } continue; } if (!ptxoutwit) - return false; + return "blinded output value without output witness"; if (secp256k1_generator_parse(secp256k1_ctx_verify_amounts, &gen, &asset.vchCommitment[0]) != 1) - return false; + return "output asset generator parse failed"; secp256k1_surjectionproof proof; if (secp256k1_surjectionproof_parse(secp256k1_ctx_verify_amounts, &proof, &ptxoutwit->vchSurjectionproof[0], ptxoutwit->vchSurjectionproof.size()) != 1) - return false; + return "output asset surjection proof failed"; if (QueueCheck(checks, new CSurjectionCheck(proof, target_generators, gen, wtxid, store_result)) != SCRIPT_ERR_OK) { - return false; + return "CSurjectionCheck failed"; } } - return true; + return ""; } bool VerifyCoinbaseAmount(const CTransaction& tx, const CAmountMap& mapFees) { diff --git a/src/confidential_validation.h b/src/confidential_validation.h index a148ab701c..96c1bd809a 100644 --- a/src/confidential_validation.h +++ b/src/confidential_validation.h @@ -89,7 +89,7 @@ class CSurjectionCheck : public CCheck ScriptError QueueCheck(std::vector* queue, CCheck* check); -bool VerifyAmounts(const std::vector& inputs, const CTransaction& tx, std::vector* pvChecks, const bool cacheStore); +const std::string VerifyAmounts(const std::vector& inputs, const CTransaction& tx, std::vector* pvChecks, const bool cacheStore); bool VerifyCoinbaseAmount(const CTransaction& tx, const CAmountMap& mapFees); diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index b35f52a33f..03be5838d4 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -251,8 +251,12 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, } // Verify that amounts add up. - if (fScriptChecks && !VerifyAmounts(spent_inputs, tx, pvChecks, cacheStore)) { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-in-ne-out", "value in != value out"); + if (fScriptChecks) { + const std::string amounts_check_result = VerifyAmounts(spent_inputs, tx, pvChecks, cacheStore); + if (amounts_check_result.length() != 0) { + /* return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-in-ne-out", "value in != value out"); */ + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-in-ne-out", amounts_check_result); + } } fee_map += GetFeeMap(tx); if (!MoneyRange(fee_map)) { diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 0756e4b825..529e7441a4 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1075,6 +1075,7 @@ static RPCHelpMan testmempoolaccept() result_inner.pushKV("reject-reason", "missing-inputs"); } else { result_inner.pushKV("reject-reason", state.GetRejectReason()); + result_inner.pushKV("error", state.ToString()); } } rpc_result.push_back(result_inner); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1751f56f05..6a705427c1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2091,7 +2091,9 @@ TransactionError CWallet::SignPSBT(PartiallySignedTransaction& psbtx, bool& comp } CTransaction tx_tmp(tx); - if (!VerifyAmounts(inputs_utxos, tx_tmp, nullptr, false)) { + const std::string verify_amounts_result = VerifyAmounts(inputs_utxos, tx_tmp, nullptr, false); + if (verify_amounts_result.length() != 0) { + WalletLogPrintf("VerifyAmounts error: %s", verify_amounts_result); return TransactionError::VALUE_IMBALANCE; } }