merge bitcoin#15632: Remove ResendWalletTransactions from the Validation Interface

This commit is contained in:
Kittywhiskers Van Gogh 2021-12-12 21:27:51 +05:30
parent 3284d82296
commit ed48a889bf
9 changed files with 56 additions and 48 deletions

View File

@ -217,17 +217,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<const llmq::CChainLockSig>& clsig) override
{
m_notifications->NotifyChainLock(pindexChainLock, clsig);
@ -365,6 +359,7 @@ public:
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(); }

View File

@ -223,6 +223,9 @@ public:
//! 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 +259,8 @@ public:
virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason) {}
virtual void BlockConnected(const CBlock& block, const std::vector<CTransactionRef>& 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<const llmq::CChainLockSig>& clsig) {}
virtual void NotifyTransactionLock(const CTransactionRef &tx, const std::shared_ptr<const llmq::CInstantSendLock>& islock) {}
};

View File

@ -237,8 +237,6 @@ namespace {
/** Expiration-time ordered list of (expire time, relay map entry) pairs. */
std::deque<std::pair<int64_t, MapRelay::iterator>> vRelayExpiration GUARDED_BY(cs_main);
std::atomic<int64_t> nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block
struct IteratorComparator
{
template<typename I>
@ -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
//

View File

@ -25,7 +25,6 @@ struct ValidationInterfaceConnections {
boost::signals2::scoped_connection BlockDisconnected;
boost::signals2::scoped_connection TransactionRemovedFromMempool;
boost::signals2::scoped_connection ChainStateFlushed;
boost::signals2::scoped_connection Broadcast;
boost::signals2::scoped_connection BlockChecked;
boost::signals2::scoped_connection NewPoWValidBlock;
boost::signals2::scoped_connection AcceptedBlockHeader;
@ -48,7 +47,6 @@ struct MainSignalsInstance {
boost::signals2::signal<void (const std::shared_ptr<const CBlock> &, const CBlockIndex* pindexDisconnected)> BlockDisconnected;
boost::signals2::signal<void (const CTransactionRef &, MemPoolRemovalReason)> TransactionRemovedFromMempool;
boost::signals2::signal<void (const CBlockLocator &)> ChainStateFlushed;
boost::signals2::signal<void (int64_t nBestBlockTime, CConnman* connman)> Broadcast;
boost::signals2::signal<void (const CBlock&, const CValidationState&)> BlockChecked;
boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
boost::signals2::signal<void (const CBlockIndex *)>AcceptedBlockHeader;
@ -122,7 +120,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
conns.NotifyChainLock = g_signals.m_internals->NotifyChainLock.connect(std::bind(&CValidationInterface::NotifyChainLock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1, std::placeholders::_2));
conns.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, std::placeholders::_1));
conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1, std::placeholders::_2));
conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2));
conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
conns.NotifyGovernanceObject = g_signals.m_internals->NotifyGovernanceObject.connect(std::bind(&CValidationInterface::NotifyGovernanceObject, pwalletIn, std::placeholders::_1));
@ -205,10 +202,6 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) {
});
}
void CMainSignals::Broadcast(int64_t nBestBlockTime, CConnman* connman) {
m_internals->Broadcast(nBestBlockTime, connman);
}
void CMainSignals::BlockChecked(const CBlock& block, const CValidationState& state) {
m_internals->BlockChecked(block, state);
}

View File

@ -156,8 +156,6 @@ protected:
* Called on a background thread.
*/
virtual void ChainStateFlushed(const CBlockLocator &locator) {}
/** Tells listeners to broadcast their data. */
virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {}
/**
* Notifies listeners of a block validation result.
* If the provided CValidationState IsValid, the provided block
@ -216,7 +214,6 @@ public:
void NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig> &sig);
void NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff);
void ChainStateFlushed(const CBlockLocator &);
void Broadcast(int64_t nBestBlockTime, CConnman* connman);
void BlockChecked(const CBlock&, const CValidationState&);
void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr<const CBlock>&);
};

View File

@ -83,8 +83,9 @@ void StartWallets(CScheduler& scheduler)
pwallet->postInitProcess();
}
// Run a thread to flush wallet periodically
// Schedule periodic wallet flushes and tx rebroadcasts
scheduler.scheduleEvery(MaybeCompactWalletDB, 500);
scheduler.scheduleEvery(MaybeResendWalletTxs, 1000);
if (!fMasternodeMode && CCoinJoinClientOptions::IsEnabled()) {
scheduler.scheduleEvery(std::bind(&DoCoinJoinMaintenance, std::ref(*g_connman)), 1 * 1000);

View File

@ -1473,6 +1473,10 @@ void CWallet::BlockDisconnected(const CBlock& block) {
fAnonymizableTallyCachedNonDenom = false;
}
void CWallet::UpdatedBlockTip()
{
m_best_block_time = GetTime();
}
void CWallet::BlockUntilSyncedToCurrentChain() {
@ -2598,8 +2602,21 @@ bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
return CTransaction(tx1) == CTransaction(tx2);
}
void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime)
// Rebroadcast transactions from the wallet. We do this on a random timer
// to slightly obfuscate which transactions come from our wallet.
//
// Ideally, we'd only resend transactions that we think should have been
// mined in the most recent block. Any transaction that wasn't in the top
// blockweight of transactions in the mempool shouldn't have been mined,
// and so is probably just sitting in the mempool waiting to be confirmed.
// Rebroadcasting does nothing to speed up confirmation and only damages
// privacy.
void CWallet::ResendWalletTransactions()
{
// During reindex, importing and IBD, old wallet transactions become
// unconfirmed. Don't resend them as that would spam other nodes.
if (!chain().isReadyToBroadcast()) return;
// Do this infrequently and randomly to avoid giving away
// that these are our transactions.
if (GetTime() < nNextResend || !fBroadcastTransactions) return;
@ -2608,12 +2625,13 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in
if (fFirst) return;
// Only do it if there's been a new block since last time
if (nBestBlockTime < nLastResend) return;
if (m_best_block_time < nLastResend) return;
nLastResend = GetTime();
int relayed_tx_count = 0;
{ // mempool.cs and cs_wallet scope
{ // locked_chain, mempool.cs and cs_wallet scope
auto locked_chain = chain().lock();
LOCK2(mempool.cs, cs_wallet);
// Relay transactions
@ -2621,10 +2639,10 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in
CWalletTx& wtx = item.second;
// only rebroadcast unconfirmed txes older than 5 minutes before the
// last block was found
if (wtx.nTimeReceived > nBestBlockTime - 5 * 60) continue;
relayed_tx_count += wtx.RelayWalletTransaction(*locked_chain) ? 1 : 0;
if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
if (wtx.RelayWalletTransaction(*locked_chain)) ++relayed_tx_count;
}
} // mempool.cs and cs_wallet
} // locked_chain, mempool.cs and cs_wallet
if (relayed_tx_count > 0) {
WalletLogPrintf("%s: rebroadcast %u unconfirmed transactions\n", __func__, relayed_tx_count);
@ -2633,7 +2651,12 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in
/** @} */ // end of mapWallet
void MaybeResendWalletTxs()
{
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
pwallet->ResendWalletTransactions();
}
}
/** @defgroup Actions

View File

@ -657,6 +657,8 @@ private:
int64_t nNextResend = 0;
int64_t nLastResend = 0;
bool fBroadcastTransactions = false;
// Local time that the tip block was received. Used to schedule wallet rebroadcasts.
std::atomic<int64_t> m_best_block_time {0};
mutable bool fAnonymizableTallyCached = false;
mutable std::vector<CompactTallyItem> vecAnonymizableTallyCached;
@ -983,6 +985,7 @@ public:
void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) override;
void BlockConnected(const CBlock& block, const std::vector<CTransactionRef>& vtxConflicted) override;
void BlockDisconnected(const CBlock& block) override;
void UpdatedBlockTip() override;
int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update);
struct ScanResult {
@ -1003,7 +1006,7 @@ public:
ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;
void ReacceptWalletTransactions();
void ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime) override;
void ResendWalletTransactions();
struct Balance {
CAmount m_mine_trusted{0}; //!< Trusted, at depth=GetBalance.min_depth or more
CAmount m_mine_untrusted_pending{0}; //!< Untrusted, but in mempool (pending)
@ -1282,6 +1285,12 @@ public:
bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
};
/**
* Called periodically by the schedule thread. Prompts individual wallets to resend
* their transactions. Actual rebroadcast schedule is managed by the wallets themselves.
*/
void MaybeResendWalletTxs();
/** A key allocated from the key pool. */
class CReserveKey final : public CReserveScript
{

View File

@ -39,6 +39,12 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
self.log.info("Create a new transaction and wait until it's broadcast")
txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
# Wallet rebroadcast is first scheduled 1 sec after startup (see
# nNextResend in ResendWalletTransactions()). Sleep for just over a
# second to be certain that it has been called before the first
# setmocktime call below.
time.sleep(1.1)
# Can take a few seconds due to transaction trickling
def wait_p2p():
self.bump_mocktime(1)