Skip to content

Commit

Permalink
Merge #6130: backport: Merge bitcoin#22337
Browse files Browse the repository at this point in the history
1973532 Merge bitcoin#22337: wallet: Use bilingual_str for errors (Samuel Dobson)

Pull request description:

  bitcoin backport

Top commit has no ACKs.

Tree-SHA512: ec638bde047788824961c230804461cee96ddcc0dbe1928bfb8c675c7f7f8c0c104850676a70fb8c9380401ead6d5986022ae3698a2bbc84f679531b11349580
  • Loading branch information
PastaPastaPasta committed Aug 28, 2024
2 parents b9a2f08 + 1973532 commit 9bcda44
Show file tree
Hide file tree
Showing 23 changed files with 85 additions and 69 deletions.
6 changes: 3 additions & 3 deletions src/coinjoin/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -638,14 +638,14 @@ bool CCoinJoinClientSession::SignFinalTransaction(CNode& peer, CChainState& acti

// fill values for found outpoints
m_wallet.chain().findCoins(coins);
std::map<int, std::string> signing_errors;
std::map<int, bilingual_str> signing_errors;
m_wallet.SignTransaction(finalMutableTransaction, coins, SIGHASH_ALL | SIGHASH_ANYONECANPAY, signing_errors);

for (const auto& [input_index, error_string] : signing_errors) {
// NOTE: this is a partial signing so it's expected for SignTransaction to return
// "Input not found or already spent" errors for inputs that aren't ours
if (error_string != "Input not found or already spent") {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- signing input %d failed: %s!\n", __func__, input_index, error_string);
if (error_string.original != "Input not found or already spent") {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- signing input %d failed: %s!\n", __func__, input_index, error_string.original);
UnlockCoins();
keyHolderStorage.ReturnAll();
SetNull();
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/chain.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ class Chain
virtual bool broadcastTransaction(const CTransactionRef& tx,
const CAmount& max_tx_fee,
bool relay,
std::string& err_string) = 0;
bilingual_str& err_string) = 0;

//! Calculate mempool ancestor and descendant counts for the given transaction.
virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -879,7 +879,7 @@ class ChainImpl : public Chain
auto it = m_node.mempool->GetIter(txid);
return it && (*it)->GetCountWithDescendants() > 1;
}
bool broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, std::string& err_string) override
bool broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) override
{
const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
// Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures.
Expand Down
7 changes: 4 additions & 3 deletions src/node/transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <validationinterface.h>
#include <node/context.h>
#include <node/transaction.h>
#include <util/translation.h>

#include <future>

Expand All @@ -28,7 +29,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
}
}

TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits)
TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, bilingual_str& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits)
{
// BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs.
// node.peerman is assigned both before chain clients and before RPC server is accepting calls,
Expand Down Expand Up @@ -59,7 +60,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx,
bypass_limits, true /* test_accept */);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string);
return HandleATMPError(result.m_state, err_string.original);
} else if (result.m_base_fees.value() > max_tx_fee) {
return TransactionError::MAX_FEE_EXCEEDED;
}
Expand All @@ -68,7 +69,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx,
bypass_limits, false /* test_accept */);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string);
return HandleATMPError(result.m_state, err_string.original);
}

// Transaction was accepted to the mempool.
Expand Down
4 changes: 2 additions & 2 deletions src/node/transaction.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
*
* @param[in] node reference to node context
* @param[in] tx the transaction to broadcast
* @param[out] err_string reference to std::string to fill with error string if available
* @param[out] err_string reference to bilingual_str to fill with error string if available
* @param[in] relay flag if both mempool insertion and p2p relay are requested
* @param[in] wait_callback wait until callbacks have been processed to avoid stale result due to a sequentially RPC.
* return error
*/
[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const CAmount& highfee, bool relay, bool wait_callback, bool bypass_limits = false);
[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, bilingual_str& err_string, const CAmount& highfee, bool relay, bool wait_callback, bool bypass_limits = false);

/**
* Return transaction with a given hash.
Expand Down
2 changes: 1 addition & 1 deletion src/qt/psbtoperationsdialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ void PSBTOperationsDialog::broadcastTransaction()
}

CTransactionRef tx = MakeTransactionRef(mtx);
std::string err_string;
bilingual_str err_string;
TransactionError error = BroadcastTransaction(
*m_client_model->node().context(), tx, err_string, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), /* relay */ true, /* await_callback */ false);

Expand Down
5 changes: 3 additions & 2 deletions src/rpc/rawtransaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <util/moneystr.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <util/translation.h>
#include <validation.h>
#include <validationinterface.h>
#include <util/irange.h>
Expand Down Expand Up @@ -1165,12 +1166,12 @@ RPCHelpMan sendrawtransaction()

bool bypass_limits = false;
if (!request.params[3].isNull()) bypass_limits = request.params[3].get_bool();
std::string err_string;
bilingual_str err_string;
AssertLockNotHeld(cs_main);
NodeContext& node = EnsureAnyNodeContext(request.context);
const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, /* relay */ true, /* wait_callback */ true, bypass_limits);
if (TransactionError::OK != err) {
throw JSONRPCTransactionError(err, err_string);
throw JSONRPCTransactionError(err, err_string.original);
}

return tx->GetHash().GetHex();
Expand Down
9 changes: 5 additions & 4 deletions src/rpc/rawtransaction_util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <util/strencodings.h>
#include <validation.h>
#include <txmempool.h>
#include <util/translation.h>

CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime)
{
Expand Down Expand Up @@ -227,22 +228,22 @@ void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
int nHashType = ParseSighashString(hashType);

// Script verification errors
std::map<int, std::string> input_errors;
std::map<int, bilingual_str> input_errors;

bool complete = SignTransaction(mtx, keystore, coins, nHashType, input_errors);
SignTransactionResultToJSON(mtx, complete, coins, input_errors, result);
}

void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const std::map<COutPoint, Coin>& coins, const std::map<int, std::string>& input_errors, UniValue& result)
void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const std::map<COutPoint, Coin>& coins, const std::map<int, bilingual_str>& input_errors, UniValue& result)
{
// Make errors UniValue
UniValue vErrors(UniValue::VARR);
for (const auto& err_pair : input_errors) {
if (err_pair.second == "Missing amount") {
if (err_pair.second.original == "Missing amount") {
// This particular error needs to be an exception for some reason
throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing amount for %s", coins.at(mtx.vin.at(err_pair.first).prevout).out.ToString()));
}
TxInErrorToJSON(mtx.vin.at(err_pair.first), vErrors, err_pair.second);
TxInErrorToJSON(mtx.vin.at(err_pair.first), vErrors, err_pair.second.original);
}

result.pushKV("hex", EncodeHexTx(CTransaction(mtx)));
Expand Down
3 changes: 2 additions & 1 deletion src/rpc/rawtransaction_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <map>
#include <string>

struct bilingual_str;
class FillableSigningProvider;
class UniValue;
struct CMutableTransaction;
Expand All @@ -25,7 +26,7 @@ class SigningProvider;
* @param result JSON object where signed transaction results accumulate
*/
void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result);
void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const std::map<COutPoint, Coin>& coins, const std::map<int, std::string>& input_errors, UniValue& result);
void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const std::map<COutPoint, Coin>& coins, const std::map<int, bilingual_str>& input_errors, UniValue& result);

/**
* Parse a prevtxs UniValue array and get the map of coins from it
Expand Down
11 changes: 6 additions & 5 deletions src/script/sign.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <script/signingprovider.h>
#include <script/standard.h>
#include <uint256.h>
#include <util/translation.h>

typedef std::vector<unsigned char> valtype;

Expand Down Expand Up @@ -385,7 +386,7 @@ bool IsSolvable(const SigningProvider& provider, const CScript& script)
return false;
}

bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, int nHashType, std::map<int, std::string>& input_errors)
bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, int nHashType, std::map<int, bilingual_str>& input_errors)
{
bool fHashSingle = ((nHashType & ~SIGHASH_ANYONECANPAY) == SIGHASH_SINGLE);

Expand All @@ -397,7 +398,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
CTxIn& txin = mtx.vin[i];
auto coin = coins.find(txin.prevout);
if (coin == coins.end() || coin->second.IsSpent()) {
input_errors[i] = "Input not found or already spent";
input_errors[i] = _("Input not found or already spent");
continue;
}
const CScript& prevPubKey = coin->second.out.scriptPubKey;
Expand All @@ -415,12 +416,12 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount), &serror)) {
if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
// Unable to sign input and verification failed (possible attempt to partially sign).
input_errors[i] = "Unable to sign input, invalid stack size (possibly missing key)";
input_errors[i] = Untranslated("Unable to sign input, invalid stack size (possibly missing key)");
} else if (serror == SCRIPT_ERR_SIG_NULLFAIL) {
// Verification failed (possibly due to insufficient signatures).
input_errors[i] = "CHECK(MULTI)SIG failing with non-zero signature (possibly need more signatures)";
input_errors[i] = Untranslated("CHECK(MULTI)SIG failing with non-zero signature (possibly need more signatures)");
} else {
input_errors[i] = ScriptErrorString(serror);
input_errors[i] = Untranslated(ScriptErrorString(serror));
}
} else {
// If this input succeeds, make sure there is no error set for it
Expand Down
3 changes: 2 additions & 1 deletion src/script/sign.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class CScriptID;
class CTransaction;
class SigningProvider;

struct bilingual_str;
struct CMutableTransaction;

/** Interface for signature creators. */
Expand Down Expand Up @@ -163,6 +164,6 @@ void UpdateInput(CTxIn& input, const SignatureData& data);
bool IsSolvable(const SigningProvider& provider, const CScript& script);

/** Sign the CMutableTransaction */
bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* provider, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors);
bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* provider, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, bilingual_str>& input_errors);

#endif // BITCOIN_SCRIPT_SIGN_H
3 changes: 2 additions & 1 deletion src/test/fuzz/script_sign.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <test/fuzz/FuzzedDataProvider.h>
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
#include <util/translation.h>

#include <cassert>
#include <cstdint>
Expand Down Expand Up @@ -134,7 +135,7 @@ FUZZ_TARGET_INIT(script_sign, initialize_script_sign)
}
coins[*outpoint] = *coin;
}
std::map<int, std::string> input_errors;
std::map<int, bilingual_str> input_errors;
// (void)SignTransaction(sign_transaction_tx_to, &provider, coins, fuzzed_data_provider.ConsumeIntegral<int>(), input_errors);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/util/setup_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ CMutableTransaction TestChainSetup::CreateValidMempoolTransaction(CTransactionRe
input_coins.insert({outpoint_to_spend, utxo_to_spend});
// - Default signature hashing type
int nHashType = SIGHASH_ALL;
std::map<int, std::string> input_errors;
std::map<int, bilingual_str> input_errors;
assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors));

// If submit=true, add transaction to the mempool.
Expand Down
3 changes: 2 additions & 1 deletion src/test/util/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <outputtype.h>
#include <script/standard.h>
#ifdef ENABLE_WALLET
#include <util/translation.h>
#include <wallet/wallet.h>
#endif

Expand All @@ -18,7 +19,7 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq
std::string getnewaddress(CWallet& w)
{
CTxDestination dest;
std::string error;
bilingual_str error;
if (!w.GetNewDestination("", dest, error)) assert(false);

return EncodeDestination(dest);
Expand Down
6 changes: 6 additions & 0 deletions src/util/translation.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ struct bilingual_str {
{
return original.empty();
}

void clear()
{
original.clear();
translated.clear();
}
};

inline bilingual_str operator+(bilingual_str lhs, const bilingual_str& rhs)
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <uint256.h>
#include <util/check.h>
#include <util/system.h>
#include <util/translation.h>
#include <util/ui_change_type.h>
#include <validation.h>
#include <wallet/context.h>
Expand Down Expand Up @@ -156,7 +157,7 @@ class WalletImpl : public Wallet
bool getNewDestination(const std::string label, CTxDestination& dest) override
{
LOCK(m_wallet->cs_wallet);
std::string error;
bilingual_str error;
return m_wallet->GetNewDestination(label, dest, error);
}
bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override
Expand Down
10 changes: 5 additions & 5 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,9 @@ RPCHelpMan getnewaddress()
label = LabelFromValue(request.params[0]);

CTxDestination dest;
std::string error;
bilingual_str error;
if (!pwallet->GetNewDestination(label, dest, error)) {
throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error);
throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error.original);
}
return EncodeDestination(dest);
},
Expand Down Expand Up @@ -303,9 +303,9 @@ RPCHelpMan getrawchangeaddress()
}

CTxDestination dest;
std::string error;
bilingual_str error;
if (!pwallet->GetNewChangeDestination(dest, error)) {
throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error);
throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error.original);
}
return EncodeDestination(dest);
},
Expand Down Expand Up @@ -3669,7 +3669,7 @@ RPCHelpMan signrawtransactionwithwallet()
int nHashType = ParseSighashString(request.params[2]);

// Script verification errors
std::map<int, std::string> input_errors;
std::map<int, bilingual_str> input_errors;

bool complete = pwallet->SignTransaction(mtx, coins, nHashType, input_errors);
UniValue result(UniValue::VOBJ);
Expand Down
Loading

0 comments on commit 9bcda44

Please sign in to comment.