diff options
Diffstat (limited to 'src')
37 files changed, 768 insertions, 446 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 66cb7cec2a..e2ed70556d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -351,6 +351,7 @@ libbitcoin_server_a_SOURCES = \ node/ui_interface.cpp \ noui.cpp \ policy/fees.cpp \ + policy/packages.cpp \ policy/rbf.cpp \ policy/settings.cpp \ pow.cpp \ diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index c279a9af2f..5beb833b48 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -56,7 +56,7 @@ static void CoinSelection(benchmark::Bench& bench) bench.run([&] { std::set<CInputCoin> setCoinsRet; CAmount nValueRet; - bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); + bool success = wallet.AttemptSelection(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); assert(success); assert(nValueRet == 1003 * COIN); assert(setCoinsRet.size() == 2); diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 1dd1e92e2f..35b6160cea 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -6,6 +6,7 @@ #define BITCOIN_INTERFACES_NODE_H #include <amount.h> // For CAmount +#include <external_signer.h> #include <net.h> // For NodeId #include <net_types.h> // For banmap_t #include <netaddress.h> // For Network @@ -110,6 +111,11 @@ public: //! Disconnect node by id. virtual bool disconnectById(NodeId id) = 0; +#ifdef ENABLE_EXTERNAL_SIGNER + //! List external signers + virtual std::vector<ExternalSigner> externalSigners() = 0; +#endif + //! Get total bytes recv. virtual int64_t getTotalBytesRecv() = 0; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 88f93321f9..a0cb2787b7 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -118,6 +118,9 @@ public: //! Save or remove receive request. virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; + //! Display address on external signer + virtual bool displayAddress(const CTxDestination& dest) = 0; + //! Lock coin. virtual void lockCoin(const COutPoint& output) = 0; @@ -252,6 +255,9 @@ public: // Return whether private keys enabled. virtual bool privateKeysDisabled() = 0; + // Return whether wallet uses an external signer. + virtual bool hasExternalSigner() = 0; + // Get default address type. virtual OutputType getDefaultAddressType() = 0; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 65224b4259..c5a389f228 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -159,10 +159,10 @@ static constexpr size_t MAX_ADDR_TO_SEND{1000}; namespace { /** Blocks that are in flight, and that are in the queue to be downloaded. */ struct QueuedBlock { - uint256 hash; - const CBlockIndex* pindex; //!< Optional. - bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request. - std::unique_ptr<PartiallyDownloadedBlock> partialBlock; //!< Optional, used for CMPCTBLOCK downloads + /** BlockIndex. We must have this since we only request blocks when we've already validated the header. */ + const CBlockIndex* pindex; + /** Optional, used for CMPCTBLOCK downloads */ + std::unique_ptr<PartiallyDownloadedBlock> partialBlock; }; /** @@ -463,16 +463,20 @@ private: Mutex m_recent_confirmed_transactions_mutex; std::unique_ptr<CRollingBloomFilter> m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex); - /* Returns a bool indicating whether we requested this block. - * Also used if a block was /not/ received and timed out or started with another peer + /** Have we requested this block from a peer */ + bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + + /** Remove this block from our tracked requested blocks. Called if: + * - the block has been recieved from a peer + * - the request for the block has timed out */ - bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void RemoveBlockRequest(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /* Mark a block as in flight * Returns false, still setting pit, if the block was already in flight from the same peer * pit will only be valid as long as the same cs_main lock is being held */ - bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool BlockRequested(NodeId nodeid, const CBlockIndex* pindex, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -512,7 +516,7 @@ private: std::list<NodeId> lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main); /** Number of peers from which we're downloading blocks. */ - int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0; + int m_peers_downloading_from GUARDED_BY(cs_main) = 0; /** Storage for orphan information */ TxOrphanage m_orphanage; @@ -627,7 +631,6 @@ struct CNodeState { //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty. std::chrono::microseconds m_downloading_since{0us}; int nBlocksInFlight{0}; - int nBlocksInFlightValidHeaders{0}; //! Whether we consider this a preferred download peer. bool fPreferredDownload{false}; //! Whether this peer wants invs or headers (when possible) for block announcements. @@ -758,32 +761,43 @@ static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUS nPreferredDownload += state->fPreferredDownload; } -bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) +bool PeerManagerImpl::IsBlockRequested(const uint256& hash) { - std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); - if (itInFlight != mapBlocksInFlight.end()) { - CNodeState *state = State(itInFlight->second.first); - assert(state != nullptr); - state->nBlocksInFlightValidHeaders -= itInFlight->second.second->fValidatedHeaders; - if (state->nBlocksInFlightValidHeaders == 0 && itInFlight->second.second->fValidatedHeaders) { - // Last validated block on the queue was received. - nPeersWithValidatedDownloads--; - } - if (state->vBlocksInFlight.begin() == itInFlight->second.second) { - // First block on the queue was received, update the start download time for the next one - state->m_downloading_since = std::max(state->m_downloading_since, GetTime<std::chrono::microseconds>()); - } - state->vBlocksInFlight.erase(itInFlight->second.second); - state->nBlocksInFlight--; - state->m_stalling_since = 0us; - mapBlocksInFlight.erase(itInFlight); - return true; + return mapBlocksInFlight.find(hash) != mapBlocksInFlight.end(); +} + +void PeerManagerImpl::RemoveBlockRequest(const uint256& hash) +{ + auto it = mapBlocksInFlight.find(hash); + if (it == mapBlocksInFlight.end()) { + // Block was not requested + return; } - return false; + + auto [node_id, list_it] = it->second; + CNodeState *state = State(node_id); + assert(state != nullptr); + + if (state->vBlocksInFlight.begin() == list_it) { + // First block on the queue was received, update the start download time for the next one + state->m_downloading_since = std::max(state->m_downloading_since, GetTime<std::chrono::microseconds>()); + } + state->vBlocksInFlight.erase(list_it); + + state->nBlocksInFlight--; + if (state->nBlocksInFlight == 0) { + // Last validated block on the queue was received. + m_peers_downloading_from--; + } + state->m_stalling_since = 0us; + mapBlocksInFlight.erase(it); } -bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex, std::list<QueuedBlock>::iterator** pit) +bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex* pindex, std::list<QueuedBlock>::iterator** pit) { + assert(pindex); + const uint256& hash{pindex->GetBlockHash()}; + CNodeState *state = State(nodeid); assert(state != nullptr); @@ -797,18 +811,15 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, co } // Make sure it's not listed somewhere already. - MarkBlockAsReceived(hash); + RemoveBlockRequest(hash); std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), - {hash, pindex, pindex != nullptr, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); + {pindex, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); state->nBlocksInFlight++; - state->nBlocksInFlightValidHeaders += it->fValidatedHeaders; if (state->nBlocksInFlight == 1) { // We're starting a block download (batch) from this peer. state->m_downloading_since = GetTime<std::chrono::microseconds>(); - } - if (state->nBlocksInFlightValidHeaders == 1 && pindex != nullptr) { - nPeersWithValidatedDownloads++; + m_peers_downloading_from++; } itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first; if (pit) @@ -978,7 +989,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count if (pindex->nStatus & BLOCK_HAVE_DATA || m_chainman.ActiveChain().Contains(pindex)) { if (pindex->HaveTxsDownloaded()) state->pindexLastCommonBlock = pindex; - } else if (mapBlocksInFlight.count(pindex->GetBlockHash()) == 0) { + } else if (!IsBlockRequested(pindex->GetBlockHash())) { // The block is not already downloaded, and not yet in flight. if (pindex->nHeight > nWindowEnd) { // We reached the end of the window. @@ -1129,13 +1140,13 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) nSyncStarted--; for (const QueuedBlock& entry : state->vBlocksInFlight) { - mapBlocksInFlight.erase(entry.hash); + mapBlocksInFlight.erase(entry.pindex->GetBlockHash()); } WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid)); m_txrequest.DisconnectedPeer(nodeid); nPreferredDownload -= state->fPreferredDownload; - nPeersWithValidatedDownloads -= (state->nBlocksInFlightValidHeaders != 0); - assert(nPeersWithValidatedDownloads >= 0); + m_peers_downloading_from -= (state->nBlocksInFlight != 0); + assert(m_peers_downloading_from >= 0); m_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect; assert(m_outbound_peers_with_protect_from_disconnect >= 0); m_wtxid_relay_peers -= state->m_wtxid_relay; @@ -1147,7 +1158,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) // Do a consistency check after the last peer is removed. assert(mapBlocksInFlight.empty()); assert(nPreferredDownload == 0); - assert(nPeersWithValidatedDownloads == 0); + assert(m_peers_downloading_from == 0); assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); assert(m_txrequest.Size() == 0); @@ -2056,7 +2067,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, // Calculate all the blocks we'd need to switch to pindexLast, up to a limit. while (pindexWalk && !m_chainman.ActiveChain().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && - !mapBlocksInFlight.count(pindexWalk->GetBlockHash()) && + !IsBlockRequested(pindexWalk->GetBlockHash()) && (!IsWitnessEnabled(pindexWalk->pprev, m_chainparams.GetConsensus()) || State(pfrom.GetId())->fHaveWitness)) { // We don't have this block, and it's not yet in flight. vToFetch.push_back(pindexWalk); @@ -2081,7 +2092,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, } uint32_t nFetchFlags = GetFetchFlags(pfrom); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(pfrom.GetId(), pindex->GetBlockHash(), pindex); + BlockRequested(pfrom.GetId(), pindex); LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n", pindex->GetBlockHash().ToString(), pfrom.GetId()); } @@ -2827,7 +2838,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); UpdateBlockAvailability(pfrom.GetId(), inv.hash); - if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { + if (!fAlreadyHave && !fImporting && !fReindex && !IsBlockRequested(inv.hash)) { // Headers-first is the primary method of announcement on // the network. If a node fell back to sending blocks by inv, // it's probably for a re-org. The final block hash @@ -3384,7 +3395,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) || (fAlreadyInFlight && blockInFlightIt->second.first == pfrom.GetId())) { std::list<QueuedBlock>::iterator* queuedBlockIt = nullptr; - if (!MarkBlockAsInFlight(pfrom.GetId(), pindex->GetBlockHash(), pindex, &queuedBlockIt)) { + if (!BlockRequested(pfrom.GetId(), pindex, &queuedBlockIt)) { if (!(*queuedBlockIt)->partialBlock) (*queuedBlockIt)->partialBlock.reset(new PartiallyDownloadedBlock(&m_mempool)); else { @@ -3397,7 +3408,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock; ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect + RemoveBlockRequest(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect Misbehaving(pfrom.GetId(), 100, "invalid compact block"); return; } else if (status == READ_STATUS_FAILED) { @@ -3492,7 +3503,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // process from some other peer. We do this after calling // ProcessNewBlock so that a malleated cmpctblock announcement // can't be used to interfere with block relay. - MarkBlockAsReceived(pblock->GetHash()); + RemoveBlockRequest(pblock->GetHash()); } } return; @@ -3524,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect + RemoveBlockRequest(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect Misbehaving(pfrom.GetId(), 100, "invalid compact block/non-matching block transactions"); return; } else if (status == READ_STATUS_FAILED) { @@ -3550,7 +3561,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // though the block was successfully read, and rely on the // handling in ProcessNewBlock to ensure the block index is // updated, etc. - MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer + RemoveBlockRequest(resp.blockhash); // it is now an empty pointer fBlockRead = true; // mapBlockSource is used for potentially punishing peers and // updating which peers send us compact blocks, so the race @@ -3615,9 +3626,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const uint256 hash(pblock->GetHash()); { LOCK(cs_main); - // Also always process if we requested the block explicitly, as we may - // need it even though it is not a candidate for a new best tip. - forceProcessing |= MarkBlockAsReceived(hash); + // Always process the block if we requested it, since we may + // need it even when it's not a candidate for a new best tip. + forceProcessing = IsBlockRequested(hash); + RemoveBlockRequest(hash); // mapBlockSource is only used for punishing peers and setting // which peers send us compact blocks, so the race between here and // cs_main in ProcessNewBlock is fine. @@ -4712,9 +4724,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // to unreasonably increase our timeout. if (state.vBlocksInFlight.size() > 0) { QueuedBlock &queuedBlock = state.vBlocksInFlight.front(); - int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - (state.nBlocksInFlightValidHeaders > 0); + int nOtherPeersWithValidatedDownloads = m_peers_downloading_from - 1; if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) { - LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.hash.ToString(), pto->GetId()); + LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.pindex->GetBlockHash().ToString(), pto->GetId()); pto->fDisconnect = true; return true; } @@ -4767,7 +4779,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const CBlockIndex *pindex : vToDownload) { uint32_t nFetchFlags = GetFetchFlags(*pto); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(pto->GetId(), pindex->GetBlockHash(), pindex); + BlockRequested(pto->GetId(), pindex); LogPrint(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(), pindex->nHeight, pto->GetId()); } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8befbf5e30..171f15d4fb 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -170,6 +170,16 @@ public: } return false; } +#ifdef ENABLE_EXTERNAL_SIGNER + std::vector<ExternalSigner> externalSigners() override + { + std::vector<ExternalSigner> signers = {}; + const std::string command = gArgs.GetArg("-signer", ""); + if (command == "") return signers; + ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); + return signers; + } +#endif int64_t getTotalBytesRecv() override { return m_context->connman ? m_context->connman->GetTotalBytesRecv() : 0; } int64_t getTotalBytesSent() override { return m_context->connman ? m_context->connman->GetTotalBytesSent() : 0; } size_t getMempoolSize() override { return m_context->mempool ? m_context->mempool->size() : 0; } diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp new file mode 100644 index 0000000000..cfd0539965 --- /dev/null +++ b/src/policy/packages.cpp @@ -0,0 +1,62 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include <consensus/validation.h> +#include <policy/packages.h> +#include <primitives/transaction.h> +#include <uint256.h> +#include <util/hasher.h> + +#include <numeric> +#include <unordered_set> + +bool CheckPackage(const Package& txns, PackageValidationState& state) +{ + const unsigned int package_count = txns.size(); + + if (package_count > MAX_PACKAGE_COUNT) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); + } + + const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, + [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); + // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. + if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); + } + + // Require the package to be sorted in order of dependency, i.e. parents appear before children. + // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and + // fail on something less ambiguous (missing-inputs could also be an orphan or trying to + // spend nonexistent coins). + std::unordered_set<uint256, SaltedTxidHasher> later_txids; + std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), + [](const auto& tx) { return tx->GetHash(); }); + for (const auto& tx : txns) { + for (const auto& input : tx->vin) { + if (later_txids.find(input.prevout.hash) != later_txids.end()) { + // The parent is a subsequent transaction in the package. + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); + } + } + later_txids.erase(tx->GetHash()); + } + + // Don't allow any conflicting transactions, i.e. spending the same inputs, in a package. + std::unordered_set<COutPoint, SaltedOutpointHasher> inputs_seen; + for (const auto& tx : txns) { + for (const auto& input : tx->vin) { + if (inputs_seen.find(input.prevout) != inputs_seen.end()) { + // This input is also present in another tx in the package. + return state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); + } + } + // Batch-add all the inputs for a tx at a time. If we added them 1 at a time, we could + // catch duplicate inputs within a single tx. This is a more severe, consensus error, + // and we want to report that from CheckTransaction instead. + std::transform(tx->vin.cbegin(), tx->vin.cend(), std::inserter(inputs_seen, inputs_seen.end()), + [](const auto& input) { return input.prevout; }); + } + return true; +} diff --git a/src/policy/packages.h b/src/policy/packages.h index 4b1463dcb3..6b7ac3e450 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -6,6 +6,7 @@ #define BITCOIN_POLICY_PACKAGES_H #include <consensus/validation.h> +#include <policy/policy.h> #include <primitives/transaction.h> #include <vector> @@ -14,6 +15,7 @@ static constexpr uint32_t MAX_PACKAGE_COUNT{25}; /** Default maximum total virtual size of transactions in a package in KvB. */ static constexpr uint32_t MAX_PACKAGE_SIZE{101}; +static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT); /** A "reason" why a package was invalid. It may be that one or more of the included * transactions is invalid or the package itself violates our rules. @@ -31,4 +33,12 @@ using Package = std::vector<CTransactionRef>; class PackageValidationState : public ValidationState<PackageValidationResult> {}; +/** Context-free package policy checks: + * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT. + * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE. + * 3. If any dependencies exist between transactions, parents must appear before children. + * 4. Transactions cannot conflict, i.e., spend the same inputs. + */ +bool CheckPackage(const Package& txns, PackageValidationState& state); + #endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp index 113bd30a0c..e593697b46 100644 --- a/src/qt/createwalletdialog.cpp +++ b/src/qt/createwalletdialog.cpp @@ -6,6 +6,7 @@ #include <config/bitcoin-config.h> #endif +#include <external_signer.h> #include <qt/createwalletdialog.h> #include <qt/forms/ui_createwalletdialog.h> @@ -27,14 +28,39 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : }); connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) { - // Disable the disable_privkeys_checkbox when isEncryptWalletChecked is + // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is // set to true, enable it when isEncryptWalletChecked is false. ui->disable_privkeys_checkbox->setEnabled(!checked); + ui->external_signer_checkbox->setEnabled(!checked); // When the disable_privkeys_checkbox is disabled, uncheck it. if (!ui->disable_privkeys_checkbox->isEnabled()) { ui->disable_privkeys_checkbox->setChecked(false); } + + // When the external_signer_checkbox box is disabled, uncheck it. + if (!ui->external_signer_checkbox->isEnabled()) { + ui->external_signer_checkbox->setChecked(false); + } + + }); + + connect(ui->external_signer_checkbox, &QCheckBox::toggled, [this](bool checked) { + ui->encrypt_wallet_checkbox->setEnabled(!checked); + ui->blank_wallet_checkbox->setEnabled(!checked); + ui->disable_privkeys_checkbox->setEnabled(!checked); + ui->descriptor_checkbox->setEnabled(!checked); + + // The external signer checkbox is only enabled when a device is detected. + // In that case it is checked by default. Toggling it restores the other + // options to their default. + ui->descriptor_checkbox->setChecked(checked); + ui->encrypt_wallet_checkbox->setChecked(false); + ui->disable_privkeys_checkbox->setChecked(checked); + // The blank check box is ambiguous. This flag is always true for a + // watch-only wallet, even though we immedidately fetch keys from the + // external signer. + ui->blank_wallet_checkbox->setChecked(checked); }); connect(ui->disable_privkeys_checkbox, &QCheckBox::toggled, [this](bool checked) { @@ -63,11 +89,22 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : ui->descriptor_checkbox->setToolTip(tr("Compiled without sqlite support (required for descriptor wallets)")); ui->descriptor_checkbox->setEnabled(false); ui->descriptor_checkbox->setChecked(false); + ui->external_signer_checkbox->setEnabled(false); + ui->external_signer_checkbox->setChecked(false); #endif + #ifndef USE_BDB ui->descriptor_checkbox->setEnabled(false); ui->descriptor_checkbox->setChecked(true); #endif + +#ifndef ENABLE_EXTERNAL_SIGNER + //: "External signing" means using devices such as hardware wallets. + ui->external_signer_checkbox->setToolTip(tr("Compiled without external signing support (required for external signing)")); + ui->external_signer_checkbox->setEnabled(false); + ui->external_signer_checkbox->setChecked(false); +#endif + } CreateWalletDialog::~CreateWalletDialog() @@ -75,6 +112,28 @@ CreateWalletDialog::~CreateWalletDialog() delete ui; } +#ifdef ENABLE_EXTERNAL_SIGNER +void CreateWalletDialog::setSigners(std::vector<ExternalSigner>& signers) +{ + if (!signers.empty()) { + ui->external_signer_checkbox->setEnabled(true); + ui->external_signer_checkbox->setChecked(true); + ui->encrypt_wallet_checkbox->setEnabled(false); + ui->encrypt_wallet_checkbox->setChecked(false); + // The order matters, because connect() is called when toggling a checkbox: + ui->blank_wallet_checkbox->setEnabled(false); + ui->blank_wallet_checkbox->setChecked(false); + ui->disable_privkeys_checkbox->setEnabled(false); + ui->disable_privkeys_checkbox->setChecked(true); + const std::string label = signers[0].m_name; + ui->wallet_name_line_edit->setText(QString::fromStdString(label)); + ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(true); + } else { + ui->external_signer_checkbox->setEnabled(false); + } +} +#endif + QString CreateWalletDialog::walletName() const { return ui->wallet_name_line_edit->text(); @@ -99,3 +158,8 @@ bool CreateWalletDialog::isDescriptorWalletChecked() const { return ui->descriptor_checkbox->isChecked(); } + +bool CreateWalletDialog::isExternalSignerChecked() const +{ + return ui->external_signer_checkbox->isChecked(); +} diff --git a/src/qt/createwalletdialog.h b/src/qt/createwalletdialog.h index 20cce937c8..585b1461f7 100644 --- a/src/qt/createwalletdialog.h +++ b/src/qt/createwalletdialog.h @@ -9,6 +9,10 @@ class WalletModel; +#ifdef ENABLE_EXTERNAL_SIGNER +class ExternalSigner; +#endif + namespace Ui { class CreateWalletDialog; } @@ -23,11 +27,16 @@ public: explicit CreateWalletDialog(QWidget* parent); virtual ~CreateWalletDialog(); +#ifdef ENABLE_EXTERNAL_SIGNER + void setSigners(std::vector<ExternalSigner>& signers); +#endif + QString walletName() const; bool isEncryptWalletChecked() const; bool isDisablePrivateKeysChecked() const; bool isMakeBlankWalletChecked() const; bool isDescriptorWalletChecked() const; + bool isExternalSignerChecked() const; private: Ui::CreateWalletDialog *ui; diff --git a/src/qt/forms/createwalletdialog.ui b/src/qt/forms/createwalletdialog.ui index 881869a46c..b11fb026b0 100644 --- a/src/qt/forms/createwalletdialog.ui +++ b/src/qt/forms/createwalletdialog.ui @@ -109,6 +109,16 @@ </property> </widget> </item> + <item> + <widget class="QCheckBox" name="external_signer_checkbox"> + <property name="toolTip"> + <string>Use an external signing device such as a hardware wallet. Configure the external signer script in wallet preferences first.</string> + </property> + <property name="text"> + <string>External signer</string> + </property> + </widget> + </item> </layout> </widget> </item> @@ -143,6 +153,7 @@ <tabstop>disable_privkeys_checkbox</tabstop> <tabstop>blank_wallet_checkbox</tabstop> <tabstop>descriptor_checkbox</tabstop> + <tabstop>external_signer_checkbox</tabstop> </tabstops> <resources/> <connections> diff --git a/src/qt/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui index f199e8c1a1..bd72328c02 100644 --- a/src/qt/forms/optionsdialog.ui +++ b/src/qt/forms/optionsdialog.ui @@ -230,6 +230,36 @@ </widget> </item> <item> + <widget class="QGroupBox" name="groupBoxHww"> + <property name="title"> + <string>External Signer (e.g. hardware wallet)</string> + </property> + <layout class="QVBoxLayout" name="verticalLayoutHww"> + <item> + <layout class="QHBoxLayout" name="horizontalLayoutHww"> + <item> + <widget class="QLabel" name="externalSignerPathLabel"> + <property name="text"> + <string>&External signer script path</string> + </property> + <property name="buddy"> + <cstring>externalSignerPath</cstring> + </property> + </widget> + </item> + <item> + <widget class="QLineEdit" name="externalSignerPath"> + <property name="toolTip"> + <string>Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string> + </property> + </widget> + </item> + </layout> + </item> + </layout> + </widget> + </item> + <item> <spacer name="verticalSpacer_Wallet"> <property name="orientation"> <enum>Qt::Vertical</enum> diff --git a/src/qt/forms/receiverequestdialog.ui b/src/qt/forms/receiverequestdialog.ui index 7d95a8bc90..70a7cf71de 100644 --- a/src/qt/forms/receiverequestdialog.ui +++ b/src/qt/forms/receiverequestdialog.ui @@ -255,6 +255,19 @@ </widget> </item> <item> + <widget class="QPushButton" name="btnVerify"> + <property name="text"> + <string>&Verify</string> + </property> + <property name="toolTip"> + <string>Verify this address on e.g. a hardware wallet screen</string> + </property> + <property name="autoDefault"> + <bool>false</bool> + </property> + </widget> + </item> + <item> <widget class="QPushButton" name="btnSaveAs"> <property name="text"> <string>&Save Image…</string> diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index 8a32994e3f..6ad8db4348 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -199,6 +199,7 @@ void OptionsDialog::setModel(OptionsModel *_model) connect(ui->prune, &QCheckBox::clicked, this, &OptionsDialog::togglePruneWarning); connect(ui->pruneSize, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); connect(ui->databaseCache, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); + connect(ui->externalSignerPath, &QLineEdit::textChanged, [this]{ showRestartWarning(); }); connect(ui->threadsScriptVerif, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); /* Wallet */ connect(ui->spendZeroConfChange, &QCheckBox::clicked, this, &OptionsDialog::showRestartWarning); @@ -233,6 +234,7 @@ void OptionsDialog::setMapper() /* Wallet */ mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange); mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures); + mapper->addMapping(ui->externalSignerPath, OptionsModel::ExternalSignerPath); /* Network */ mapper->addMapping(ui->mapPortUpnp, OptionsModel::MapPortUPnP); diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index abdf9e9ae6..24a4e9ee96 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -117,6 +117,13 @@ void OptionsModel::Init(bool resetSettings) settings.setValue("bSpendZeroConfChange", true); if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool())) addOverriddenOption("-spendzeroconfchange"); + + if (!settings.contains("external_signer_path")) + settings.setValue("external_signer_path", ""); + + if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) { + addOverriddenOption("-signer"); + } #endif // Network @@ -326,6 +333,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const #ifdef ENABLE_WALLET case SpendZeroConfChange: return settings.value("bSpendZeroConfChange"); + case ExternalSignerPath: + return settings.value("external_signer_path"); #endif case DisplayUnit: return nDisplayUnit; @@ -445,6 +454,12 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in setRestartRequired(true); } break; + case ExternalSignerPath: + if (settings.value("external_signer_path") != value.toString()) { + settings.setValue("external_signer_path", value.toString()); + setRestartRequired(true); + } + break; #endif case DisplayUnit: setDisplayUnit(value); diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h index 4d012a9b8f..535843e8ba 100644 --- a/src/qt/optionsmodel.h +++ b/src/qt/optionsmodel.h @@ -65,6 +65,7 @@ public: Prune, // bool PruneSize, // int DatabaseCache, // int + ExternalSignerPath, // QString SpendZeroConfChange, // bool Listen, // bool OptionIDRowCount, diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp index 78ae5c07da..abe7de8f89 100644 --- a/src/qt/receiverequestdialog.cpp +++ b/src/qt/receiverequestdialog.cpp @@ -89,6 +89,12 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) ui->wallet_tag->hide(); ui->wallet_content->hide(); } + + ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner()); + + connect(ui->btnVerify, &QPushButton::clicked, [this] { + model->displayAddress(info.address.toStdString()); + }); } void ReceiveRequestDialog::updateDisplayUnit() diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 160b43324f..e87a2b97bc 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -199,7 +199,16 @@ void SendCoinsDialog::setModel(WalletModel *_model) // set default rbf checkbox state ui->optInRBF->setCheckState(Qt::Checked); - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().hasExternalSigner()) { + ui->sendButton->setText(tr("Sign on device")); + if (gArgs.GetArg("-signer", "") != "") { + ui->sendButton->setEnabled(true); + ui->sendButton->setToolTip(tr("Connect your hardware wallet first.")); + } else { + ui->sendButton->setEnabled(false); + ui->sendButton->setToolTip(tr("Set external signer script path in Options -> Wallet")); + } + } else if (model->wallet().privateKeysDisabled()) { ui->sendButton->setText(tr("Cr&eate Unsigned")); ui->sendButton->setToolTip(tr("Creates a Partially Signed Bitcoin Transaction (PSBT) for use with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); } @@ -313,14 +322,14 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa formatted.append(recipientElement); } - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { question_string.append(tr("Do you want to draft this transaction?")); } else { question_string.append(tr("Are you sure you want to send?")); } question_string.append("<br /><span style='font-size:10pt;'>"); - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); } else { question_string.append(tr("Please, review your transaction.")); @@ -386,8 +395,8 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) if (!PrepareSendText(question_string, informative_text, detailed_text)) return; assert(m_current_transaction); - const QString confirmation = model->wallet().privateKeysDisabled() ? tr("Confirm transaction proposal") : tr("Confirm send coins"); - const QString confirmButtonText = model->wallet().privateKeysDisabled() ? tr("Create Unsigned") : tr("Send"); + const QString confirmation = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Confirm transaction proposal") : tr("Confirm send coins"); + const QString confirmButtonText = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Create Unsigned") : tr("Sign and send"); SendConfirmationDialog confirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this); confirmationDialog.exec(); QMessageBox::StandardButton retval = static_cast<QMessageBox::StandardButton>(confirmationDialog.result()); @@ -403,9 +412,58 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())}; PartiallySignedTransaction psbtx(mtx); bool complete = false; - const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete, nullptr); + // Always fill without signing first. This prevents an external signer + // from being called prematurely and is not expensive. + TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete, nullptr); assert(!complete); assert(err == TransactionError::OK); + if (model->wallet().hasExternalSigner()) { + try { + err = model->wallet().fillPSBT(SIGHASH_ALL, true /* sign */, true /* bip32derivs */, psbtx, complete, nullptr); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); + send_failure = true; + return; + } + if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { + QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found"); + send_failure = true; + return; + } + if (err == TransactionError::EXTERNAL_SIGNER_FAILED) { + QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure"); + send_failure = true; + return; + } + if (err != TransactionError::OK) { + tfm::format(std::cerr, "Failed to sign PSBT"); + processSendCoinsReturn(WalletModel::TransactionCreationFailed); + send_failure = true; + return; + } + // fillPSBT does not always properly finalize + complete = FinalizeAndExtractPSBT(psbtx, mtx); + } + + // Broadcast transaction if complete (even with an external signer this + // is not always the case, e.g. in a multisig wallet). + if (complete) { + const CTransactionRef tx = MakeTransactionRef(mtx); + m_current_transaction->setWtx(tx); + WalletModel::SendCoinsReturn sendStatus = model->sendCoins(*m_current_transaction); + // process sendStatus and on error generate message shown to user + processSendCoinsReturn(sendStatus); + + if (sendStatus.status == WalletModel::OK) { + Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash()); + } else { + send_failure = true; + } + return; + } + + // Copy PSBT to clipboard and offer to save + assert(!complete); // Serialize the PSBT CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << psbtx; @@ -447,7 +505,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) break; default: assert(false); - } + } // msgBox.exec() } else { // now send the prepared transaction WalletModel::SendCoinsReturn sendStatus = model->sendCoins(*m_current_transaction); @@ -614,7 +672,9 @@ void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances) if(model && model->getOptionsModel()) { CAmount balance = balances.balance; - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().hasExternalSigner()) { + ui->labelBalanceName->setText(tr("External balance:")); + } else if (model->wallet().privateKeysDisabled()) { balance = balances.watch_only_balance; ui->labelBalanceName->setText(tr("Watch-only balance:")); } @@ -698,7 +758,7 @@ void SendCoinsDialog::on_buttonMinimizeFee_clicked() void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry) { // Include watch-only for wallets without private key - m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); // Calculate available amount to send. CAmount amount = model->wallet().getAvailableBalance(*m_coin_control); @@ -753,7 +813,7 @@ void SendCoinsDialog::updateCoinControlState() m_coin_control->m_confirm_target = getConfTargetForIndex(ui->confTargetSelector->currentIndex()); m_coin_control->m_signal_bip125_rbf = ui->optInRBF->isChecked(); // Include watch-only for wallets without private key - m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); } void SendCoinsDialog::updateNumberOfBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool headers, SynchronizationState sync_state) { diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index aa26a01541..7e5790fd87 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -263,6 +263,9 @@ void CreateWalletActivity::createWallet() if (m_create_wallet_dialog->isDescriptorWalletChecked()) { flags |= WALLET_FLAG_DESCRIPTORS; } + if (m_create_wallet_dialog->isExternalSignerChecked()) { + flags |= WALLET_FLAG_EXTERNAL_SIGNER; + } QTimer::singleShot(500, worker(), [this, name, flags] { std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message); @@ -291,6 +294,17 @@ void CreateWalletActivity::finish() void CreateWalletActivity::create() { m_create_wallet_dialog = new CreateWalletDialog(m_parent_widget); + +#ifdef ENABLE_EXTERNAL_SIGNER + std::vector<ExternalSigner> signers; + try { + signers = node().externalSigners(); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Can't list signers"), e.what()); + } + m_create_wallet_dialog->setSigners(signers); +#endif + m_create_wallet_dialog->setWindowModality(Qt::ApplicationModal); m_create_wallet_dialog->show(); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 7c58b8afd2..e32b7c2807 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -552,6 +552,18 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash) return true; } +bool WalletModel::displayAddress(std::string sAddress) +{ + CTxDestination dest = DecodeDestination(sAddress); + bool res = false; + try { + res = m_wallet->displayAddress(dest); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Can't display address"), e.what()); + } + return res; +} + bool WalletModel::isWalletEnabled() { return !gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index b2ce5d69fb..47a21bcfcf 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -136,6 +136,7 @@ public: UnlockContext requestUnlock(); bool bumpFee(uint256 hash, uint256& new_hash); + bool displayAddress(std::string sAddress); static bool isWalletEnabled(); diff --git a/src/qt/walletmodeltransaction.cpp b/src/qt/walletmodeltransaction.cpp index 25172e774c..d185ddb7e8 100644 --- a/src/qt/walletmodeltransaction.cpp +++ b/src/qt/walletmodeltransaction.cpp @@ -26,6 +26,11 @@ CTransactionRef& WalletModelTransaction::getWtx() return wtx; } +void WalletModelTransaction::setWtx(const CTransactionRef& newTx) +{ + wtx = newTx; +} + unsigned int WalletModelTransaction::getTransactionSize() { return wtx ? GetVirtualTransactionSize(*wtx) : 0; diff --git a/src/qt/walletmodeltransaction.h b/src/qt/walletmodeltransaction.h index f9a95362c8..120d240d91 100644 --- a/src/qt/walletmodeltransaction.h +++ b/src/qt/walletmodeltransaction.h @@ -27,6 +27,8 @@ public: QList<SendCoinsRecipient> getRecipients() const; CTransactionRef& getWtx(); + void setWtx(const CTransactionRef&); + unsigned int getTransactionSize(); void setTransactionFee(const CAmount& newFee); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 339d711ac9..414c6637a5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -889,7 +889,7 @@ static RPCHelpMan testmempoolaccept() "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n" "\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other.\n" "\nIf one transaction fails, other transactions may not be fully validated (the 'allowed' key will be blank).\n" - "\nThe maximum number of transactions allowed is 25 (MAX_PACKAGE_COUNT)\n" + "\nThe maximum number of transactions allowed is " + ToString(MAX_PACKAGE_COUNT) + ".\n" "\nThis checks if transactions violate the consensus or policy rules.\n" "\nSee sendrawtransaction call.\n", { @@ -905,7 +905,7 @@ static RPCHelpMan testmempoolaccept() RPCResult{ RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" "Returns results for each transaction in the same order they were passed in.\n" - "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n", + "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n", { {RPCResult::Type::OBJ, "", "", { @@ -939,7 +939,6 @@ static RPCHelpMan testmempoolaccept() UniValue::VARR, UniValueType(), // VNUM or VSTR, checked inside AmountFromValue() }); - const UniValue raw_transactions = request.params[0].get_array(); if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) { throw JSONRPCError(RPC_INVALID_PARAMETER, @@ -951,6 +950,7 @@ static RPCHelpMan testmempoolaccept() CFeeRate(AmountFromValue(request.params[1])); std::vector<CTransactionRef> txns; + txns.reserve(raw_transactions.size()); for (const auto& rawtx : raw_transactions.getValues()) { CMutableTransaction mtx; if (!DecodeHexTx(mtx, rawtx.get_str())) { @@ -971,8 +971,8 @@ static RPCHelpMan testmempoolaccept() }(); UniValue rpc_result(UniValue::VARR); - // We will check transaction fees we iterate through txns in order. If any transaction fee - // exceeds maxfeerate, we will keave the rest of the validation results blank, because it + // We will check transaction fees while we iterate through txns in order. If any transaction fee + // exceeds maxfeerate, we will leave the rest of the validation results blank, because it // doesn't make sense to return a validation result for a transaction if its ancestor(s) would // not be submitted. bool exit_early{false}; diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index c47d0eae1e..e54948a840 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -28,8 +28,8 @@ struct MinerTestingSetup : public TestingSetup { void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs); bool TestSequenceLocks(const CTransaction& tx, int flags) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs) { - CCoinsViewMemPool viewMempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool); - return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), viewMempool, tx, flags); + CCoinsViewMemPool view_mempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool); + return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), view_mempool, tx, flags); } BlockAssembler AssemblerForTest(const CChainParams& params); }; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4413da7ea7..7c73a2d78d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -515,9 +515,9 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) LockPoints lp = it->GetLockPoints(); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); - CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this); + CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this); if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) - || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), viewMempool, tx, flags, &lp, validLP)) { + || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) { // Note if CheckSequenceLocks fails the LockPoints may still be invalid // So it's critical that we remove the tx and not depend on the LockPoints. txToRemove.insert(it); diff --git a/src/txmempool.h b/src/txmempool.h index 46b89049bb..ae4b16d377 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -874,7 +874,8 @@ protected: public: CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn); bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; - /** Add the coins created by this transaction. */ + /** Add the coins created by this transaction. These coins are only temporarily stored in + * m_temp_added and cannot be flushed to the back end. Only used for package validation. */ void PackageAddTransaction(const CTransactionRef& tx); }; diff --git a/src/validation.cpp b/src/validation.cpp index 5e3d429c2e..d5c4be6811 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -472,8 +472,10 @@ public: */ std::vector<COutPoint>& m_coins_to_uncache; const bool m_test_accept; - /** Disable BIP125 RBFing; disallow all conflicts with mempool transactions. */ - const bool disallow_mempool_conflicts; + /** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false, + * any transaction spending the same inputs as a transaction in the mempool is considered + * a conflict. */ + const bool m_allow_bip125_replacement{true}; }; // Single transaction acceptance @@ -482,7 +484,7 @@ public: /** * Multiple transaction acceptance. Transactions may or may not be interdependent, * but must not conflict with each other. Parents must come before children if any - * dependencies exist, otherwise a TX_MISSING_INPUTS error will be returned. + * dependencies exist. */ PackageMempoolAcceptResult AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -619,6 +621,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) { const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout); if (ptxConflicting) { + if (!args.m_allow_bip125_replacement) { + // Transaction conflicts with a mempool tx, but we're not allowing replacements. + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed"); + } if (!setConflicts.count(ptxConflicting->GetHash())) { // Allow opt-out of transaction replacement by setting @@ -645,7 +651,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) break; } } - if (fReplacementOptOut || args.disallow_mempool_conflicts) { + if (fReplacementOptOut) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } @@ -1080,65 +1086,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: { AssertLockHeld(cs_main); + // These context-free package limits can be done before taking the mempool lock. PackageValidationState package_state; - const unsigned int package_count = txns.size(); - - // These context-free package limits can be checked before taking the mempool lock. - if (package_count > MAX_PACKAGE_COUNT) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); - return PackageMempoolAcceptResult(package_state, {}); - } - - const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, - [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); - // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. - if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); - return PackageMempoolAcceptResult(package_state, {}); - } + if (!CheckPackage(txns, package_state)) return PackageMempoolAcceptResult(package_state, {}); - // Construct workspaces and check package policies. std::vector<Workspace> workspaces{}; - workspaces.reserve(package_count); - { - std::unordered_set<uint256, SaltedTxidHasher> later_txids; - std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), - [](const auto& tx) { return tx->GetHash(); }); - // Require the package to be sorted in order of dependency, i.e. parents appear before children. - // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and - // fail on something less ambiguous (missing-inputs could also be an orphan or trying to - // spend nonexistent coins). - for (const auto& tx : txns) { - for (const auto& input : tx->vin) { - if (later_txids.find(input.prevout.hash) != later_txids.end()) { - // The parent is a subsequent transaction in the package. - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); - return PackageMempoolAcceptResult(package_state, {}); - } - } - later_txids.erase(tx->GetHash()); - workspaces.emplace_back(Workspace(tx)); - } - } + workspaces.reserve(txns.size()); + std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces), + [](const auto& tx) { return Workspace(tx); }); std::map<const uint256, const MempoolAcceptResult> results; - { - // Don't allow any conflicting transactions, i.e. spending the same inputs, in a package. - std::unordered_set<COutPoint, SaltedOutpointHasher> inputs_seen; - for (const auto& tx : txns) { - for (const auto& input : tx->vin) { - if (inputs_seen.find(input.prevout) != inputs_seen.end()) { - // This input is also present in another tx in the package. - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); - return PackageMempoolAcceptResult(package_state, {}); - } - } - // Batch-add all the inputs for a tx at a time. If we added them 1 at a time, we could - // catch duplicate inputs within a single tx. This is a more severe, consensus error, - // and we want to report that from CheckTransaction instead. - std::transform(tx->vin.cbegin(), tx->vin.cend(), std::inserter(inputs_seen, inputs_seen.end()), - [](const auto& input) { return input.prevout; }); - } - } LOCK(m_pool.cs); @@ -1151,10 +1107,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: return PackageMempoolAcceptResult(package_state, std::move(results)); } // Make the coins created by this transaction available for subsequent transactions in the - // package to spend. Since we already checked conflicts in the package and RBFs are - // impossible, we don't need to track the coins spent. Note that this logic will need to be - // updated if RBFs in packages are allowed in the future. - assert(args.disallow_mempool_conflicts); + // package to spend. Since we already checked conflicts in the package and we don't allow + // replacements, we don't need to track the coins spent. Note that this logic will need to be + // updated if package replace-by-fee is allowed in the future. + assert(!args.m_allow_bip125_replacement); m_viewmempool.PackageAddTransaction(ws.m_ptx); } @@ -1188,7 +1144,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp { std::vector<COutPoint> coins_to_uncache; MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, - test_accept, /* disallow_mempool_conflicts */ false }; + test_accept, /* m_allow_bip125_replacement */ true }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); @@ -1225,12 +1181,11 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx std::vector<COutPoint> coins_to_uncache; const CChainParams& chainparams = Params(); MemPoolAccept::ATMPArgs args { chainparams, GetTime(), /* bypass_limits */ false, coins_to_uncache, - test_accept, /* disallow_mempool_conflicts */ true }; + test_accept, /* m_allow_bip125_replacement */ false }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); const PackageMempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args); // Uncache coins pertaining to transactions that were not submitted to the mempool. - // Ensure the cache is still within its size limits. for (const COutPoint& hashTx : coins_to_uncache) { active_chainstate.CoinsTip().Uncache(hashTx); } diff --git a/src/validation.h b/src/validation.h index 359a6c779f..0c396c92ee 100644 --- a/src/validation.h +++ b/src/validation.h @@ -234,11 +234,13 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** -* Atomically test acceptance of a package. If the package only contains one tx, package rules still apply. +* Atomically test acceptance of a package. If the package only contains one tx, package rules still +* apply. Package validation does not allow BIP125 replacements, so the transaction(s) cannot spend +* the same inputs as any transaction in the mempool. * @param[in] txns Group of transactions which may be independent or contain -* parent-child dependencies. The transactions must not conflict, i.e. -* must not spend the same inputs, even if it would be a valid BIP125 -* replace-by-fee. Parents must appear before children. +* parent-child dependencies. The transactions must not conflict +* with each other, i.e., must not spend the same inputs. If any +* dependencies exist, parents must appear before children. * @returns a PackageMempoolAcceptResult which includes a MempoolAcceptResult for each transaction. * If a transaction fails, validation will exit early and some results may be missing. */ @@ -269,9 +271,13 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE * Check if transaction will be BIP68 final in the next block to be created on top of tip. * @param[in] tip Chain tip to check tx sequence locks against. For example, * the tip of the current active chain. - * @param[in] coins_view Any CCoinsView that provides access to the relevant coins - * for checking sequence locks. Any CCoinsView can be passed in; - * it is assumed to be consistent with the tip. + * @param[in] coins_view Any CCoinsView that provides access to the relevant coins for + * checking sequence locks. For example, it can be a CCoinsViewCache + * that isn't connected to anything but contains all the relevant + * coins, or a CCoinsViewMemPool that is connected to the + * mempool and chainstate UTXO set. In the latter case, the caller is + * responsible for holding the appropriate locks to ensure that + * calls to GetCoin() return correct coins. * Simulates calling SequenceLocks() with data from the tip passed in. * Optionally stores in LockPoints the resulting height and time calculated and the hash * of the block needed for calculation or skips the calculation and uses the LockPoints diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 5bf037b222..8d5316e0af 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -12,7 +12,7 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir) { - const size_t offset = wallet_dir.string().size() + 1; + const size_t offset = wallet_dir.string().size() + (wallet_dir == wallet_dir.root_name() ? 0 : 1); std::vector<fs::path> paths; boost::system::error_code ec; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index aca52964ee..ee92316b89 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -206,6 +206,11 @@ public: WalletBatch batch{m_wallet->GetDatabase()}; return m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } + bool displayAddress(const CTxDestination& dest) override + { + LOCK(m_wallet->cs_wallet); + return m_wallet->DisplayAddress(dest); + } void lockCoin(const COutPoint& output) override { LOCK(m_wallet->cs_wallet); @@ -446,6 +451,7 @@ public: unsigned int getConfirmTarget() override { return m_wallet->m_confirm_target; } bool hdEnabled() override { return m_wallet->IsHDEnabled(); } bool canGetAddresses() override { return m_wallet->CanGetAddresses(); } + bool hasExternalSigner() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); } bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); } OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; } CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 97fc7acca5..c8ded4c51e 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -352,7 +352,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu return groups_out; } -bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool CWallet::AttemptSelection(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params) const { setCoinsRet.clear(); @@ -456,32 +456,32 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six // confirmations on outputs received from other wallets and only spend confirmed change. - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; + if (AttemptSelection(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; + if (AttemptSelection(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as // possible) if we cannot fund the transaction otherwise. if (m_spend_zero_conf_change) { - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; } - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; } // If partial groups are allowed, relax the requirement of spending OutputGroups (groups // of UTXOs sent to the same address, which are obviously controlled by a single wallet) // in their entirety. - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; } // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs // received from other wallets. if (coin_control.m_include_unsafe_inputs - && SelectCoinsMinConf(value_to_select, + && AttemptSelection(value_to_select, CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; @@ -489,7 +489,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm // Try with unlimited ancestors/descendants. The transaction will still need to meet // mempool ancestor/descendant policy to be accepted to mempool and broadcasted, but // OutputGroups use heuristics that may overestimate ancestor/descendant counts. - if (!fRejectLongChains && SelectCoinsMinConf(value_to_select, + if (!fRejectLongChains && AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(), std::numeric_limits<uint64_t>::max(), true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; @@ -499,7 +499,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm return false; }(); - // SelectCoinsMinConf clears setCoinsRet, so add the preset inputs from coin_control to the coinset + // AttemptSelection clears setCoinsRet, so add the preset inputs from coin_control to the coinset util::insert(setCoinsRet, setPresetCoins); // add preset inputs to the total value selected @@ -578,287 +578,266 @@ bool CWallet::CreateTransactionInternal( FeeCalculation& fee_calc_out, bool sign) { - CAmount nValue = 0; + AssertLockHeld(cs_wallet); + + CMutableTransaction txNew; // The resulting transaction that we make + txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); + + CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy + coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; + + CAmount recipients_sum = 0; const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend); ReserveDestination reservedest(this, change_type); - unsigned int nSubtractFeeFromAmount = 0; - for (const auto& recipient : vecSend) - { - if (nValue < 0 || recipient.nAmount < 0) - { - error = _("Transaction amounts must not be negative"); - return false; + unsigned int outputs_to_subtract_fee_from = 0; // The number of outputs which we are subtracting the fee from + for (const auto& recipient : vecSend) { + recipients_sum += recipient.nAmount; + + if (recipient.fSubtractFeeFromAmount) { + outputs_to_subtract_fee_from++; + coin_selection_params.m_subtract_fee_outputs = true; } - nValue += recipient.nAmount; + } - if (recipient.fSubtractFeeFromAmount) - nSubtractFeeFromAmount++; + // Create change script that will be used if we need change + // TODO: pass in scriptChange instead of reservedest so + // change transaction isn't always pay-to-bitcoin-address + CScript scriptChange; + + // coin control: send change to custom address + if (!std::get_if<CNoDestination>(&coin_control.destChange)) { + scriptChange = GetScriptForDestination(coin_control.destChange); + } else { // no coin control: send change to newly generated address + // Note: We use a new key here to keep it from being obvious which side is the change. + // The drawback is that by not reusing a previous key, the change may be lost if a + // backup is restored, if the backup doesn't have the new private key for the change. + // If we reused the old key, it would be possible to add code to look for and + // rediscover unknown transactions that were written with keys of ours to recover + // post-backup change. + + // Reserve a new key pair from key pool. If it fails, provide a dummy + // destination in case we don't need change. + CTxDestination dest; + if (!reservedest.GetReservedDestination(dest, true)) { + error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); + } + scriptChange = GetScriptForDestination(dest); + // A valid destination implies a change script (and + // vice-versa). An empty change script will abort later, if the + // change keypool ran out, but change is required. + CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); } - if (vecSend.empty()) - { - error = _("Transaction must have at least one recipient"); - return false; + CTxOut change_prototype_txout(0, scriptChange); + coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); + + // Get size of spending the change output + int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); + // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh + // as lower-bound to allow BnB to do it's thing + if (change_spend_size == -1) { + coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE; + } else { + coin_selection_params.change_spend_size = (size_t)change_spend_size; } - CMutableTransaction txNew; + // Set discard feerate + coin_selection_params.m_discard_feerate = GetDiscardRate(*this); + + // Get the fee rate to use effective values in coin selection FeeCalculation feeCalc; - TxSize tx_sizes; - int nBytes; + coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); + // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly + // provided one + if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)); + return false; + } + if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { + // eventually allow a fallback fee + error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); + return false; + } + + // Get long term estimate + CCoinControl cc_temp; + cc_temp.m_confirm_target = chain().estimateMaxBlocks(); + coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + + // Calculate the cost of change + // Cost of change is the cost of creating the change output + cost of spending the change output in the future. + // For creating the change output now, we use the effective feerate. + // For spending the change output in the future, we use the discard feerate for now. + // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) + coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); + coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; + + // vouts to the payees + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size) + } + for (const auto& recipient : vecSend) { - std::set<CInputCoin> setCoins; - LOCK(cs_wallet); - txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); - { - std::vector<COutput> vAvailableCoins; - AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); - CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy - coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; - - // Create change script that will be used if we need change - // TODO: pass in scriptChange instead of reservedest so - // change transaction isn't always pay-to-bitcoin-address - CScript scriptChange; - - // coin control: send change to custom address - if (!std::get_if<CNoDestination>(&coin_control.destChange)) { - scriptChange = GetScriptForDestination(coin_control.destChange); - } else { // no coin control: send change to newly generated address - // Note: We use a new key here to keep it from being obvious which side is the change. - // The drawback is that by not reusing a previous key, the change may be lost if a - // backup is restored, if the backup doesn't have the new private key for the change. - // If we reused the old key, it would be possible to add code to look for and - // rediscover unknown transactions that were written with keys of ours to recover - // post-backup change. - - // Reserve a new key pair from key pool. If it fails, provide a dummy - // destination in case we don't need change. - CTxDestination dest; - if (!reservedest.GetReservedDestination(dest, true)) { - error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); - } - scriptChange = GetScriptForDestination(dest); - // A valid destination implies a change script (and - // vice-versa). An empty change script will abort later, if the - // change keypool ran out, but change is required. - CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); - } - CTxOut change_prototype_txout(0, scriptChange); - coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); - - // Get size of spending the change output - int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); - // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh - // as lower-bound to allow BnB to do it's thing - if (change_spend_size == -1) { - coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE; - } else { - coin_selection_params.change_spend_size = (size_t)change_spend_size; - } + CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - // Set discard feerate - coin_selection_params.m_discard_feerate = GetDiscardRate(*this); + // Include the fee cost for outputs. + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); + } - // Get the fee rate to use effective values in coin selection - coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); - // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly - // provided one - if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { - error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)); - return false; - } - if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { - // eventually allow a fallback fee - error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); - return false; - } + if (IsDust(txout, chain().relayDustFee())) + { + error = _("Transaction amount too small"); + return false; + } + txNew.vout.push_back(txout); + } - // Get long term estimate - CCoinControl cc_temp; - cc_temp.m_confirm_target = chain().estimateMaxBlocks(); - coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + // Include the fees for things that aren't inputs, excluding the change output + const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); + CAmount selection_target = recipients_sum + not_input_fees; - // Calculate the cost of change - // Cost of change is the cost of creating the change output + cost of spending the change output in the future. - // For creating the change output now, we use the effective feerate. - // For spending the change output in the future, we use the discard feerate for now. - // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) - coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); - coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; + // Get available coins + std::vector<COutput> vAvailableCoins; + AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); - coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values + // Choose coins to use + CAmount inputs_sum = 0; + std::set<CInputCoin> setCoins; + if (!SelectCoins(vAvailableCoins, /* nTargetValue */ selection_target, setCoins, inputs_sum, coin_control, coin_selection_params)) + { + error = _("Insufficient funds"); + return false; + } - // vouts to the payees - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size) - } - for (const auto& recipient : vecSend) - { - CTxOut txout(recipient.nAmount, recipient.scriptPubKey); + // Always make a change output + // We will reduce the fee from this change output later, and remove the output if it is too small. + const CAmount change_and_fee = inputs_sum - recipients_sum; + assert(change_and_fee >= 0); + CTxOut newTxOut(change_and_fee, scriptChange); - // Include the fee cost for outputs. - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); - } + if (nChangePosInOut == -1) + { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + { + error = _("Change index out of range"); + return false; + } - if (IsDust(txout, chain().relayDustFee())) - { - error = _("Transaction amount too small"); - return false; - } - txNew.vout.push_back(txout); - } + assert(nChangePosInOut != -1); + auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); - // Include the fees for things that aren't inputs, excluding the change output - const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); - CAmount nValueToSelect = nValue + not_input_fees; + // Shuffle selected coins and fill in final vin + std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end()); + Shuffle(selected_coins.begin(), selected_coins.end(), FastRandomContext()); - // Choose coins to use - CAmount inputs_sum = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params)) - { - error = _("Insufficient funds"); - return false; - } - - // Always make a change output - // We will reduce the fee from this change output later, and remove the output if it is too small. - const CAmount change_and_fee = inputs_sum - nValue; - assert(change_and_fee >= 0); - CTxOut newTxOut(change_and_fee, scriptChange); + // Note how the sequence number is set to non-maxint so that + // the nLockTime set above actually works. + // + // BIP125 defines opt-in RBF as any nSequence < maxint-1, so + // we use the highest possible value in that range (maxint-2) + // to avoid conflicting with other possible uses of nSequence, + // and in the spirit of "smallest possible change from prior + // behavior." + const uint32_t nSequence = coin_control.m_signal_bip125_rbf.value_or(m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : (CTxIn::SEQUENCE_FINAL - 1); + for (const auto& coin : selected_coins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); + } - if (nChangePosInOut == -1) - { - // Insert change txn at random position: - nChangePosInOut = GetRandInt(txNew.vout.size()+1); - } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) - { - error = _("Change index out of range"); - return false; - } + // Calculate the transaction fee + TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + int nBytes = tx_sizes.vsize; + if (nBytes < 0) { + error = _("Signing transaction failed"); + return false; + } + nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); - assert(nChangePosInOut != -1); - auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + // Subtract fee from the change output if not subtracting it from recipient outputs + CAmount fee_needed = nFeeRet; + if (!coin_selection_params.m_subtract_fee_outputs) { + change_position->nValue -= fee_needed; + } - // Dummy fill vin for maximum size estimation - // - for (const auto& coin : setCoins) { - txNew.vin.push_back(CTxIn(coin.outpoint,CScript())); - } + // We want to drop the change to fees if: + // 1. The change output would be dust + // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) + CAmount change_amount = change_position->nValue; + if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) + { + nChangePosInOut = -1; + change_amount = 0; + txNew.vout.erase(change_position); + + // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those + tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + nBytes = tx_sizes.vsize; + fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); + } - // Calculate the transaction fee - tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - nBytes = tx_sizes.vsize; - if (nBytes < 0) { - error = _("Signing transaction failed"); - return false; - } - nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); + // Update nFeeRet in case fee_needed changed due to dropping the change output + if (fee_needed <= change_and_fee - change_amount) { + nFeeRet = change_and_fee - change_amount; + } - // Subtract fee from the change output if not subtrating it from recipient outputs - CAmount fee_needed = nFeeRet; - if (nSubtractFeeFromAmount == 0) { - change_position->nValue -= fee_needed; + // Reduce output values for subtractFeeFromAmount + if (coin_selection_params.m_subtract_fee_outputs) { + CAmount to_reduce = fee_needed + change_amount - change_and_fee; + int i = 0; + bool fFirst = true; + for (const auto& recipient : vecSend) + { + if (i == nChangePosInOut) { + ++i; } + CTxOut& txout = txNew.vout[i]; - // We want to drop the change to fees if: - // 1. The change output would be dust - // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) - CAmount change_amount = change_position->nValue; - if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) + if (recipient.fSubtractFeeFromAmount) { - nChangePosInOut = -1; - change_amount = 0; - txNew.vout.erase(change_position); - - // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those - tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - nBytes = tx_sizes.vsize; - fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - } + txout.nValue -= to_reduce / outputs_to_subtract_fee_from; // Subtract fee equally from each selected recipient - // Update nFeeRet in case fee_needed changed due to dropping the change output - if (fee_needed <= change_and_fee - change_amount) { - nFeeRet = change_and_fee - change_amount; - } - - // Reduce output values for subtractFeeFromAmount - if (nSubtractFeeFromAmount != 0) { - CAmount to_reduce = fee_needed + change_amount - change_and_fee; - int i = 0; - bool fFirst = true; - for (const auto& recipient : vecSend) + if (fFirst) // first receiver pays the remainder not divisible by output count { - if (i == nChangePosInOut) { - ++i; - } - CTxOut& txout = txNew.vout[i]; - - if (recipient.fSubtractFeeFromAmount) - { - txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - - if (fFirst) // first receiver pays the remainder not divisible by output count - { - fFirst = false; - txout.nValue -= to_reduce % nSubtractFeeFromAmount; - } - - // Error if this output is reduced to be below dust - if (IsDust(txout, chain().relayDustFee())) { - if (txout.nValue < 0) { - error = _("The transaction amount is too small to pay the fee"); - } else { - error = _("The transaction amount is too small to send after the fee has been deducted"); - } - return false; - } - } - ++i; + fFirst = false; + txout.nValue -= to_reduce % outputs_to_subtract_fee_from; } - nFeeRet = fee_needed; - } - // Give up if change keypool ran out and change is required - if (scriptChange.empty() && nChangePosInOut != -1) { - return false; + // Error if this output is reduced to be below dust + if (IsDust(txout, chain().relayDustFee())) { + if (txout.nValue < 0) { + error = _("The transaction amount is too small to pay the fee"); + } else { + error = _("The transaction amount is too small to send after the fee has been deducted"); + } + return false; + } } + ++i; } + nFeeRet = fee_needed; + } - // Shuffle selected coins and fill in final vin - txNew.vin.clear(); - std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end()); - Shuffle(selected_coins.begin(), selected_coins.end(), FastRandomContext()); - - // Note how the sequence number is set to non-maxint so that - // the nLockTime set above actually works. - // - // BIP125 defines opt-in RBF as any nSequence < maxint-1, so - // we use the highest possible value in that range (maxint-2) - // to avoid conflicting with other possible uses of nSequence, - // and in the spirit of "smallest possible change from prior - // behavior." - const uint32_t nSequence = coin_control.m_signal_bip125_rbf.value_or(m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : (CTxIn::SEQUENCE_FINAL - 1); - for (const auto& coin : selected_coins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); - } + // Give up if change keypool ran out and change is required + if (scriptChange.empty() && nChangePosInOut != -1) { + return false; + } - if (sign && !SignTransaction(txNew)) { - error = _("Signing transaction failed"); - return false; - } + if (sign && !SignTransaction(txNew)) { + error = _("Signing transaction failed"); + return false; + } - // Return the constructed transaction data. - tx = MakeTransactionRef(std::move(txNew)); + // Return the constructed transaction data. + tx = MakeTransactionRef(std::move(txNew)); - // Limit size - if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || - (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) - { - error = _("Transaction too large"); - return false; - } + // Limit size + if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || + (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) + { + error = _("Transaction too large"); + return false; } if (nFeeRet > m_default_max_tx_fee) { @@ -900,6 +879,18 @@ bool CWallet::CreateTransaction( FeeCalculation& fee_calc_out, bool sign) { + if (vecSend.empty()) { + error = _("Transaction must have at least one recipient"); + return false; + } + + if (std::any_of(vecSend.cbegin(), vecSend.cend(), [](const auto& recipient){ return recipient.nAmount < 0; })) { + error = _("Transaction amounts must not be negative"); + return false; + } + + LOCK(cs_wallet); + int nChangePosIn = nChangePosInOut; Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr) bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 14c3578473..c65ebad52f 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -270,7 +270,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret)); } - // Make sure that effective value is working in SelectCoinsMinConf when BnB is used + // Make sure that effective value is working in AttemptSelection when BnB is used CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), @@ -280,14 +280,14 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) empty_wallet(); add_coin(1); vCoins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); + BOOST_CHECK(!testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); // Test fees subtracted from output: empty_wallet(); add_coin(1 * CENT); vCoins.at(0).nInputBytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; - BOOST_CHECK(testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); + BOOST_CHECK(testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); // Make sure that can use BnB when there are preset inputs @@ -322,24 +322,24 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) empty_wallet(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); add_coin(1*CENT, 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // but we can find a new 1 cent - BOOST_CHECK( testWallet.SelectCoinsMinConf( 1 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 1 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); add_coin(2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 3 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection( 3 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // we can make 3 cents of new coins - BOOST_CHECK( testWallet.SelectCoinsMinConf( 3 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 3 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 3 * CENT); add_coin(5*CENT); // add a mature 5 cent coin, @@ -349,33 +349,33 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection(38 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard_extra, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection(38 * CENT, filter_standard_extra, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // but we can make 37 cents if we accept new coins from ourself - BOOST_CHECK( testWallet.SelectCoinsMinConf(37 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(37 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 37 * CENT); // and we can make 38 cents if we accept all new coins - BOOST_CHECK( testWallet.SelectCoinsMinConf(38 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(38 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - BOOST_CHECK( testWallet.SelectCoinsMinConf(34 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(34 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - BOOST_CHECK( testWallet.SelectCoinsMinConf( 7 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 7 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 7 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - BOOST_CHECK( testWallet.SelectCoinsMinConf( 8 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 8 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK(nValueRet == 8 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - BOOST_CHECK( testWallet.SelectCoinsMinConf( 9 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 9 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 10 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -389,30 +389,30 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - BOOST_CHECK( testWallet.SelectCoinsMinConf(71 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); - BOOST_CHECK(!testWallet.SelectCoinsMinConf(72 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(71 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection(72 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); add_coin( 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); add_coin( 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - BOOST_CHECK( testWallet.SelectCoinsMinConf(11 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(11 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 11 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); @@ -421,11 +421,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin( 2*COIN); add_coin( 3*COIN); add_coin( 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - BOOST_CHECK( testWallet.SelectCoinsMinConf(95 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(95 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); - BOOST_CHECK( testWallet.SelectCoinsMinConf(195 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(195 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -440,14 +440,14 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly - BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // but if we add a bigger coin, small change is avoided add_coin(1111*MIN_CHANGE); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // if we add more small coins: @@ -455,7 +455,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 7 / 10); // and try again to make 1.0 * MIN_CHANGE - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) @@ -464,7 +464,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(50000 * COIN); - BOOST_CHECK( testWallet.SelectCoinsMinConf(500000 * COIN, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(500000 * COIN, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins @@ -477,7 +477,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 6 / 10); add_coin(MIN_CHANGE * 7 / 10); add_coin(1111 * MIN_CHANGE); - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -487,7 +487,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 6 / 10); add_coin(MIN_CHANGE * 8 / 10); add_coin(1111 * MIN_CHANGE); - BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 @@ -498,12 +498,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 100); // trying to make 100.01 from these three coins - BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(MIN_CHANGE * 10001 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(MIN_CHANGE * 9990 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -517,7 +517,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - BOOST_CHECK(testWallet.SelectCoinsMinConf(2000, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(2000, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); if (amt - 2000 < MIN_CHANGE) { // needs more than one input: @@ -602,7 +602,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(1000 * COIN); add_coin(3 * COIN); - BOOST_CHECK(testWallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1003 * COIN); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 788a901f95..d0e26c416c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -326,7 +326,7 @@ private: // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers; - bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign); + bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Catch wallet up to current chain, scanning new blocks, updating the best @@ -445,7 +445,7 @@ public: * param@[out] setCoinsRet Populated with the coins selected if successful. * param@[out] nValueRet Used to return the total value of selected coins. */ - bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, + bool AttemptSelection(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params) const; bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c06b319b0b..24d5351945 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -712,6 +712,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) } } +#ifndef ENABLE_EXTERNAL_SIGNER + if (pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { + pwallet->WalletLogPrintf("Error: External signer wallet being loaded without external signer support compiled\n"); + return DBErrors::TOO_NEW; + } +#endif + // Get cursor if (!m_batch->StartCursor()) { diff --git a/src/zmq/zmqutil.cpp b/src/zmq/zmqutil.cpp index f07a4ae9fd..b0f12388e5 100644 --- a/src/zmq/zmqutil.cpp +++ b/src/zmq/zmqutil.cpp @@ -5,10 +5,12 @@ #include <zmq/zmqutil.h> #include <logging.h> - #include <zmq.h> -void zmqError(const char* str) +#include <cerrno> +#include <string> + +void zmqError(const std::string& str) { - LogPrint(BCLog::ZMQ, "zmq: Error: %s, errno=%s\n", str, zmq_strerror(errno)); + LogPrint(BCLog::ZMQ, "zmq: Error: %s, msg: %s\n", str, zmq_strerror(errno)); } diff --git a/src/zmq/zmqutil.h b/src/zmq/zmqutil.h index 4c1df5d6db..90c0b00edb 100644 --- a/src/zmq/zmqutil.h +++ b/src/zmq/zmqutil.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_ZMQ_ZMQUTIL_H #define BITCOIN_ZMQ_ZMQUTIL_H -void zmqError(const char* str); +#include <string> + +void zmqError(const std::string& str); #endif // BITCOIN_ZMQ_ZMQUTIL_H |