diff --git a/.gitignore b/.gitignore index 57c426256b..3971f68da9 100644 --- a/.gitignore +++ b/.gitignore @@ -91,6 +91,7 @@ libconftest.dylib* # Compilation and Qt preprocessor part *.qm Makefile +!depends/Makefile dash-qt Dash-Qt.app background.tiff* diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index ffd97844ee..653da08f39 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -21,6 +21,8 @@ bench_bench_dash_SOURCES = \ bench/bls_dkg.cpp \ bench/checkblock.cpp \ bench/checkqueue.cpp \ + bench/data.h \ + bench/data.cpp \ bench/duplicate_inputs.cpp \ bench/ecdsa.cpp \ bench/examples.cpp \ @@ -83,7 +85,7 @@ CLEAN_BITCOIN_BENCH = bench/*.gcda bench/*.gcno $(GENERATED_BENCH_FILES) CLEANFILES += $(CLEAN_BITCOIN_BENCH) -bench/checkblock.cpp: bench/data/block813851.raw.h +bench/data.cpp: bench/data/block813851.raw.h bitcoin_bench: $(BENCH_BINARY) @@ -97,7 +99,7 @@ bench/data/%.raw.h: bench/data/%.raw @$(MKDIR_P) $(@D) @{ \ echo "namespace raw_bench{" && \ - echo "static unsigned const char $(*F)[] = {" && \ + echo "static unsigned const char $(*F)_raw[] = {" && \ $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ echo "};};"; \ } > "$@.new" && mv -f "$@.new" "$@" diff --git a/src/bench/checkblock.cpp b/src/bench/checkblock.cpp index f91ec7080b..0916fdd68f 100644 --- a/src/bench/checkblock.cpp +++ b/src/bench/checkblock.cpp @@ -3,39 +3,34 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include #include -#include - // These are the two major time-sinks which happen after we have fully received // a block off the wire, but before we can relay the block on to peers using // compact block relay. static void DeserializeBlockTest(benchmark::Bench& bench) { - CDataStream stream((const char*)raw_bench::block813851, - (const char*)raw_bench::block813851+sizeof(raw_bench::block813851), - SER_NETWORK, PROTOCOL_VERSION); + CDataStream stream(benchmark::data::block813851, SER_NETWORK, PROTOCOL_VERSION); char a = '\0'; stream.write(&a, 1); // Prevent compaction bench.unit("block").run([&] { CBlock block; stream >> block; - bool rewound = stream.Rewind(sizeof(raw_bench::block813851)); + bool rewound = stream.Rewind(benchmark::data::block813851.size()); assert(rewound); }); } static void DeserializeAndCheckBlockTest(benchmark::Bench& bench) { - CDataStream stream((const char*)raw_bench::block813851, - (const char*)raw_bench::block813851+sizeof(raw_bench::block813851), - SER_NETWORK, PROTOCOL_VERSION); + CDataStream stream(benchmark::data::block813851, SER_NETWORK, PROTOCOL_VERSION); char a = '\0'; stream.write(&a, 1); // Prevent compaction @@ -44,7 +39,7 @@ static void DeserializeAndCheckBlockTest(benchmark::Bench& bench) bench.unit("block").run([&] { CBlock block; // Note that CBlock caches its checked state, so we need to recreate it here stream >> block; - bool rewound = stream.Rewind(sizeof(raw_bench::block813851)); + bool rewound = stream.Rewind(benchmark::data::block813851.size()); assert(rewound); CValidationState validationState; diff --git a/src/bench/data.cpp b/src/bench/data.cpp new file mode 100644 index 0000000000..16f1169fd2 --- /dev/null +++ b/src/bench/data.cpp @@ -0,0 +1,14 @@ +// Copyright (c) 2019 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 + +namespace benchmark { +namespace data { + +#include +const std::vector block813851{raw_bench::block813851_raw, raw_bench::block813851_raw + sizeof(raw_bench::block813851_raw) / sizeof(raw_bench::block813851_raw[0])}; + +} // namespace data +} // namespace benchmark diff --git a/src/bench/data.h b/src/bench/data.h new file mode 100644 index 0000000000..bd99a3e53b --- /dev/null +++ b/src/bench/data.h @@ -0,0 +1,19 @@ +// Copyright (c) 2019 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_BENCH_DATA_H +#define BITCOIN_BENCH_DATA_H + +#include +#include + +namespace benchmark { +namespace data { + +extern const std::vector block813851; + +} // namespace data +} // namespace benchmark + +#endif // BITCOIN_BENCH_DATA_H diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index de7bac827b..4660fe1884 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/src/coinjoin/coinjoin.cpp @@ -22,6 +22,8 @@ #include +constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10}; + bool CCoinJoinEntry::AddScriptSig(const CTxIn& txin) { for (auto& txdsin : vecTxDSIn) { @@ -370,7 +372,7 @@ bool CCoinJoin::IsCollateralValid(const CTransaction& txCollateral) { LOCK(cs_main); CValidationState validationState; - if (!AcceptToMemoryPool(mempool, validationState, MakeTransactionRef(txCollateral), nullptr /* pfMissingInputs */, false /* bypass_limits */, maxTxFee /* nAbsurdFee */, true /* fDryRun */)) { + if (!AcceptToMemoryPool(mempool, validationState, MakeTransactionRef(txCollateral), nullptr /* pfMissingInputs */, false /* bypass_limits */, DEFAULT_MAX_RAW_TX_FEE /* nAbsurdFee */, true /* fDryRun */)) { LogPrint(BCLog::COINJOIN, "CCoinJoin::IsCollateralValid -- didn't pass AcceptToMemoryPool()\n"); return false; } diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index 419a048764..40497a2c13 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -23,6 +23,7 @@ #include CCoinJoinServer coinJoinServer; +constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10}; void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, CConnman& connman, bool enable_bip61) { @@ -341,7 +342,7 @@ void CCoinJoinServer::CommitFinalTransaction(CConnman& connman) TRY_LOCK(cs_main, lockMain); CValidationState validationState; mempool.PrioritiseTransaction(hashTx, 0.1 * COIN); - if (!lockMain || !AcceptToMemoryPool(mempool, validationState, finalTransaction, nullptr /* pfMissingInputs */, false /* bypass_limits */, maxTxFee /* nAbsurdFee */)) { + if (!lockMain || !AcceptToMemoryPool(mempool, validationState, finalTransaction, nullptr /* pfMissingInputs */, false /* bypass_limits */, DEFAULT_MAX_RAW_TX_FEE /* nAbsurdFee */)) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- AcceptToMemoryPool() error: Transaction not valid\n"); SetNull(); // not much we can do in this case, just notify clients diff --git a/src/coinjoin/util.cpp b/src/coinjoin/util.cpp index 507019c84d..7ab33f5768 100644 --- a/src/coinjoin/util.cpp +++ b/src/coinjoin/util.cpp @@ -236,8 +236,8 @@ CAmount CTransactionBuilder::GetFee(unsigned int nBytes) const if (nRequiredFee > nFeeCalc) { nFeeCalc = nRequiredFee; } - if (nFeeCalc > ::maxTxFee) { - nFeeCalc = ::maxTxFee; + if (nFeeCalc > pwallet->m_default_max_tx_fee) { + nFeeCalc = pwallet->m_default_max_tx_fee; } return nFeeCalc; } diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index 3e52120005..0aaea5d596 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -37,6 +37,7 @@ void DummyWalletInit::AddWalletOptions() const "-disablewallet", "-instantsendnotify=", "-keypool=", + "-maxtxfee=", "-rescan=", "-salvagewallet", "-spendzeroconfchange", diff --git a/src/init.cpp b/src/init.cpp index c9bc43907d..8fbad41b5f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -685,8 +685,6 @@ void SetupServerArgs() gArgs.AddArg("-maxsigcachesize=", strprintf("Limit sum of signature cache and script execution cache sizes to MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); gArgs.AddArg("-maxtipage=", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); gArgs.AddArg("-mocktime=", "Replace actual time with seconds since epoch (default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - gArgs.AddArg("-maxtxfee=", strprintf("Maximum total fees (in %s) to use in a single wallet transaction or raw transaction; setting this too low may abort large transactions (default: %s)", - CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); gArgs.AddArg("-minsporkkeys=", "Overrides minimum spork signers to change spork value. Only useful for regtest and devnet. Using this on mainnet or testnet will ban you.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); gArgs.AddArg("-printpriority", strprintf("Log transaction fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); gArgs.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); @@ -1496,22 +1494,6 @@ bool AppInitParameterInteraction() dustRelayFee = CFeeRate(n); } - // This is required by both the wallet and node - if (gArgs.IsArgSet("-maxtxfee")) - { - CAmount nMaxFee = 0; - if (!ParseMoney(gArgs.GetArg("-maxtxfee", ""), nMaxFee)) - return InitError(AmountErrMsg("maxtxfee", gArgs.GetArg("-maxtxfee", ""))); - if (nMaxFee > HIGH_MAX_TX_FEE) - InitWarning(_("-maxtxfee is set very high! Fees this large could be paid on a single transaction.")); - maxTxFee = nMaxFee; - if (CFeeRate(maxTxFee, 1000) < ::minRelayTxFee) - { - return InitError(strprintf(_("Invalid amount for -maxtxfee=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), - gArgs.GetArg("-maxtxfee", ""), ::minRelayTxFee.ToString())); - } - } - fRequireStandard = !gArgs.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard()); if (chainparams.RequireStandard() && !fRequireStandard) return InitError(strprintf("acceptnonstdtxn is not currently supported for %s chain", chainparams.NetworkIDString())); diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 056ee0d3ba..df8f6c7fb7 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -37,7 +38,7 @@ namespace interfaces { namespace { -class LockImpl : public Chain::Lock +class LockImpl : public Chain::Lock, public UniqueLock { Optional getHeight() override { @@ -170,16 +171,7 @@ class LockImpl : public Chain::Lock LockAnnotation lock(::cs_main); return CheckFinalTx(tx); } - bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override - { - LockAnnotation lock(::cs_main); - return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, - false /* bypass limits */, absurd_fee); - } -}; -class LockingStateImpl : public LockImpl, public UniqueLock -{ using UniqueLock::UniqueLock; }; @@ -217,17 +209,11 @@ public: { m_notifications->BlockDisconnected(*block); } - void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->ChainStateFlushed(locator); } - void ResendWalletTransactions(int64_t best_block_time, CConnman*) override + void UpdatedBlockTip(const CBlockIndex* index, const CBlockIndex* fork_index, bool is_ibd) override { - // `cs_main` is always held when this method is called, so it is safe to - // call `assumeLocked`. This is awkward, and the `assumeLocked` method - // should be able to be removed entirely if `ResendWalletTransactions` - // is replaced by a wallet timer as suggested in - // https://github.com/bitcoin/bitcoin/issues/15619 - auto locked_chain = m_chain.assumeLocked(); - m_notifications->ResendWalletTransactions(*locked_chain, best_block_time); + m_notifications->UpdatedBlockTip(); } + void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->ChainStateFlushed(locator); } void NotifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) override { m_notifications->NotifyChainLock(pindexChainLock, clsig); @@ -284,13 +270,12 @@ class ChainImpl : public Chain public: std::unique_ptr lock(bool try_lock) override { - auto result = MakeUnique(::cs_main, "cs_main", __FILE__, __LINE__, try_lock); + auto result = MakeUnique(::cs_main, "cs_main", __FILE__, __LINE__, try_lock); if (try_lock && result && !*result) return {}; // std::move necessary on some compilers due to conversion from - // LockingStateImpl to Lock pointer + // LockImpl to Lock pointer return std::move(result); } - std::unique_ptr assumeLocked() override { return MakeUnique(); } bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override { CBlockIndex* index; @@ -324,10 +309,13 @@ public: auto it = ::mempool.GetIter(txid); return it && (*it)->GetCountWithDescendants() > 1; } - void relayTransaction(const uint256& txid) override + bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override { - CInv inv(CCoinJoin::GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid); - g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); }); + const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false); + // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures. + // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures + // that Chain clients do not need to know about. + return TransactionError::OK == err; } void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) override { @@ -362,9 +350,9 @@ public: CFeeRate relayMinFee() override { return ::minRelayTxFee; } CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; } CFeeRate relayDustFee() override { return ::dustRelayFee; } - CAmount maxTxFee() override { return ::maxTxFee; } bool getPruneMode() override { return ::fPruneMode; } bool p2pEnabled() override { return g_connman != nullptr; } + bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !::ChainstateActive().IsInitialBlockDownload(); } bool isInitialBlockDownload() override { return ::ChainstateActive().IsInitialBlockDownload(); } bool shutdownRequested() override { return ShutdownRequested(); } int64_t getAdjustedTime() override { return GetAdjustedTime(); } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index aa4fe8afd9..10f1b3064d 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -52,10 +52,6 @@ class Handler; //! asynchronously //! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269). //! -//! * The relayTransactions() and submitToMemoryPool() methods could be replaced -//! with a higher-level broadcastTransaction method -//! (https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984). -//! //! * The initMessages() and loadWallet() methods which the wallet uses to send //! notifications to the GUI should go away when GUI and wallet can directly //! communicate with each other without going through the node @@ -145,22 +141,12 @@ public: //! Check if transaction will be final given chain height current time. virtual bool checkFinalTx(const CTransaction& tx) = 0; - - //! Add transaction to memory pool if the transaction fee is below the - //! amount specified by absurd_fee. Returns false if the transaction - //! could not be added due to the fee or for another reason. - virtual bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) = 0; }; //! Return Lock interface. Chain is locked when this is called, and //! unlocked when the returned interface is freed. virtual std::unique_ptr lock(bool try_lock = false) = 0; - //! Return Lock interface assuming chain is already locked. This - //! method is temporary and is only used in a few places to avoid changing - //! behavior while code is transitioned to use the Chain::Lock interface. - virtual std::unique_ptr assumeLocked() = 0; - //! Return whether node has the block and optionally return block metadata //! or contents. //! @@ -184,8 +170,10 @@ public: //! Check if transaction has descendants in mempool. virtual bool hasDescendantsInMempool(const uint256& txid) = 0; - //! Relay transaction. - virtual void relayTransaction(const uint256& txid) = 0; + //! Transaction is added to memory pool, if the transaction fee is below the + //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true. + //! Return false if the transaction could not be added due to the fee or for another reason. + virtual bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) = 0; //! Calculate mempool ancestor and descendant counts for the given transaction. virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) = 0; @@ -211,18 +199,15 @@ public: //! Relay dust fee setting (-dustrelayfee), reflecting lowest rate it's economical to spend. virtual CFeeRate relayDustFee() = 0; - //! Node max tx fee setting (-maxtxfee). - //! This could be replaced by a per-wallet max fee, as proposed at - //! https://github.com/bitcoin/bitcoin/issues/15355 - //! But for the time being, wallets call this to access the node setting. - virtual CAmount maxTxFee() = 0; - //! Check if pruning is enabled. virtual bool getPruneMode() = 0; //! Check if p2p enabled. virtual bool p2pEnabled() = 0; + //! Check if the node is ready to broadcast transactions. + virtual bool isReadyToBroadcast() = 0; + //! Check if in IBD. virtual bool isInitialBlockDownload() = 0; @@ -256,8 +241,8 @@ public: virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason) {} virtual void BlockConnected(const CBlock& block, const std::vector& tx_conflicted) {} virtual void BlockDisconnected(const CBlock& block) {} + virtual void UpdatedBlockTip() {} virtual void ChainStateFlushed(const CBlockLocator& locator) {} - virtual void ResendWalletTransactions(Lock& locked_chain, int64_t best_block_time) {} virtual void NotifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) {} virtual void NotifyTransactionLock(const CTransactionRef &tx, const std::shared_ptr& islock) {} }; diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp index 5ae996d778..787cdd35f7 100644 --- a/src/interfaces/node.cpp +++ b/src/interfaces/node.cpp @@ -344,7 +344,6 @@ public: } } bool getNetworkActive() override { return g_connman && g_connman->GetNetworkActive(); } - CAmount getMaxTxFee() override { return ::maxTxFee; } CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override { FeeCalculation fee_calc; diff --git a/src/interfaces/node.h b/src/interfaces/node.h index ec6a41f068..9c892708d2 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -229,9 +229,6 @@ public: //! Get network active. virtual bool getNetworkActive() = 0; - //! Get max tx fee. - virtual CAmount getMaxTxFee() = 0; - //! Estimate smart fee. virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) = 0; diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index bb49f9bf73..9972711d44 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -42,32 +42,6 @@ namespace interfaces { namespace { -class PendingWalletTxImpl : public PendingWalletTx -{ -public: - explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet), m_key(&wallet) {} - - const CTransaction& get() override { return *m_tx; } - - bool commit(WalletValueMap value_map, - WalletOrderForm order_form, - std::string& reject_reason) override - { - auto locked_chain = m_wallet.chain().lock(); - LOCK2(mempool.cs, m_wallet.cs_wallet); - CValidationState state; - if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_key, state)) { - reject_reason = state.GetRejectReason(); - return false; - } - return true; - } - - CTransactionRef m_tx; - CWallet& m_wallet; - CReserveKey m_key; -}; - //! Construct wallet tx struct. WalletTx MakeWalletTx(interfaces::Chain::Lock& locked_chain, CWallet& wallet, const CWalletTx& wtx) { @@ -308,7 +282,7 @@ public: LOCK2(cs_main, m_wallet->cs_wallet); return m_wallet->ListProTxCoins(outputs); } - std::unique_ptr createTransaction(const std::vector& recipients, + CTransactionRef createTransaction(const std::vector& recipients, const CCoinControl& coin_control, bool sign, int& change_pos, @@ -317,12 +291,28 @@ public: { auto locked_chain = m_wallet->chain().lock(); LOCK2(mempool.cs, m_wallet->cs_wallet); - auto pending = MakeUnique(*m_wallet); - if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_key, fee, change_pos, + CReserveKey m_key(m_wallet.get()); + CTransactionRef tx; + if (!m_wallet->CreateTransaction(*locked_chain, recipients, tx, m_key, fee, change_pos, fail_reason, coin_control, sign)) { return {}; } - return std::move(pending); + return tx; + } + bool commitTransaction(CTransactionRef tx, + WalletValueMap value_map, + WalletOrderForm order_form, + std::string& reject_reason) override + { + auto locked_chain = m_wallet->chain().lock(); + LOCK2(mempool.cs, m_wallet->cs_wallet); + CReserveKey m_key(m_wallet.get()); + CValidationState state; + if (!m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form), m_key, state)) { + reject_reason = state.GetRejectReason(); + return false; + } + return true; } bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); } bool abandonTransaction(const uint256& txid) override @@ -544,6 +534,7 @@ public: bool hdEnabled() override { return m_wallet->IsHDEnabled(); } bool IsWalletFlagSet(uint64_t flag) override { return m_wallet->IsWalletFlagSet(flag); } CoinJoin::Client& coinJoin() override { return m_coinjoin; } + CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; } void remove() override { RemoveWallet(m_wallet); diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 6eff7991c1..da57c10b3b 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -32,7 +32,6 @@ struct CRecipient; namespace interfaces { class Handler; -class PendingWalletTx; struct WalletAddress; struct WalletBalances; struct WalletTx; @@ -159,13 +158,19 @@ public: virtual void listProTxCoins(std::vector& vOutpts) = 0; //! Create transaction. - virtual std::unique_ptr createTransaction(const std::vector& recipients, + virtual CTransactionRef createTransaction(const std::vector& recipients, const CCoinControl& coin_control, bool sign, int& change_pos, CAmount& fee, std::string& fail_reason) = 0; + //! Commit transaction. + virtual bool commitTransaction(CTransactionRef tx, + WalletValueMap value_map, + WalletOrderForm order_form, + std::string& reject_reason) = 0; + //! Return whether transaction can be abandoned. virtual bool transactionCanBeAbandoned(const uint256& txid) = 0; @@ -266,6 +271,9 @@ public: virtual CoinJoin::Client& coinJoin() = 0; + //! Get max tx fee. + virtual CAmount getDefaultMaxTxFee() = 0; + // Remove wallet. virtual void remove() = 0; @@ -310,21 +318,6 @@ public: virtual std::unique_ptr handleCanGetAddressesChanged(CanGetAddressesChangedFn fn) = 0; }; -//! Tracking object returned by CreateTransaction and passed to CommitTransaction. -class PendingWalletTx -{ -public: - virtual ~PendingWalletTx() {} - - //! Get transaction data. - virtual const CTransaction& get() = 0; - - //! Send pending transaction and commit to wallet. - virtual bool commit(WalletValueMap value_map, - WalletOrderForm order_form, - std::string& reject_reason) = 0; -}; - //! Information about one wallet address. struct WalletAddress { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e0d03c21c0..fe89b16c23 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -237,8 +237,6 @@ namespace { /** Expiration-time ordered list of (expire time, relay map entry) pairs. */ std::deque> vRelayExpiration GUARDED_BY(cs_main); - std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block - struct IteratorComparator { template @@ -1314,8 +1312,6 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB }); connman->WakeMessageHandler(); } - - nTimeBestReceived = GetTime(); } /** @@ -4381,21 +4377,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto) } } - // Resend wallet transactions that haven't gotten in a block yet - // Except during reindex, importing and IBD, when old wallet - // transactions become unconfirmed and spams other nodes. - if (!fReindex && !fImporting && !::ChainstateActive().IsInitialBlockDownload()) - { - static int64_t nLastBroadcastTime = 0; - // HACK: Call this only once every few seconds. SendMessages is called once per peer, which makes this signal very expensive - // The proper solution would be to move this out of here, but this is not worth the effort right now as bitcoin#15632 will later do this. - // Luckily, the Broadcast signal is not used for anything else then CWallet::ResendWalletTransactionsBefore. - if (nNow - nLastBroadcastTime >= 5000000) { - GetMainSignals().Broadcast(nTimeBestReceived, connman); - nLastBroadcastTime = nNow; - } - } - // // Try sending block announcements via headers // diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index da8b5ec53a..b1be5eda77 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -13,26 +13,30 @@ #include -TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee, const bool bypass_limits) +TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits) { + assert(g_connman); std::promise promise; - hashTx = tx->GetHash(); + uint256 hashTx = tx->GetHash(); + bool callback_set = false; { // cs_main scope LOCK(cs_main); + // If the transaction is already confirmed in the chain, don't do anything + // and return early. CCoinsViewCache &view = ::ChainstateActive().CoinsTip(); - bool fHaveChain = false; - for (size_t o = 0; !fHaveChain && o < tx->vout.size(); o++) { + for (size_t o = 0; o < tx->vout.size(); o++) { const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o)); - fHaveChain = !existingCoin.IsSpent(); + // IsSpent doesnt mean the coin is spent, it means the output doesnt' exist. + // So if the output does exist, then this transaction exists in the chain. + if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; } - bool fHaveMempool = mempool.exists(hashTx); - if (!fHaveMempool && !fHaveChain) { - // push to local node and sync with wallets + if (!mempool.exists(hashTx)) { + // Transaction is not already in the mempool. Submit it. CValidationState state; bool fMissingInputs; if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs, - bypass_limits, highfee)) { + bypass_limits, max_tx_fee)) { if (state.IsInvalid()) { err_string = FormatStateMessage(state); return TransactionError::MEMPOOL_REJECTED; @@ -43,33 +47,37 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, err_string = FormatStateMessage(state); return TransactionError::MEMPOOL_ERROR; } - } else { - // If wallet is enabled, ensure that the wallet has been made aware - // of the new transaction prior to returning. This prevents a race - // where a user might call sendrawtransaction with a transaction - // to/from their wallet, immediately call some wallet RPC, and get - // a stale result because callbacks have not yet been processed. + } + + // Transaction was accepted to the mempool. + + if (wait_callback) { + // For transactions broadcast from outside the wallet, make sure + // that the wallet has been notified of the transaction before + // continuing. + // + // This prevents a race where a user might call sendrawtransaction + // with a transaction to/from their wallet, immediately call some + // wallet RPC, and get a stale result because callbacks have not + // yet been processed. CallFunctionInValidationInterfaceQueue([&promise] { promise.set_value(); }); + callback_set = true; } - } else if (fHaveChain) { - return TransactionError::ALREADY_IN_CHAIN; - } else { - // Make sure we don't block forever if re-sending - // a transaction already in mempool. - promise.set_value(); } } // cs_main - promise.get_future().wait(); - - if (!g_connman) { - return TransactionError::P2P_DISABLED; + if (callback_set) { + // Wait until Validation Interface clients have been notified of the + // transaction entering the mempool. + promise.get_future().wait(); } - g_connman->RelayTransaction(*tx); + if (relay) { + g_connman->RelayTransaction(*tx); + } return TransactionError::OK; } diff --git a/src/node/transaction.h b/src/node/transaction.h index 7404334001..de6699197f 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -11,14 +11,21 @@ #include /** - * Broadcast a transaction + * Submit a transaction to the mempool and (optionally) relay it to all P2P peers. + * + * Mempool submission can be synchronous (will await mempool entry notification + * over the CValidationInterface) or asynchronous (will submit and not wait for + * notification), depending on the value of wait_callback. wait_callback MUST + * NOT be set while cs_main, cs_mempool or cs_wallet are held to avoid + * deadlock. * * @param[in] tx the transaction to broadcast - * @param[out] &txid the txid of the transaction, if successfully broadcast * @param[out] &err_string reference to std::string to fill with error string if available - * @param[in] highfee Reject txs with fees higher than this (if 0, accept any fee) + * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee) + * @param[in] relay flag if both mempool insertion and p2p relay are requested + * @param[in] wait_callback, wait until callbacks have been processed to avoid stale result due to a sequentially RPC. * return error */ -[[nodiscard]] TransactionError BroadcastTransaction(CTransactionRef tx, uint256& txid, std::string& err_string, const CAmount& highfee, const bool bypass_limits = false); +[[nodiscard]] TransactionError BroadcastTransaction(CTransactionRef tx, std::string& err_string, const CAmount& highfee, bool relay, bool wait_callback, bool bypass_limits = false); #endif // BITCOIN_NODE_TRANSACTION_H diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 7dc2658b9c..cca328eaad 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -411,7 +411,7 @@ void SendCoinsDialog::send(QList recipients) if (m_coin_control->IsUsingCoinJoin()) { // append number of inputs questionString.append("
"); - int nInputs = currentTransaction.getWtx()->get().vin.size(); + int nInputs = currentTransaction.getWtx()->vin.size(); questionString.append(tr("This transaction will consume %n input(s)", "", nInputs)); // warn about potential privacy issues when spending too many inputs at once @@ -464,7 +464,7 @@ void SendCoinsDialog::send(QList recipients) accept(); m_coin_control->UnSelectAll(); coinControlUpdateLabels(); - Q_EMIT coinsSent(currentTransaction.getWtx()->get().GetHash()); + Q_EMIT coinsSent(currentTransaction.getWtx()->GetHash()); } fNewRecipientAllowed = true; } @@ -666,7 +666,7 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn msgParams.second = CClientUIInterface::MSG_ERROR; break; case WalletModel::AbsurdFee: - msgParams.first = tr("A fee higher than %1 is considered an absurdly high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), model->node().getMaxTxFee())); + msgParams.first = tr("A fee higher than %1 is considered an absurdly high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), model->wallet().getDefaultMaxTxFee())); break; case WalletModel::PaymentRequestExpired: msgParams.first = tr("Payment request expired."); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 20094194e4..364b87918f 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -270,9 +270,9 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact } // reject absurdly high fee. (This can never happen because the - // wallet caps the fee at maxTxFee. This merely serves as a + // wallet caps the fee at m_default_max_tx_fee. This merely serves as a // belt-and-suspenders check) - if (nFeeRequired > m_node.getMaxTxFee()) + if (nFeeRequired > m_wallet->getDefaultMaxTxFee()) return AbsurdFee; return SendCoinsReturn(OK); @@ -312,11 +312,11 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran auto& newTx = transaction.getWtx(); std::string rejectReason; - if (!newTx->commit(std::move(mapValue), std::move(vOrderForm), rejectReason)) + if (!wallet().commitTransaction(newTx, std::move(mapValue), std::move(vOrderForm), rejectReason)) return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(rejectReason)); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); - ssTx << newTx->get(); + ssTx << *newTx; transaction_array.append(ssTx.data(), ssTx.size()); } diff --git a/src/qt/walletmodeltransaction.cpp b/src/qt/walletmodeltransaction.cpp index 9e31cb4c2a..8bc3cc2463 100644 --- a/src/qt/walletmodeltransaction.cpp +++ b/src/qt/walletmodeltransaction.cpp @@ -22,14 +22,14 @@ QList WalletModelTransaction::getRecipients() const return recipients; } -std::unique_ptr& WalletModelTransaction::getWtx() +CTransactionRef& WalletModelTransaction::getWtx() { return wtx; } unsigned int WalletModelTransaction::getTransactionSize() { - return wtx ? ::GetSerializeSize(wtx->get(), SER_NETWORK, PROTOCOL_VERSION) : 0; + return wtx != nullptr ? ::GetSerializeSize(*wtx, SER_NETWORK, PROTOCOL_VERSION) : 0; } CAmount WalletModelTransaction::getTransactionFee() const @@ -60,7 +60,7 @@ void WalletModelTransaction::reassignAmounts() if (out.amount() <= 0) continue; const unsigned char* scriptStr = (const unsigned char*)out.script().data(); CScript scriptPubKey(scriptStr, scriptStr+out.script().size()); - for (const auto& txout : wtx->get().vout) { + for (const auto& txout : wtx.get()->vout) { if (txout.scriptPubKey == scriptPubKey) { subtotal += txout.nValue; break; @@ -72,7 +72,7 @@ void WalletModelTransaction::reassignAmounts() else // normal recipient (no payment request) #endif { - for (const auto& txout : wtx->get().vout) { + for (const auto& txout : wtx.get()->vout) { CScript scriptPubKey = GetScriptForDestination(DecodeDestination(rcp.address.toStdString())); if (txout.scriptPubKey == scriptPubKey) { rcp.amount = txout.nValue; diff --git a/src/qt/walletmodeltransaction.h b/src/qt/walletmodeltransaction.h index 2f01051cc3..6f3366cd08 100644 --- a/src/qt/walletmodeltransaction.h +++ b/src/qt/walletmodeltransaction.h @@ -16,7 +16,6 @@ class SendCoinsRecipient; namespace interfaces { class Node; -class PendingWalletTx; } /** Data model for a walletmodel transaction. */ @@ -27,7 +26,7 @@ public: QList getRecipients() const; - std::unique_ptr& getWtx(); + CTransactionRef& getWtx(); unsigned int getTransactionSize(); void setTransactionFee(const CAmount& newFee); @@ -39,7 +38,7 @@ public: private: QList recipients; - std::unique_ptr wtx; + CTransactionRef wtx; CAmount fee; }; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 3185917107..5fc8ded07b 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2010 Satoshi Nakamoto -// Copyright (c) 2009-2015 The Bitcoin Core developers +// Copyright (c) 2009-2019 The Bitcoin Core developers // Copyright (c) 2014-2021 The Dash Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -2026,9 +2026,7 @@ static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t) static UniValue getblockstats(const JSONRPCRequest& request) { - if (request.fHelp || request.params.size() < 1 || request.params.size() > 4) { - throw std::runtime_error( - RPCHelpMan{"getblockstats", + const RPCHelpMan help{"getblockstats", "\nCompute per block statistics for a given window. All amounts are in duffs.\n" "It won't work for some heights with pruning.\n" "It won't work without -txindex for utxo_size_inc, *fee or *feerate stats.\n", @@ -2080,7 +2078,9 @@ static UniValue getblockstats(const JSONRPCRequest& request) HelpExampleCli("getblockstats", "1000 '[\"minfeerate\",\"avgfeerate\"]'") + HelpExampleRpc("getblockstats", "1000 '[\"minfeerate\",\"avgfeerate\"]'") }, - }.ToString()); + }; + if (request.fHelp || !help.IsValidNumArgs(request.params.size())) { + throw std::runtime_error(help.ToString()); } if (g_txindex) { diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index af5bea8bee..31e704891c 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -111,10 +111,12 @@ static const CRPCConvertParam vRPCConvertParams[] = { "signrawtransactionwithkey", 2, "prevtxs" }, { "signrawtransactionwithwallet", 1, "prevtxs" }, { "sendrawtransaction", 1, "allowhighfees" }, + { "sendrawtransaction", 1, "maxfeerate" }, { "sendrawtransaction", 2, "instantsend" }, { "sendrawtransaction", 3, "bypasslimits" }, { "testmempoolaccept", 0, "rawtxs" }, { "testmempoolaccept", 1, "allowhighfees" }, + { "testmempoolaccept", 1, "maxfeerate" }, { "combinerawtransaction", 0, "txs" }, { "fundrawtransaction", 1, "options" }, { "walletcreatefundedpsbt", 0, "inputs" }, diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index e31be9bb10..6c81036466 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -584,13 +584,7 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request) static UniValue setban(const JSONRPCRequest& request) { - std::string strCommand; - if (!request.params[1].isNull()) - strCommand = request.params[1].get_str(); - if (request.fHelp || request.params.size() < 2 || - (strCommand != "add" && strCommand != "remove")) - throw std::runtime_error( - RPCHelpMan{"setban", + const RPCHelpMan help{"setban", "\nAttempts to add or remove an IP/Subnet from the banned list.\n", { {"subnet", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP/Subnet (see getpeerinfo for nodes IP) with an optional netmask (default is /32 = single IP)"}, @@ -604,7 +598,13 @@ static UniValue setban(const JSONRPCRequest& request) + HelpExampleCli("setban", "\"192.168.0.0/24\" \"add\"") + HelpExampleRpc("setban", "\"192.168.0.6\", \"add\", 86400") }, - }.ToString()); + }; + std::string strCommand; + if (!request.params[1].isNull()) + strCommand = request.params[1].get_str(); + if (request.fHelp || !help.IsValidNumArgs(request.params.size()) || (strCommand != "add" && strCommand != "remove")) { + throw std::runtime_error(help.ToString()); + } if (!g_banman) { throw JSONRPCError(RPC_DATABASE_ERROR, "Error: Ban database not loaded"); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 55ab4845d5..cc29103c70 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -30,6 +30,7 @@ #include