Skip to content

Commit 461e49f

Browse files
TheBlueMattTheBlueMatt
committed
SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected
This simplifies fixing the wallet-returns-stale-info issue as we can now hold cs_wallet across an entire block instead of only per-tx (though we only actually do so in the next commit). This change also removes the NOT_IN_BLOCK constant in favor of only passing the CBlockIndex* parameter to SyncTransactions when a new block is being connected, instead of also when a block is being disconnected. This change adds a parameter to BlockConnectedDisconnected which lists the transactions which were removed from mempool due to confliction as a result of this operation. While its somewhat of a shame to make block-validation-logic generate a list of mempool changes to be included in its generated callbacks, fixing this isnt too hard. Further in this change-set, CValidationInterface starts listening to mempool directly, placing it in the middle and giving it a bit of logic to know how to route notifications from block-validation, mempool, etc (though not listening for conflicted-removals yet).
1 parent f404334 commit 461e49f

File tree

9 files changed

+109
-63
lines changed

9 files changed

+109
-63
lines changed

‎src/net_processing.cpp‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -744,21 +744,23 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn) : connman(connmanI
744744
recentRejects.reset(new CRollingBloomFilter(120000, 0.000001));
745745
}
746746

747-
void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock) {
748-
if (nPosInBlock == CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK)
749-
return;
750-
747+
void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted) {
751748
LOCK(cs_main);
752749

753750
std::vector<uint256> vOrphanErase;
754-
// Which orphan pool entries must we evict?
755-
for (size_t j = 0; j < tx.vin.size(); j++) {
756-
auto itByPrev = mapOrphanTransactionsByPrev.find(tx.vin[j].prevout);
757-
if (itByPrev == mapOrphanTransactionsByPrev.end()) continue;
758-
for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
759-
const CTransaction& orphanTx = *(*mi)->second.tx;
760-
const uint256& orphanHash = orphanTx.GetHash();
761-
vOrphanErase.push_back(orphanHash);
751+
752+
for (const CTransactionRef& ptx : pblock->vtx) {
753+
const CTransaction& tx = *ptx;
754+
755+
// Which orphan pool entries must we evict?
756+
for (size_t j = 0; j < tx.vin.size(); j++) {
757+
auto itByPrev = mapOrphanTransactionsByPrev.find(tx.vin[j].prevout);
758+
if (itByPrev == mapOrphanTransactionsByPrev.end()) continue;
759+
for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
760+
const CTransaction& orphanTx = *(*mi)->second.tx;
761+
const uint256& orphanHash = orphanTx.GetHash();
762+
vOrphanErase.push_back(orphanHash);
763+
}
762764
}
763765
}
764766

‎src/net_processing.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ class PeerLogicValidation : public CValidationInterface {
3030
public:
3131
PeerLogicValidation(CConnman* connmanIn);
3232

33-
virtual void SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock);
33+
virtual void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted);
3434
virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
3535
virtual void BlockChecked(const CBlock& block, const CValidationState& state);
3636
virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock);

‎src/validation.cpp‎

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -949,7 +949,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
949949
}
950950
}
951951

952-
GetMainSignals().SyncTransaction(tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
952+
GetMainSignals().TransactionAddedToMempool(ptx);
953953

954954
return true;
955955
}
@@ -2120,7 +2120,8 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
21202120
CBlockIndex *pindexDelete = chainActive.Tip();
21212121
assert(pindexDelete);
21222122
// Read block from disk.
2123-
CBlock block;
2123+
std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
2124+
CBlock& block = *pblock;
21242125
if (!ReadBlockFromDisk(block, pindexDelete, chainparams.GetConsensus()))
21252126
return AbortNode(state, "Failed to read block");
21262127
// Apply the block atomically to the chain state.
@@ -2162,9 +2163,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
21622163
UpdateTip(pindexDelete->pprev, chainparams);
21632164
// Let wallets know transactions went from 1-confirmed to
21642165
// 0-confirmed or conflicted:
2165-
for (const auto& tx : block.vtx) {
2166-
GetMainSignals().SyncTransaction(*tx, pindexDelete->pprev, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
2167-
}
2166+
GetMainSignals().BlockDisconnected(pblock);
21682167
return true;
21692168
}
21702169

@@ -2511,29 +2510,9 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
25112510
pindexFork = chainActive.FindFork(pindexOldTip);
25122511
fInitialDownload = IsInitialBlockDownload();
25132512

2514-
// TODO: Temporarily ensure that mempool removals are notified before
2515-
// connected transactions. This shouldn't matter, but the abandoned
2516-
// state of transactions in our wallet is currently cleared when we
2517-
// receive another notification and there is a race condition where
2518-
// notification of a connected conflict might cause an outside process
2519-
// to abandon a transaction and then have it inadvertently cleared by
2520-
// the notification that the conflicted transaction was evicted.
2521-
2522-
// throw all transactions though the signal-interface
2523-
auto blocksConnected = connectTrace.GetBlocksConnected();
2524-
for (const PerBlockConnectTrace& trace : blocksConnected) {
2525-
assert(trace.conflictedTxs);
2526-
for (const auto& tx : *trace.conflictedTxs) {
2527-
GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
2528-
}
2529-
}
2530-
2531-
// Transactions in the connected block are notified
2532-
for (const PerBlockConnectTrace& trace : blocksConnected) {
2513+
for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
25332514
assert(trace.pblock && trace.pindex);
2534-
const CBlock& block = *(trace.pblock);
2535-
for (unsigned int i = 0; i < block.vtx.size(); i++)
2536-
GetMainSignals().SyncTransaction(*block.vtx[i], trace.pindex, i);
2515+
GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs);
25372516
}
25382517
}
25392518
// When we reach this point, we switched to a new tip (stored in pindexNewTip).

‎src/validationinterface.cpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ CMainSignals& GetMainSignals()
1414

1515
void RegisterValidationInterface(CValidationInterface* pwalletIn) {
1616
g_signals.UpdatedBlockTip.connect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3));
17-
g_signals.SyncTransaction.connect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2, _3));
17+
g_signals.TransactionAddedToMempool.connect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1));
18+
g_signals.BlockConnected.connect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3));
19+
g_signals.BlockDisconnected.connect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1));
1820
g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1));
1921
g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1));
2022
g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1));
@@ -33,7 +35,9 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
3335
g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1));
3436
g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1));
3537
g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1));
36-
g_signals.SyncTransaction.disconnect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2, _3));
38+
g_signals.TransactionAddedToMempool.disconnect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1));
39+
g_signals.BlockConnected.disconnect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3));
40+
g_signals.BlockDisconnected.disconnect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1));
3741
g_signals.UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3));
3842
g_signals.NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2));
3943
}
@@ -46,7 +50,9 @@ void UnregisterAllValidationInterfaces() {
4650
g_signals.Inventory.disconnect_all_slots();
4751
g_signals.SetBestChain.disconnect_all_slots();
4852
g_signals.UpdatedTransaction.disconnect_all_slots();
49-
g_signals.SyncTransaction.disconnect_all_slots();
53+
g_signals.TransactionAddedToMempool.disconnect_all_slots();
54+
g_signals.BlockConnected.disconnect_all_slots();
55+
g_signals.BlockDisconnected.disconnect_all_slots();
5056
g_signals.UpdatedBlockTip.disconnect_all_slots();
5157
g_signals.NewPoWValidBlock.disconnect_all_slots();
5258
}

‎src/validationinterface.h‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,14 @@
1010
#include <boost/shared_ptr.hpp>
1111
#include <memory>
1212

13+
#include "primitives/transaction.h" // CTransaction(Ref)
14+
1315
class CBlock;
1416
class CBlockIndex;
1517
struct CBlockLocator;
1618
class CBlockIndex;
1719
class CConnman;
1820
class CReserveScript;
19-
class CTransaction;
2021
class CValidationInterface;
2122
class CValidationState;
2223
class uint256;
@@ -33,7 +34,9 @@ void UnregisterAllValidationInterfaces();
3334
class CValidationInterface {
3435
protected:
3536
virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
36-
virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) {}
37+
virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {}
38+
virtual void BlockConnected(const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex, const std::vector<CTransactionRef> &txnConflicted) {}
39+
virtual void BlockDisconnected(const std::shared_ptr<const CBlock> &block) {}
3740
virtual void SetBestChain(const CBlockLocator &locator) {}
3841
virtual void UpdatedTransaction(const uint256 &hash) {}
3942
virtual void Inventory(const uint256 &hash) {}
@@ -50,17 +53,15 @@ class CValidationInterface {
5053
struct CMainSignals {
5154
/** Notifies listeners of updated block chain tip */
5255
boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip;
53-
/** A posInBlock value for SyncTransaction calls for transactions not
54-
* included in connected blocks such as transactions removed from mempool,
55-
* accepted to mempool or appearing in disconnected blocks.*/
56-
static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1;
57-
/** Notifies listeners of updated transaction data (transaction, and
58-
* optionally the block it is found in). Called with block data when
59-
* transaction is included in a connected block, and without block data when
60-
* transaction was accepted to mempool, removed from mempool (only when
61-
* removal was due to conflict from connected block), or appeared in a
62-
* disconnected block.*/
63-
boost::signals2::signal<void (const CTransaction &, const CBlockIndex *pindex, int posInBlock)> SyncTransaction;
56+
/** Notifies listeners of a transaction having been added to mempool. */
57+
boost::signals2::signal<void (const CTransactionRef &)> TransactionAddedToMempool;
58+
/**
59+
* Notifies listeners of a block being connected.
60+
* Provides a vector of transactions evicted from the mempool as a result.
61+
*/
62+
boost::signals2::signal<void (const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex, const std::vector<CTransactionRef> &)> BlockConnected;
63+
/** Notifies listeners of a block being disconnected */
64+
boost::signals2::signal<void (const std::shared_ptr<const CBlock> &)> BlockDisconnected;
6465
/** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */
6566
boost::signals2::signal<void (const uint256 &)> UpdatedTransaction;
6667
/** Notifies listeners of a new active block chain. */

‎src/wallet/wallet.cpp‎

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1116,11 +1116,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
11161116
}
11171117
}
11181118

1119-
void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
1120-
{
1121-
LOCK2(cs_main, cs_wallet);
1119+
void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) {
1120+
const CTransaction& tx = *ptx;
11221121

1123-
if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true))
1122+
if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true))
11241123
return; // Not one of ours
11251124

11261125
// If a transaction changes 'conflicted' state, that changes the balance
@@ -1133,6 +1132,38 @@ void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex,
11331132
}
11341133
}
11351134

1135+
void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) {
1136+
LOCK2(cs_main, cs_wallet);
1137+
SyncTransaction(ptx, NULL, -1);
1138+
}
1139+
1140+
void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) {
1141+
// TODO: Tempoarily ensure that mempool removals are notified before
1142+
// connected transactions. This shouldn't matter, but the abandoned
1143+
// state of transactions in our wallet is currently cleared when we
1144+
// receive another notification and there is a race condition where
1145+
// notification of a connected conflict might cause an outside process
1146+
// to abandon a transaction and then have it inadvertantly cleared by
1147+
// the notification that the conflicted transaction was evicted.
1148+
1149+
for (const CTransactionRef& ptx : vtxConflicted) {
1150+
LOCK2(cs_main, cs_wallet);
1151+
SyncTransaction(ptx, NULL, -1);
1152+
}
1153+
for (size_t i = 0; i < pblock->vtx.size(); i++) {
1154+
LOCK2(cs_main, cs_wallet);
1155+
SyncTransaction(pblock->vtx[i], pindex, i);
1156+
}
1157+
}
1158+
1159+
void CWallet::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) {
1160+
for (const CTransactionRef& ptx : pblock->vtx) {
1161+
LOCK2(cs_main, cs_wallet);
1162+
SyncTransaction(ptx, NULL, -1);
1163+
}
1164+
}
1165+
1166+
11361167

11371168
isminetype CWallet::IsMine(const CTxIn &txin) const
11381169
{

‎src/wallet/wallet.h‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -661,6 +661,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
661661

662662
void SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator>);
663663

664+
/* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected */
665+
void SyncTransaction(const CTransactionRef& tx, const CBlockIndex *pindexBlockConnected, int posInBlock);
666+
664667
/* the HD chain data model (external chain counters) */
665668
CHDChain hdChain;
666669

@@ -849,7 +852,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
849852
void MarkDirty();
850853
bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true);
851854
bool LoadToWallet(const CWalletTx& wtxIn);
852-
void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock) override;
855+
void TransactionAddedToMempool(const CTransactionRef& tx) override;
856+
void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) override;
857+
void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override;
853858
bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate);
854859
CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false);
855860
void ReacceptWalletTransactions();

‎src/zmq/zmqnotificationinterface.cpp‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,12 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co
144144
}
145145
}
146146

147-
void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
147+
void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx)
148148
{
149+
// Used by BlockConnected and BlockDisconnected as well, because they're
150+
// all the same external callback.
151+
const CTransaction& tx = *ptx;
152+
149153
for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); )
150154
{
151155
CZMQAbstractNotifier *notifier = *i;
@@ -160,3 +164,19 @@ void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CB
160164
}
161165
}
162166
}
167+
168+
void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted)
169+
{
170+
for (const CTransactionRef& ptx : pblock->vtx) {
171+
// Do a normal notify for each transaction added in the block
172+
TransactionAddedToMempool(ptx);
173+
}
174+
}
175+
176+
void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock)
177+
{
178+
for (const CTransactionRef& ptx : pblock->vtx) {
179+
// Do a normal notify for each transaction removed in block disconnection
180+
TransactionAddedToMempool(ptx);
181+
}
182+
}

‎src/zmq/zmqnotificationinterface.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ class CZMQNotificationInterface : public CValidationInterface
2525
void Shutdown();
2626

2727
// CValidationInterface
28-
void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock);
28+
void TransactionAddedToMempool(const CTransactionRef& tx);
29+
void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted);
30+
void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock);
2931
void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
3032

3133
private:

0 commit comments

Comments
 (0)