LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #15713: refactor: Replace chain relayTransactions/submitMemoryPool by higher method

Browse source 
fb62f128bb Tidy up BroadcastTransaction() (John Newbery)
b8eecf8e79 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f5652b Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c198 Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19b4b Add BroadcastTransaction utility usage in Chain interface (Antoine Riard)

Pull request description:

  Remove CWalletTx::AcceptToMemoryPool

  Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay

  Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.

  Obviously, working on implementing https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984 to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly

ACKs for top commit:
  MarcoFalke:
    re-ACK fb62f128bb
  Sjors:
    re-ACK fb62f128bb

Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f
This commit is contained in:
MarcoFalke 2019-08-02 09:12:55 -04:00
commit be0e8b4bff
No known key found for this signature in database
GPG key ID: D2EA4850E7528B25
7 changed files with 87 additions and 97 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
src/interfaces/chain.cpp
View file

@ -11,6 +11,7 @@
#include <net.h>
#include <net_processing.h>
#include <node/coin.h>
#include <node/transaction.h>
#include <policy/fees.h>
#include <policy/policy.h>
#include <policy/rbf.h>
@ -150,12 +151,6 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
LockAssertion lock(::cs_main);
return CheckFinalTx(tx);
}
bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override
{
LockAssertion lock(::cs_main);
return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */,
false /* bypass limits */, absurd_fee);
}
using UniqueLock::UniqueLock;
};
@ -291,9 +286,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
{
RelayTransaction(txid, *g_connman);
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
{

15
src/interfaces/chain.h
View file

@ -43,10 +43,6 @@ class Wallet;
//! 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
@ -127,11 +123,6 @@ 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
@ -164,8 +155,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;

60
src/node/transaction.cpp
View file

@ -14,26 +14,30 @@
#include <future>
TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee)
TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback)
{
assert(g_connman);
std::promise<void> 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 = *pcoinsTip;
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,
nullptr /* plTxnReplaced */, false /* bypass_limits */, highfee)) {
nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) {
if (state.IsInvalid()) {
err_string = FormatStateMessage(state);
return TransactionError::MEMPOOL_REJECTED;
@ -44,33 +48,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();
}
RelayTransaction(hashTx, *g_connman);
if (relay) {
RelayTransaction(hashTx, *g_connman);
}
return TransactionError::OK;
}

15
src/node/transaction.h
View file

@ -11,14 +11,21 @@
#include <util/error.h>
/**
* 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);
NODISCARD TransactionError BroadcastTransaction(CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback);
#endif // BITCOIN_NODE_TRANSACTION_H

6
src/rpc/rawtransaction.cpp
View file

@ -814,14 +814,14 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
max_raw_tx_fee = fr.GetFee((weight+3)/4);
}
uint256 txid;
std::string err_string;
const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_raw_tx_fee);
AssertLockNotHeld(cs_main);
const TransactionError err = BroadcastTransaction(tx, err_string, max_raw_tx_fee, /*relay*/ true, /*wait_callback*/ true);
if (TransactionError::OK != err) {
throw JSONRPCTransactionError(err, err_string);
}
return txid.GetHex();
return tx->GetHash().GetHex();
}
static UniValue testmempoolaccept(const JSONRPCRequest& request)

66
src/wallet/wallet.cpp
View file

@ -2134,8 +2134,7 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
std::map<int64_t, CWalletTx*> mapSorted;
// Sort pending wallet transactions based on their initial wallet insertion order
for (std::pair<const uint256, CWalletTx>& item : mapWallet)
{
for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
const uint256& wtxid = item.first;
CWalletTx& wtx = item.second;
assert(wtx.GetHash() == wtxid);
@ -2150,32 +2149,32 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
// Try to add wallet transactions to memory pool
for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
CWalletTx& wtx = *(item.second);
CValidationState state;
wtx.AcceptToMemoryPool(locked_chain, state);
std::string unused_err_string;
wtx.SubmitMemoryPoolAndRelay(unused_err_string, false);
}
}
bool CWalletTx::RelayWalletTransaction(interfaces::Chain::Lock& locked_chain)
bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay)
{
// Can't relay if wallet is not broadcasting
if (!pwallet->GetBroadcastTransactions()) return false;
// Don't relay coinbase transactions outside blocks
if (IsCoinBase()) return false;
// Don't relay abandoned transactions
if (isAbandoned()) return false;
// Don't relay conflicted or already confirmed transactions
if (GetDepthInMainChain(locked_chain) != 0) return false;
// Don't relay transactions that aren't accepted to the mempool
CValidationState unused_state;
if (!InMempool() && !AcceptToMemoryPool(locked_chain, unused_state)) return false;
// Don't try to relay if the node is not connected to the p2p network
if (!pwallet->chain().p2pEnabled()) return false;
// Try to relay the transaction
pwallet->WalletLogPrintf("Relaying wtx %s\n", GetHash().ToString());
pwallet->chain().relayTransaction(GetHash());
return true;
// Submit transaction to mempool for relay
pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString());
// We must set fInMempool here - while it will be re-set to true by the
// entered-mempool callback, if we did not there would be a race where a
// user could call sendmoney in a loop and hit spurious out of funds errors
// because we think that this newly generated transaction's change is
// unavailable as we're not yet aware that it is in the mempool.
//
// Irrespective of the failure reason, un-marking fInMempool
// out-of-order is incorrect - it should be unmarked when
// TransactionRemovedFromMempool fires.
bool ret = pwallet->chain().broadcastTransaction(tx, err_string, pwallet->m_default_max_tx_fee, relay);
fInMempool |= ret;
return ret;
}
std::set<uint256> CWalletTx::GetConflicts() const
@ -2366,7 +2365,7 @@ void CWallet::ResendWalletTransactions()
if (m_best_block_time < nLastResend) return;
nLastResend = GetTime();
int relayed_tx_count = 0;
int submitted_tx_count = 0;
{ // locked_chain and cs_wallet scope
auto locked_chain = chain().lock();
@ -2378,12 +2377,13 @@ void CWallet::ResendWalletTransactions()
// only rebroadcast unconfirmed txes older than 5 minutes before the
// last block was found
if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
if (wtx.RelayWalletTransaction(*locked_chain)) ++relayed_tx_count;
std::string unused_err_string;
if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true)) ++submitted_tx_count;
}
} // locked_chain and cs_wallet
if (relayed_tx_count > 0) {
WalletLogPrintf("%s: rebroadcast %u unconfirmed transactions\n", __func__, relayed_tx_count);
if (submitted_tx_count > 0) {
WalletLogPrintf("%s: resubmit %u unconfirmed transactions\n", __func__, submitted_tx_count);
}
}
@ -3322,12 +3322,10 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
if (fBroadcastTransactions)
{
// Broadcast
if (!wtx.AcceptToMemoryPool(*locked_chain, state)) {
WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", FormatStateMessage(state));
std::string err_string;
if (!wtx.SubmitMemoryPoolAndRelay(err_string, true)) {
WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string);
// TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure.
} else {
wtx.RelayWalletTransaction(*locked_chain);
}
}
}
@ -4662,18 +4660,6 @@ bool CWalletTx::IsImmatureCoinBase(interfaces::Chain::Lock& locked_chain) const
return GetBlocksToMaturity(locked_chain) > 0;
}
bool CWalletTx::AcceptToMemoryPool(interfaces::Chain::Lock& locked_chain, CValidationState& state)
{
// We must set fInMempool here - while it will be re-set to true by the
// entered-mempool callback, if we did not there would be a race where a
// user could call sendmoney in a loop and hit spurious out of funds errors
// because we think that this newly generated transaction's change is
// unavailable as we're not yet aware that it is in the mempool.
bool ret = locked_chain.submitToMemoryPool(tx, pwallet->m_default_max_tx_fee, state);
fInMempool |= ret;
return ret;
}
void CWallet::LearnRelatedScripts(const CPubKey& key, OutputType type)
{
if (key.IsCompressed() && (type == OutputType::P2SH_SEGWIT || type == OutputType::BECH32)) {

7
src/wallet/wallet.h
View file

@ -579,11 +579,8 @@ public:
int64_t GetTxTime() const;
// Pass this transaction to the node to relay to its peers
bool RelayWalletTransaction(interfaces::Chain::Lock& locked_chain);
/** Pass this transaction to the mempool. Fails if absolute fee exceeds absurd fee. */
bool AcceptToMemoryPool(interfaces::Chain::Lock& locked_chain, CValidationState& state);
// Pass this transaction to node for mempool insertion and relay to peers if flag set to true
bool SubmitMemoryPoolAndRelay(std::string& err_string, bool relay);
// TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
// annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation