aboutsummaryrefslogtreecommitdiff
path: root/src
diff options
context:
space:
mode:
authorWladimir J. van der Laan <laanwj@gmail.com>2017-02-01 08:41:21 +0100
committerWladimir J. van der Laan <laanwj@gmail.com>2017-02-01 08:42:53 +0100
commit7bfb77045c4b06db3597c023de97411ed287e252 (patch)
treeb9fb2476ca21c7d62366eab57aef7cdf5ec791a9 /src
parente99f0d7ad443ff72087c6f80e9fab65cace0bf19 (diff)
parent9522b53a91f28032c34b94662d50b000534708ce (diff)
Merge #9640: Bumpfee: bugfixes for error handling and feerate calculation
9522b53 rpc: bumpfee: handle errors more gracefully (Suhas Daftuar) f626594 rpc: bumpfee: use correct maximum signed tx size for fee calculation (Suhas Daftuar) d625b90 wallet: Refactor dummy signature signing for reusability (Suhas Daftuar)
Diffstat (limited to 'src')
-rw-r--r--src/wallet/rpcwallet.cpp53
-rw-r--r--src/wallet/wallet.cpp18
-rw-r--r--src/wallet/wallet.h27
3 files changed, 77 insertions, 21 deletions
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index bfc738afad..8b135b0e0f 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<pair<CWalletTx *, unsigned int>> 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)) {
@@ -2706,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" +
@@ -2769,9 +2797,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 +2873,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);
}
}
@@ -2914,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;
}
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<CRecipient>& vecSend, CWalletTx& wt
std::numeric_limits<unsigned int>::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<CAccountingEntry>& entries);
bool AddAccountingEntry(const CAccountingEntry&);
bool AddAccountingEntry(const CAccountingEntry&, CWalletDB *pwalletdb);
+ template <typename ContainerType>
+ 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<CWalletTx *, int>, and be iterable
+// so that each entry corresponds to each vIn, in order.
+template <typename ContainerType>
+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