DragonX/dragonx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 3 Wiki Activity

audit: fix ADDR send amplification and the IVK-only witness deref (UNTESTED)

Browse Source 
Two more findings from the general audit. Compile-verified only, like the rest of
this branch.

main.cpp SendMessages / ADDR: the per-address loop called
    pto->PushAddrMessage(msgMaker.Make(flags, msg_type, pto->vAddrToSend))
i.e. it pushed the ENTIRE vector once per accepted address. One 24-byte getaddr
therefore produced N messages of N addresses each instead of one message of N --
on a live addrman (GetAddr returning ~494) that is ~494 x ~14.8 KB = ~7.3 MB of
upstream for a 24-byte request, and up to ~30 MB with a full addrman. All of it
serialized, with a double-SHA256 checksum per message, while cs_main is held, so
each burst also stalls block validation and RPC.

The locally built vAddr was accumulated and then discarded, and the
`vAddr.resize(MAX_ADDR_TO_SEND)` sat exactly where upstream has `vAddr.clear()` --
it can never fire, because vAddr.size() already equals MAX_ADDR_TO_SEND there.
This is a local regression, not inherited: 512da314a rewrote the correct upstream
form into this one.

Restores the upstream idiom (accumulate, flush in MAX_ADDR_TO_SEND batches, send
the remainder after the loop) and hoists the msg_type/make_flags selection out of
the loop, since neither varies per address.

wallet.cpp VerifyAndSetInitialWitness: five sites dereferenced
`*item.second.nullifier` on a boost::optional that can legitimately be unset. A
Sapling note discovered through an imported INCOMING viewing key has no nullifier
-- computing one requires the full viewing key, and z_importviewingkey calls only
AddSaplingIncomingViewingKey. Dereferencing it aborts the daemon with a boost
assertion rather than an RPC error, at the end of the import's own rescan; and
because the IVK-only note data is already persisted in the wallet transaction and
BuildWitnessCache is re-driven from ChainTip on every connected block, the node
then fails the same way on every restart.

The codebase already had the right pattern in the two neighbouring functions --
DecrementNoteWitnesses guards with `if (nd->nullifier && ...)` and BuildWitnessCache
with `if (!nd->nullifier) continue;` -- so only VerifyAndSetInitialWitness was
missing it. Those guards also establish the intended semantics: a note whose spend
depth cannot be computed is treated as UNSPENT, keeping its witness rather than
pruning a note we cannot prove spent. Adds a boost::optional overload of
SaplingWitnessMinimumHeight doing exactly that, and routes all five sites through it.

NOT fixed here, deliberately, because none can be done responsibly without tests:
  - the inline TLS handshake on ThreadSocketHandler (architectural: one slow
    unauthenticated connection can freeze all P2P I/O for up to 60s);
  - the getblocktemplate function-static leaking a CBlockTemplate per concurrent
    caller, assigned with cs_main released;
  - z_sendmany's pre-flight size estimate omitting Sapling spends, so a wallet with
    ~499+ notes builds an oversize, unbroadcastable transaction after minutes of
    proving;
  - z_shieldcoinbase's opt-in donation paying a hardcoded upstream-Hush z-address
    that no DragonX party can spend (a policy decision, not a code fix).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo
This commit is contained in:
DanS
2026-08-31 22:11:42 -05:00
parent fa3a4223ec
commit 520e1e0ede
3 changed files with 43 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
src/main.cpp
View File

@@ -8336,6 +8336,24 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
// Message: addr
if (fSendTrickle)
{
// Accumulate into vAddr and send it ONCE (or in MAX_ADDR_TO_SEND-sized batches).
// This loop previously pushed the ENTIRE pto->vAddrToSend on every accepted address,
// so a single 24-byte getaddr produced N messages of N addresses each instead of one
// message of N -- ~500x the intended bandwidth on a typical addrman, all serialized
// (with per-message double-SHA256 checksums) while cs_main is held. The locally built
// vAddr was accumulated and then discarded, and the vAddr.resize(MAX_ADDR_TO_SEND) was
// a no-op standing where upstream has vAddr.clear().
const char* msg_type;
int make_flags;
if (pto->m_wants_addrv2) {
msg_type = NetMsgType::ADDRV2;
make_flags = ADDRV2_FORMAT;
} else {
msg_type = NetMsgType::ADDR;
make_flags = 0;
}
const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION));
vector<CAddress> vAddr;
vAddr.reserve(pto->vAddrToSend.size());
BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
@@ -8343,29 +8361,17 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
if (pto->AddAddressIfNotAlreadyKnown(addr))
{
vAddr.push_back(addr);
if (vAddr.size() >= MAX_ADDR_TO_SEND)
{
// Should be impossible since we always check size before adding to
// vAddrToSend. Recover by trimming the vector.
vAddr.resize(MAX_ADDR_TO_SEND);
pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
vAddr.clear();
}
const char* msg_type;
int make_flags;
if (pto->m_wants_addrv2) {
msg_type = NetMsgType::ADDRV2;
make_flags = ADDRV2_FORMAT;
} else {
msg_type = NetMsgType::ADDR;
make_flags = 0;
}
pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend));
}
}
pto->vAddrToSend.clear();
vAddr.clear();
if (!vAddr.empty())
pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
}
CNodeState &state = *State(pto->GetId());

20
src/wallet/wallet.cpp
View File

@@ -1240,6 +1240,16 @@ int CWallet::SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessH
return nMinimumHeight;
}
int CWallet::SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight)
{
// No nullifier => an incoming-viewing-key-only note (z_importviewingkey without the full
// viewing key). Spend depth is unknowable, so treat it as unspent and keep its witness.
if (!nullifier) {
return min(nWitnessHeight, nMinimumHeight);
}
return SaplingWitnessMinimumHeight(*nullifier, nWitnessHeight, nMinimumHeight);
}
int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessOnly)
{
LOCK2(cs_main, cs_wallet);
@@ -1277,7 +1287,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
//Skip Validation when witness root has been validated
if (nd->witnessRootValidated) {
nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
continue;
}
@@ -1289,12 +1299,12 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
CBlockIndex* whIndex = chainActive[nd->witnessHeight];
if (whIndex == NULL) {
//witnessHeight strictly above the active chain (transient catch-up): cannot validate yet
nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
continue;
}
if (nd->witnesses.front().root() == whIndex->hashFinalSaplingRoot) {
nd->witnessRootValidated = true;
nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
continue;
}
//root mismatch on the active chain -> desynced; fall through to rebuild below
@@ -1306,7 +1316,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
blockRoot = pblockindex->hashFinalSaplingRoot;
if (witnessRoot == blockRoot) {
nd->witnessRootValidated = true;
nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
continue;
}
}
@@ -1358,7 +1368,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
}
nd->witnessHeight = pblockindex->GetHeight();
UpdateSaplingNullifierNoteMapWithTx(wtxItem.second);
nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
}
}
}

6
src/wallet/wallet.h
View File

@@ -896,6 +896,12 @@ public:
protected:
int SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight);
//! Overload for a note whose nullifier may be unset. A note discovered through an imported
//! INCOMING viewing key has no nullifier (computing one needs the full viewing key), so
//! dereferencing the optional aborts the daemon. Treats such a note as unspent, which is the
//! conservative direction: it keeps the witness alive rather than pruning a note we cannot
//! prove spent.
int SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight);
/**
* pindex is the new tip being connected.