Skip to content

Commit c08e761

Browse files
UdjinM6UdjinM6
authored
Tighten rules for DSVIN/DSTX (#2897)
* Tighten rules for dstx * Tighten rules for dsvin * NULL -> nullptr * Make `ConsumeCollateral()` a private function instead of a lamda and reuse it in `Charge*Fees()` * Make sure inputs and outputs are of the same size Introduces new response ERR_SIZE_MISMATCH, old clients will simply bail out. * Drop now redundant vecTxOut.size() check * Check max inputs size * Fix log category
1 parent 29194b1 commit c08e761

File tree

5 files changed

+68
-25
lines changed

5 files changed

+68
-25
lines changed

‎src/net_processing.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2156,6 +2156,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
21562156
}
21572157
} else if (nInvType == MSG_DSTX) {
21582158
uint256 hashTx = tx.GetHash();
2159+
if (!dstx.IsValidStructure()) {
2160+
LogPrint(BCLog::PRIVATESEND, "DSTX -- Invalid DSTX structure: %s\n", hashTx.ToString());
2161+
return false;
2162+
}
21592163
if(CPrivateSend::GetDSTX(hashTx)) {
21602164
LogPrint(BCLog::PRIVATESEND, "DSTX -- Already have %s, skipping...\n", hashTx.ToString());
21612165
return true; // not an error

‎src/privatesend/privatesend-server.cpp‎

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -171,15 +171,17 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm
171171

172172
LogPrint(BCLog::PRIVATESEND, "DSVIN -- txCollateral %s", entry.txCollateral->ToString());
173173

174-
if (entry.vecTxDSIn.size() > PRIVATESEND_ENTRY_MAX_SIZE) {
175-
LogPrintf("DSVIN -- ERROR: too many inputs! %d/%d\n", entry.vecTxDSIn.size(), PRIVATESEND_ENTRY_MAX_SIZE);
176-
PushStatus(pfrom, STATUS_REJECTED, ERR_MAXIMUM, connman);
174+
if (entry.vecTxDSIn.size() != entry.vecTxOut.size()) {
175+
LogPrintf("DSVIN -- ERROR: inputs vs outputs size mismatch! %d vs %d\n", entry.vecTxDSIn.size(), entry.vecTxOut.size());
176+
PushStatus(pfrom, STATUS_REJECTED, ERR_SIZE_MISMATCH, connman);
177+
ConsumeCollateral(connman, entry.txCollateral);
177178
return;
178179
}
179180

180-
if (entry.vecTxOut.size() > PRIVATESEND_ENTRY_MAX_SIZE) {
181-
LogPrintf("DSVIN -- ERROR: too many outputs! %d/%d\n", entry.vecTxOut.size(), PRIVATESEND_ENTRY_MAX_SIZE);
181+
if (entry.vecTxDSIn.size() > PRIVATESEND_ENTRY_MAX_SIZE) {
182+
LogPrintf("DSVIN -- ERROR: too many inputs! %d/%d\n", entry.vecTxDSIn.size(), PRIVATESEND_ENTRY_MAX_SIZE);
182183
PushStatus(pfrom, STATUS_REJECTED, ERR_MAXIMUM, connman);
184+
ConsumeCollateral(connman, entry.txCollateral);
183185
return;
184186
}
185187

@@ -204,11 +206,13 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm
204206
if (txout.scriptPubKey.size() != 25) {
205207
LogPrintf("DSVIN -- non-standard pubkey detected! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey));
206208
PushStatus(pfrom, STATUS_REJECTED, ERR_NON_STANDARD_PUBKEY, connman);
209+
ConsumeCollateral(connman, entry.txCollateral);
207210
return;
208211
}
209212
if (!txout.scriptPubKey.IsPayToPublicKeyHash()) {
210213
LogPrintf("DSVIN -- invalid script! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey));
211214
PushStatus(pfrom, STATUS_REJECTED, ERR_INVALID_SCRIPT, connman);
215+
ConsumeCollateral(connman, entry.txCollateral);
212216
return;
213217
}
214218
}
@@ -226,8 +230,20 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm
226230
PushStatus(pfrom, STATUS_REJECTED, ERR_MISSING_TX, connman);
227231
return;
228232
}
233+
if (!CPrivateSend::IsDenominatedAmount(mempoolTx->vout[txin.prevout.n].nValue)) {
234+
LogPrintf("DSVIN -- non-denominated mempool input! txin=%s\n", txin.ToString());
235+
PushStatus(pfrom, STATUS_REJECTED, ERR_DENOM, connman);
236+
ConsumeCollateral(connman, entry.txCollateral);
237+
return;
238+
}
229239
nValueIn += mempoolTx->vout[txin.prevout.n].nValue;
230240
} else if (GetUTXOCoin(txin.prevout, coin)) {
241+
if (!CPrivateSend::IsDenominatedAmount(coin.out.nValue)) {
242+
LogPrintf("DSVIN -- non-denominated input! txin=%s\n", txin.ToString());
243+
PushStatus(pfrom, STATUS_REJECTED, ERR_DENOM, connman);
244+
ConsumeCollateral(connman, entry.txCollateral);
245+
return;
246+
}
231247
nValueIn += coin.out.nValue;
232248
} else {
233249
LogPrintf("DSVIN -- missing input! txin=%s\n", txin.ToString());
@@ -456,16 +472,7 @@ void CPrivateSendServer::ChargeFees(CConnman& connman)
456472
if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) {
457473
LogPrintf("CPrivateSendServer::ChargeFees -- found uncooperative node (didn't %s transaction), charging fees: %s",
458474
(nState == POOL_STATE_SIGNING) ? "sign" : "send", vecOffendersCollaterals[0]->ToString());
459-
460-
LOCK(cs_main);
461-
462-
CValidationState state;
463-
if (!AcceptToMemoryPool(mempool, state, vecOffendersCollaterals[0], false, NULL, false, maxTxFee)) {
464-
// should never really happen
465-
LogPrintf("CPrivateSendServer::ChargeFees -- ERROR: AcceptToMemoryPool failed!\n");
466-
} else {
467-
connman.RelayTransaction(*vecOffendersCollaterals[0]);
468-
}
475+
ConsumeCollateral(connman, vecOffendersCollaterals[0]);
469476
}
470477
}
471478

@@ -485,19 +492,22 @@ void CPrivateSendServer::ChargeRandomFees(CConnman& connman)
485492
{
486493
if (!fMasternodeMode) return;
487494

488-
LOCK(cs_main);
489-
490495
for (const auto& txCollateral : vecSessionCollaterals) {
491496
if (GetRandInt(100) > 10) return;
492497
LogPrintf("CPrivateSendServer::ChargeRandomFees -- charging random fees, txCollateral=%s", txCollateral->ToString());
498+
ConsumeCollateral(connman, txCollateral);
499+
}
500+
}
493501

494-
CValidationState state;
495-
if (!AcceptToMemoryPool(mempool, state, txCollateral, false, NULL, false, maxTxFee)) {
496-
// Can happen if this collateral belongs to some misbehaving participant we punished earlier
497-
LogPrintf("CPrivateSendServer::ChargeRandomFees -- ERROR: AcceptToMemoryPool failed!\n");
498-
} else {
499-
connman.RelayTransaction(*txCollateral);
500-
}
502+
void CPrivateSendServer::ConsumeCollateral(CConnman& connman, const CTransactionRef& txref)
503+
{
504+
LOCK(cs_main);
505+
CValidationState validationState;
506+
if (!AcceptToMemoryPool(mempool, validationState, txref, false, nullptr)) {
507+
LogPrint(BCLog::PRIVATESEND, "%s -- AcceptToMemoryPool failed\n", __func__);
508+
} else {
509+
connman.RelayTransaction(*txref);
510+
LogPrint(BCLog::PRIVATESEND, "%s -- Collateral was consumed\n", __func__);
501511
}
502512
}
503513

‎src/privatesend/privatesend-server.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ class CPrivateSendServer : public CPrivateSendBaseSession, public CPrivateSendBa
3636
void ChargeFees(CConnman& connman);
3737
/// Rarely charge fees to pay miners
3838
void ChargeRandomFees(CConnman& connman);
39+
/// Consume collateral in cases when peer misbehaved
40+
void ConsumeCollateral(CConnman& connman, const CTransactionRef& txref);
3941

4042
/// Check for process
4143
void CheckPool(CConnman& connman);

‎src/privatesend/privatesend.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,29 @@ bool CPrivateSendBroadcastTx::IsExpired(int nHeight)
122122
return (nConfirmedHeight != -1) && (nHeight - nConfirmedHeight > 24);
123123
}
124124

125+
bool CPrivateSendBroadcastTx::IsValidStructure()
126+
{
127+
// some trivial checks only
128+
if (tx->vin.size() != tx->vout.size()) {
129+
return false;
130+
}
131+
if (tx->vin.size() < CPrivateSend::GetMinPoolParticipants()) {
132+
return false;
133+
}
134+
if (tx->vin.size() > CPrivateSend::GetMaxPoolParticipants() * PRIVATESEND_ENTRY_MAX_SIZE) {
135+
return false;
136+
}
137+
for (const auto& out : tx->vout) {
138+
if (!CPrivateSend::IsDenominatedAmount(out.nValue)) {
139+
return false;
140+
}
141+
if (!out.scriptPubKey.IsPayToPublicKeyHash()) {
142+
return false;
143+
}
144+
}
145+
return true;
146+
}
147+
125148
void CPrivateSendBaseSession::SetNull()
126149
{
127150
// Both sides
@@ -445,6 +468,8 @@ std::string CPrivateSend::GetMessageByID(PoolMessage nMessageID)
445468
return _("Transaction created successfully.");
446469
case MSG_ENTRIES_ADDED:
447470
return _("Your entries added successfully.");
471+
case ERR_SIZE_MISMATCH:
472+
return _("Inputs vs outputs size mismatch.");
448473
default:
449474
return _("Unknown response.");
450475
}

‎src/privatesend/privatesend.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,9 @@ enum PoolMessage {
5252
MSG_NOERR,
5353
MSG_SUCCESS,
5454
MSG_ENTRIES_ADDED,
55+
ERR_SIZE_MISMATCH,
5556
MSG_POOL_MIN = ERR_ALREADY_HAVE,
56-
MSG_POOL_MAX = MSG_ENTRIES_ADDED
57+
MSG_POOL_MAX = ERR_SIZE_MISMATCH
5758
};
5859

5960
// pool states
@@ -310,6 +311,7 @@ class CPrivateSendBroadcastTx
310311

311312
void SetConfirmedHeight(int nConfirmedHeightIn) { nConfirmedHeight = nConfirmedHeightIn; }
312313
bool IsExpired(int nHeight);
314+
bool IsValidStructure();
313315
};
314316

315317
// base class

0 commit comments

Comments
 (0)