Skip to content

Commit b57b68c

Browse files
knstknst
committed
fix: misusing chainlocks to determine expiring of cj's tx
This commit fixes previous commit and update unit tests accordingly to catch this issue
1 parent a894ff7 commit b57b68c

5 files changed

Lines changed: 67 additions & 53 deletions

File tree

‎src/coinjoin/coinjoin.cpp‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
#include <bls/bls.h>
1717
#include <chainlock/chainlock.h>
1818
#include <instantsend/instantsend.h>
19-
#include <masternode/sync.h>
2019

2120
#include <string>
2221

@@ -81,13 +80,6 @@ bool CCoinJoinBroadcastTx::CheckSignature(const CBLSPublicKey& blsPubKey) const
8180
return true;
8281
}
8382

84-
bool CCoinJoinBroadcastTx::IsExpired(const CBlockIndex* pindex) const
85-
{
86-
// expire confirmed DSTXes after ~1h since confirmation
87-
if (!nConfirmedHeight.has_value() || pindex->nHeight < *nConfirmedHeight) return false; // not mined yet
88-
return (pindex->nHeight - *nConfirmedHeight > 24); // mined more than an hour ago
89-
}
90-
9183
bool CCoinJoinBroadcastTx::IsValidStructure() const
9284
{
9385
// some trivial checks only
@@ -415,13 +407,22 @@ CCoinJoinBroadcastTx CDSTXManager::GetDSTX(const uint256& hash)
415407
return (it == mapDSTX.end()) ? CCoinJoinBroadcastTx() : it->second;
416408
}
417409

410+
bool CDSTXManager::IsTxExpired(const CCoinJoinBroadcastTx& tx, const CBlockIndex* pindex) const
411+
{
412+
// expire confirmed DSTXes after ~1h since confirmation or chainlocked
413+
const auto& opt_confirmed_height = tx.GetConfirmedHeight();
414+
if (!opt_confirmed_height.has_value() || pindex->nHeight < *opt_confirmed_height) return false; // not mined yet
415+
return (pindex->nHeight - *opt_confirmed_height > 24) ||
416+
m_chainlocks.HasChainLock(pindex->nHeight, *pindex->phashBlock); // mined more than an hour ago or chainlocked
417+
}
418+
418419
void CDSTXManager::CheckDSTXes(const CBlockIndex* pindex)
419420
{
420421
AssertLockNotHeld(cs_mapdstx);
421422
LOCK(cs_mapdstx);
422423
auto it = mapDSTX.begin();
423424
while (it != mapDSTX.end()) {
424-
if (it->second.IsExpired(pindex) || m_chainlocks.HasChainLock(pindex->nHeight, *pindex->phashBlock)) {
425+
if (IsTxExpired(it->second, pindex)) {
425426
mapDSTX.erase(it++);
426427
} else {
427428
++it;
@@ -430,16 +431,16 @@ void CDSTXManager::CheckDSTXes(const CBlockIndex* pindex)
430431
LogPrint(BCLog::COINJOIN, "CoinJoin::CheckDSTXes -- mapDSTX.size()=%llu\n", mapDSTX.size());
431432
}
432433

433-
void CDSTXManager::UpdatedBlockTip(const CBlockIndex* pindex, const CMasternodeSync& mn_sync)
434+
void CDSTXManager::UpdatedBlockTip(const CBlockIndex* pindex)
434435
{
435-
if (pindex && mn_sync.IsBlockchainSynced()) {
436+
if (pindex) {
436437
CheckDSTXes(pindex);
437438
}
438439
}
439440

440-
void CDSTXManager::NotifyChainLock(const CBlockIndex* pindex, const CMasternodeSync& mn_sync)
441+
void CDSTXManager::NotifyChainLock(const CBlockIndex* pindex)
441442
{
442-
if (pindex && mn_sync.IsBlockchainSynced()) {
443+
if (pindex) {
443444
CheckDSTXes(pindex);
444445
}
445446
}

‎src/coinjoin/coinjoin.h‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ class CChainState;
2626
class CBLSPublicKey;
2727
class CBlockIndex;
2828
class ChainstateManager;
29-
class CMasternodeSync;
3029
class CTxMemPool;
3130

3231
namespace chainlock {
@@ -280,10 +279,8 @@ class CCoinJoinBroadcastTx
280279

281280
[[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const;
282281

283-
// Used only for unit tests
284-
[[nodiscard]] std::optional<int> GetConfirmedHeight() const { return nConfirmedHeight; }
282+
[[nodiscard]] const std::optional<int>& GetConfirmedHeight() const { return nConfirmedHeight; }
285283
void SetConfirmedHeight(std::optional<int> nConfirmedHeightIn) { assert(nConfirmedHeightIn == std::nullopt || *nConfirmedHeightIn > 0); nConfirmedHeight = nConfirmedHeightIn; }
286-
bool IsExpired(const CBlockIndex* pindex) const;
287284
[[nodiscard]] bool IsValidStructure() const;
288285
};
289286

@@ -384,20 +381,18 @@ class CDSTXManager
384381
void AddDSTX(const CCoinJoinBroadcastTx& dstx) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
385382
CCoinJoinBroadcastTx GetDSTX(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
386383

387-
void UpdatedBlockTip(const CBlockIndex* pindex, const CMasternodeSync& mn_sync)
388-
EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
389-
void NotifyChainLock(const CBlockIndex* pindex, const CMasternodeSync& mn_sync)
390-
EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
391-
384+
// CDSNotificationInterface
385+
void UpdatedBlockTip(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
386+
void NotifyChainLock(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
392387
void TransactionAddedToMempool(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
393388
void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
394389
EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
395390
void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex*)
396391
EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
397392

398393
private:
399-
void CheckDSTXes(const CBlockIndex* pindex)
400-
EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
394+
bool IsTxExpired(const CCoinJoinBroadcastTx& tx, const CBlockIndex* pindex) const EXCLUSIVE_LOCKS_REQUIRED(cs_mapdstx);
395+
void CheckDSTXes(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx);
401396
void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional<int> nHeight)
402397
EXCLUSIVE_LOCKS_REQUIRED(cs_mapdstx);
403398
};

‎src/dsnotificationinterface.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,9 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
7272
if (fInitialDownload)
7373
return;
7474

75-
m_dstxman.UpdatedBlockTip(pindexNew, m_mn_sync);
75+
if (m_mn_sync.IsBlockchainSynced()) {
76+
m_dstxman.UpdatedBlockTip(pindexNew);
77+
}
7678

7779
m_llmq_ctx->isman->UpdatedBlockTip(pindexNew);
7880
if (m_govman.IsValid()) {
@@ -117,7 +119,9 @@ void CDSNotificationInterface::NotifyChainLock(const CBlockIndex* pindex,
117119
const std::shared_ptr<const chainlock::ChainLockSig>& clsig)
118120
{
119121
Assert(m_llmq_ctx)->isman->NotifyChainLock(pindex);
120-
m_dstxman.NotifyChainLock(pindex, m_mn_sync);
122+
if (m_mn_sync.IsBlockchainSynced()) {
123+
m_dstxman.NotifyChainLock(pindex);
124+
}
121125
}
122126

123127
std::unique_ptr<CDSNotificationInterface> g_ds_notification_interface;

‎src/test/coinjoin_dstxmanager_tests.cpp‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,47 @@ BOOST_AUTO_TEST_CASE(add_get_dstx)
4949
BOOST_CHECK_EQUAL(got.tx->GetHash().ToString(), dstx.tx->GetHash().ToString());
5050
}
5151

52+
BOOST_AUTO_TEST_CASE(broadcasttx_expiry_height_logic)
53+
{
54+
// Build a valid-looking CCoinJoinBroadcastTx with confirmed height
55+
CCoinJoinBroadcastTx dstx;
56+
{
57+
CMutableTransaction mtx;
58+
const int participants = std::max(3, CoinJoin::GetMinPoolParticipants());
59+
for (int i = 0; i < participants; ++i) {
60+
mtx.vin.emplace_back(COutPoint(uint256::TWO, i));
61+
CScript spk;
62+
spk << OP_DUP << OP_HASH160 << std::vector<unsigned char>(20, i) << OP_EQUALVERIFY << OP_CHECKSIG;
63+
mtx.vout.emplace_back(CoinJoin::GetSmallestDenomination(), spk);
64+
}
65+
dstx.tx = MakeTransactionRef(mtx);
66+
dstx.m_protxHash = uint256::ONE;
67+
// mark as confirmed at height 100
68+
dstx.SetConfirmedHeight(100);
69+
}
70+
71+
int expiry_height = 124; // 125 - 100 == 25 > 24 → expired by height
72+
73+
// Minimal CBlockIndex with required fields
74+
// Create a minimal block index to satisfy the interface
75+
CBlockIndex index;
76+
uint256 blk_hash = uint256S("03");
77+
index.nHeight = expiry_height;
78+
index.phashBlock = &blk_hash;
79+
80+
auto& man = *Assert(m_node.dstxman);
81+
auto& hash = dstx.tx->GetHash();
82+
man.AddDSTX(dstx);
83+
BOOST_CHECK(static_cast<bool>(man.GetDSTX(hash)));
84+
85+
man.UpdatedBlockTip(&index);
86+
BOOST_CHECK(static_cast<bool>(man.GetDSTX(hash)));
87+
88+
index.nHeight = expiry_height + 1;
89+
man.UpdatedBlockTip(&index);
90+
BOOST_CHECK(!static_cast<bool>(man.GetDSTX(hash)));
91+
}
92+
5293
BOOST_AUTO_TEST_CASE(update_heights_block_connect_disconnect)
5394
{
5495
CCoinJoinBroadcastTx dstx = MakeDSTX();

‎src/test/coinjoin_inouts_tests.cpp‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -105,31 +105,4 @@ BOOST_AUTO_TEST_CASE(queue_timeout_bounds)
105105
// Reset mock time
106106
SetMockTime(0);
107107
}
108-
109-
BOOST_AUTO_TEST_CASE(broadcasttx_expiry_height_logic)
110-
{
111-
// Build a valid-looking CCoinJoinBroadcastTx with confirmed height
112-
CCoinJoinBroadcastTx dstx;
113-
{
114-
CMutableTransaction mtx;
115-
const int participants = std::max(3, CoinJoin::GetMinPoolParticipants());
116-
for (int i = 0; i < participants; ++i) {
117-
mtx.vin.emplace_back(COutPoint(uint256::TWO, i));
118-
mtx.vout.emplace_back(CoinJoin::GetSmallestDenomination(), P2PKHScript(static_cast<uint8_t>(i)));
119-
}
120-
dstx.tx = MakeTransactionRef(mtx);
121-
dstx.m_protxHash = uint256::ONE;
122-
// mark as confirmed at height 100
123-
dstx.SetConfirmedHeight(100);
124-
}
125-
126-
// Minimal CBlockIndex with required fields
127-
// Create a minimal block index to satisfy the interface
128-
CBlockIndex index;
129-
uint256 blk_hash = uint256S("03");
130-
index.nHeight = 125; // 125 - 100 == 25 > 24 → expired by height
131-
index.phashBlock = &blk_hash;
132-
BOOST_CHECK(dstx.IsExpired(&index));
133-
}
134-
135108
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)