Skip to content

Commit d81ff3c

Browse files
thepastaclawclaude
andcommitted
fix: bound CoinJoin entry intake
CCoinJoinEntry deserialized its input and output vectors with the unbounded operator>>. Replace the SERIALIZE_METHODS with an explicit reader that bounds both vectors via the shared UnserializeVectorWithMaxSize() before allocating, while preserving the collateral-penalty behavior AddEntry relies on: - max+1 collateral penalty: accept one input over COINJOIN_ENTRY_MAX_SIZE so AddEntry can still charge the collateral for an entry that is merely too large; a count beyond that is rejected before any element is decoded. - txCollateral / fHasOversizedTxOut invariant: outputs follow txCollateral on the wire, so an oversized output count must not throw mid-entry. Set fHasOversizedTxOut and leave txCollateral intact so AddEntry can reject the entry with ERR_MAXIMUM and consume its collateral. Counts above the generic MAX_SIZE cap are not a CoinJoin-level violation but a malformed message, and still throw from ReadCompactSize as they do for any other network message. fHasOversizedTxOut is cleared at the start of Unserialize, so a half-read entry can never reach AddEntry's collateral path. Addresses review thread discussion_r3556209431. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c72a8f5 commit d81ff3c

3 files changed

Lines changed: 120 additions & 2 deletions

File tree

src/coinjoin/coinjoin.h

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include <netaddress.h>
1414
#include <primitives/block.h>
1515
#include <primitives/transaction.h>
16+
#include <serialize.h>
1617
#include <sync.h>
1718
#include <timedata.h>
1819
#include <util/translation.h>
@@ -154,6 +155,7 @@ class CCoinJoinEntry
154155
CTransactionRef txCollateral;
155156
// memory only
156157
CService addr;
158+
bool fHasOversizedTxOut{false};
157159

158160
CCoinJoinEntry() :
159161
txCollateral(MakeTransactionRef(CMutableTransaction{}))
@@ -167,9 +169,34 @@ class CCoinJoinEntry
167169
{
168170
}
169171

170-
SERIALIZE_METHODS(CCoinJoinEntry, obj)
172+
template <typename Stream>
173+
void Serialize(Stream& s) const
171174
{
172-
READWRITE(obj.vecTxDSIn, obj.txCollateral, obj.vecTxOut);
175+
s << vecTxDSIn << txCollateral << vecTxOut;
176+
}
177+
178+
template <typename Stream>
179+
void Unserialize(Stream& s)
180+
{
181+
fHasOversizedTxOut = false;
182+
183+
// Accept one input over the cap so AddEntry can apply the collateral
184+
// penalty for an entry that is merely too large; anything bigger is
185+
// rejected before any element is decoded.
186+
if (!UnserializeVectorWithMaxSize(s, vecTxDSIn, COINJOIN_ENTRY_MAX_SIZE + 1)) {
187+
throw std::ios_base::failure("CCoinJoinEntry::vecTxDSIn size too large");
188+
}
189+
190+
s >> txCollateral;
191+
192+
if (!UnserializeVectorWithMaxSize(s, vecTxOut, COINJOIN_ENTRY_MAX_SIZE)) {
193+
// Outputs follow txCollateral on the wire, so an output count over the
194+
// cap must not throw mid-entry: flag it and leave txCollateral intact
195+
// so AddEntry can reject the entry with ERR_MAXIMUM and consume its
196+
// collateral. A count above the generic MAX_SIZE cap is malformed and
197+
// still throws from ReadCompactSize.
198+
fHasOversizedTxOut = true;
199+
}
173200
}
174201

175202
bool AddScriptSig(const CTxIn& txin);

src/coinjoin/server.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,27 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
610610
return false;
611611
}
612612

613+
if (entry.fHasOversizedTxOut || entry.vecTxOut.size() > COINJOIN_ENTRY_MAX_SIZE) {
614+
const size_t txout_size{entry.fHasOversizedTxOut ? COINJOIN_ENTRY_MAX_SIZE + 1 : entry.vecTxOut.size()};
615+
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: too many outputs! %d/%d\n", __func__,
616+
static_cast<int>(txout_size), static_cast<int>(COINJOIN_ENTRY_MAX_SIZE));
617+
nMessageIDRet = ERR_MAXIMUM;
618+
CTransactionRef txCollateralToConsume;
619+
{
620+
LOCK(cs_coinjoin);
621+
const auto it = std::ranges::find_if(vecSessionCollaterals, [&entry](const auto& txCollateral) {
622+
return *entry.txCollateral == *txCollateral;
623+
});
624+
if (it != vecSessionCollaterals.end()) {
625+
txCollateralToConsume = *it;
626+
}
627+
}
628+
if (txCollateralToConsume) {
629+
ConsumeCollateral(txCollateralToConsume);
630+
}
631+
return false;
632+
}
633+
613634
if (!CoinJoin::IsCollateralValid(m_chainman, m_isman, mempool, *entry.txCollateral)) {
614635
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: collateral not valid!\n", __func__);
615636
nMessageIDRet = ERR_INVALID_COLLATERAL;

src/test/coinjoin_inouts_tests.cpp

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,76 @@ BOOST_AUTO_TEST_CASE(server_signfinaltx_participant_oversized_count_is_rejected_
287287
BOOST_CHECK_EQUAL(server.GetEntriesCount(), 1);
288288
}
289289

290+
BOOST_AUTO_TEST_CASE(entry_allows_one_oversized_input_for_collateral_penalty)
291+
{
292+
CCoinJoinEntry oversized_entry;
293+
oversized_entry.vecTxDSIn.resize(COINJOIN_ENTRY_MAX_SIZE + 1);
294+
295+
CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
296+
stream << oversized_entry;
297+
298+
CCoinJoinEntry entry;
299+
BOOST_CHECK_NO_THROW(stream >> entry);
300+
BOOST_CHECK_EQUAL(entry.vecTxDSIn.size(), COINJOIN_ENTRY_MAX_SIZE + 1);
301+
BOOST_CHECK(entry.vecTxOut.empty());
302+
}
303+
304+
BOOST_AUTO_TEST_CASE(entry_rejects_huge_inputs_before_materializing)
305+
{
306+
CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
307+
WriteCompactSize(stream, COINJOIN_ENTRY_MAX_SIZE + 2);
308+
309+
CCoinJoinEntry entry;
310+
BOOST_CHECK_THROW(stream >> entry, std::ios_base::failure);
311+
BOOST_CHECK(entry.vecTxDSIn.empty());
312+
}
313+
314+
BOOST_AUTO_TEST_CASE(entry_flags_oversized_outputs_before_materializing)
315+
{
316+
CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
317+
WriteCompactSize(stream, 0);
318+
stream << MakeTransactionRef(CMutableTransaction{});
319+
WriteCompactSize(stream, COINJOIN_ENTRY_MAX_SIZE + 1);
320+
321+
CCoinJoinEntry entry;
322+
BOOST_CHECK_NO_THROW(stream >> entry);
323+
BOOST_CHECK(entry.fHasOversizedTxOut);
324+
BOOST_CHECK(entry.vecTxOut.empty());
325+
}
326+
327+
BOOST_AUTO_TEST_CASE(entry_throws_on_huge_outputs_before_materializing)
328+
{
329+
CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
330+
WriteCompactSize(stream, 0);
331+
stream << MakeTransactionRef(CMutableTransaction{});
332+
WriteCompactSize(stream, MAX_SIZE + 1);
333+
334+
// Above the generic CompactSize cap the message is simply malformed, so the
335+
// standard deserialization error is raised. No output is materialized, and the
336+
// half-read entry is not left looking like an oversized-but-chargeable one:
337+
// fHasOversizedTxOut stays clear, so it cannot reach AddEntry's collateral path.
338+
CCoinJoinEntry entry;
339+
BOOST_CHECK_THROW(stream >> entry, std::ios_base::failure);
340+
BOOST_CHECK(!entry.fHasOversizedTxOut);
341+
BOOST_CHECK(entry.vecTxOut.empty());
342+
}
343+
344+
BOOST_AUTO_TEST_CASE(entry_accepts_max_sized_vectors)
345+
{
346+
CCoinJoinEntry entry;
347+
entry.vecTxDSIn.resize(COINJOIN_ENTRY_MAX_SIZE); // exactly the cap must be accepted
348+
entry.vecTxOut.resize(COINJOIN_ENTRY_MAX_SIZE);
349+
350+
CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
351+
stream << entry;
352+
353+
CCoinJoinEntry roundtripped;
354+
BOOST_CHECK_NO_THROW(stream >> roundtripped);
355+
BOOST_CHECK_EQUAL(roundtripped.vecTxDSIn.size(), COINJOIN_ENTRY_MAX_SIZE);
356+
BOOST_CHECK_EQUAL(roundtripped.vecTxOut.size(), COINJOIN_ENTRY_MAX_SIZE);
357+
BOOST_CHECK(!roundtripped.fHasOversizedTxOut);
358+
}
359+
290360
BOOST_AUTO_TEST_CASE(queue_timeout_bounds)
291361
{
292362
CCoinJoinQueue dsq;

0 commit comments

Comments
 (0)