From d625b907a1800a5a30c4ad285641c7418d2c28c1 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 26 Jan 2017 12:57:04 -0500 Subject: [PATCH 1/3] wallet: Refactor dummy signature signing for reusability --- src/wallet/wallet.cpp | 18 +++--------------- src/wallet/wallet.h | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9a5f35b6e3..a7b8022bd9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2583,21 +2583,9 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt std::numeric_limits::max() - (fWalletRbf ? 2 : 1))); // Fill in dummy signatures for fee calculation. - int nIn = 0; - for (const auto& coin : setCoins) - { - const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; - SignatureData sigdata; - - if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) - { - strFailReason = _("Signing transaction failed"); - return false; - } else { - UpdateTransaction(txNew, nIn, sigdata); - } - - nIn++; + if (!DummySignTx(txNew, setCoins)) { + strFailReason = _("Signing transaction failed"); + return false; } unsigned int nBytes = GetVirtualTransactionSize(txNew); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 200ec0cba2..1de04ae16a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -13,6 +13,7 @@ #include "utilstrencodings.h" #include "validationinterface.h" #include "script/ismine.h" +#include "script/sign.h" #include "wallet/crypter.h" #include "wallet/walletdb.h" #include "wallet/rpcwallet.h" @@ -796,6 +797,8 @@ public: void ListAccountCreditDebit(const std::string& strAccount, std::list& entries); bool AddAccountingEntry(const CAccountingEntry&); bool AddAccountingEntry(const CAccountingEntry&, CWalletDB *pwalletdb); + template + bool DummySignTx(CMutableTransaction &txNew, const ContainerType &coins); static CFeeRate minTxFee; static CFeeRate fallbackFee; @@ -1028,4 +1031,28 @@ public: } }; +// Helper for producing a bunch of max-sized low-S signatures (eg 72 bytes) +// ContainerType is meant to hold pair, and be iterable +// so that each entry corresponds to each vIn, in order. +template +bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins) +{ + // Fill in dummy signatures for fee calculation. + int nIn = 0; + for (const auto& coin : coins) + { + const CScript& scriptPubKey = coin.first->tx->vout[coin.second].scriptPubKey; + SignatureData sigdata; + + if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) + { + return false; + } else { + UpdateTransaction(txNew, nIn, sigdata); + } + + nIn++; + } + return true; +} #endif // BITCOIN_WALLET_WALLET_H From f62659448cdfd752447117fb73ab5328fbe3e41d Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 26 Jan 2017 12:59:32 -0500 Subject: [PATCH 2/3] rpc: bumpfee: use correct maximum signed tx size for fee calculation More accurate than simply adding one byte per input, and properly handles the case where the original transaction happened to have very small signatures --- src/wallet/rpcwallet.cpp | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index bfc738afad..4f763fd6e2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2664,6 +2664,33 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) return result; } +// Calculate the size of the transaction assuming all signatures are max size +// Use DummySignatureCreator, which inserts 72 byte signatures everywhere. +// TODO: re-use this in CWallet::CreateTransaction (right now +// CreateTransaction uses the constructed dummy-signed tx to do a priority +// calculation, but we should be able to refactor after priority is removed). +// NOTE: this requires that all inputs must be in mapWallet (eg the tx should +// be IsAllFromMe). +int64_t CalculateMaximumSignedTxSize(const CTransaction &tx) +{ + CMutableTransaction txNew(tx); + std::vector> vCoins; + // Look up the inputs. We should have already checked that this transaction + // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our + // wallet, with a valid index into the vout array. + for (auto& input : tx.vin) { + const auto mi = pwalletMain->mapWallet.find(input.prevout.hash); + assert(mi != pwalletMain->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size()); + vCoins.emplace_back(make_pair(&(mi->second), input.prevout.n)); + } + if (!pwalletMain->DummySignTx(txNew, vCoins)) { + // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE) + // implies that we can sign for every input. + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction contains inputs that cannot be signed"); + } + return GetVirtualTransactionSize(txNew); +} + UniValue bumpfee(const JSONRPCRequest& request) { if (!EnsureWalletIsAvailable(request.fHelp)) { @@ -2769,9 +2796,9 @@ UniValue bumpfee(const JSONRPCRequest& request) throw JSONRPCError(RPC_MISC_ERROR, "Transaction does not have a change output"); } - // signature sizes can vary by a byte, so add 1 for each input when calculating the new fee + // Calculate the expected size of the new transaction. int64_t txSize = GetVirtualTransactionSize(*(wtx.tx)); - const int64_t maxNewTxSize = txSize + wtx.tx->vin.size(); + const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx); // optional parameters bool specifiedConfirmTarget = false; @@ -2845,8 +2872,12 @@ UniValue bumpfee(const JSONRPCRequest& request) nNewFeeRate = CFeeRate(nNewFee, maxNewTxSize); // New fee rate must be at least old rate + minimum incremental relay rate - if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + walletIncrementalRelayFee.GetFeePerK()) { - nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + walletIncrementalRelayFee.GetFeePerK()); + // walletIncrementalRelayFee.GetFeePerK() should be exact, because it's initialized + // in that unit (fee per kb). + // However, nOldFeeRate is a calculated value from the tx fee/size, so + // add 1 satoshi to the result, because it may have been rounded down. + if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + 1 + walletIncrementalRelayFee.GetFeePerK()) { + nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + 1 + walletIncrementalRelayFee.GetFeePerK()); nNewFee = nNewFeeRate.GetFee(maxNewTxSize); } } From 9522b53a91f28032c34b94662d50b000534708ce Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 26 Jan 2017 12:57:42 -0500 Subject: [PATCH 3/3] rpc: bumpfee: handle errors more gracefully --- src/wallet/rpcwallet.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4f763fd6e2..8b135b0e0f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2733,6 +2733,7 @@ UniValue bumpfee(const JSONRPCRequest& request) " \"txid\": \"value\", (string) The id of the new transaction\n" " \"origfee\": n, (numeric) Fee of the replaced transaction\n" " \"fee\": n, (numeric) Fee of the new transaction\n" + " \"errors\": [ str... ] (json array of strings) Errors encountered during processing (may be empty)\n" "}\n" "\nExamples:\n" "\nBump the fee, get the new transaction\'s txid\n" + @@ -2945,23 +2946,32 @@ UniValue bumpfee(const JSONRPCRequest& request) CWalletTx wtxBumped(pwalletMain, MakeTransactionRef(std::move(tx))); wtxBumped.mapValue["replaces_txid"] = hash.ToString(); CValidationState state; - if (!pwalletMain->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state) || !state.IsValid()) { + if (!pwalletMain->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) { + // NOTE: CommitTransaction never returns false, so this should never happen. throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Error: The transaction was rejected! Reason given: %s", state.GetRejectReason())); } + UniValue vErrors(UniValue::VARR); + if (state.IsInvalid()) { + // This can happen if the mempool rejected the transaction. Report + // what happened in the "errors" response. + vErrors.push_back(strprintf("Error: The transaction was rejected: %s", FormatStateMessage(state))); + } + // mark the original tx as bumped if (!pwalletMain->MarkReplaced(wtx.GetHash(), wtxBumped.GetHash())) { // TODO: see if JSON-RPC has a standard way of returning a response // along with an exception. It would be good to return information about // wtxBumped to the caller even if marking the original transaction // replaced does not succeed for some reason. - throw JSONRPCError(RPC_WALLET_ERROR, "Error: Created new bumpfee transaction but could not mark the original transaction as replaced."); + vErrors.push_back("Error: Created new bumpfee transaction but could not mark the original transaction as replaced."); } UniValue result(UniValue::VOBJ); result.push_back(Pair("txid", wtxBumped.GetHash().GetHex())); result.push_back(Pair("origfee", ValueFromAmount(nOldFee))); result.push_back(Pair("fee", ValueFromAmount(nNewFee))); + result.push_back(Pair("errors", vErrors)); return result; }