Skip to content

Commit efd8d2c

Browse files
codablockUdjinM6
codablock
authored and
UdjinM6
committed
Avoid propagating InstantSend related old recovered sigs (#3145)
* More/better logging for InstantSend * Implement CRecoveredSigsDb::TruncateRecoveredSig * Truncate recovered sigs for ISLOCKs instead of completely removing them This makes AlreadyHave() return true even when the recovered sig is deleted locally. This avoids re-requesting and re-processing of old recovered sigs. * Also truncate recovered sigs for freshly received ISLOCKs * Fix comment
1 parent 24fee30 commit efd8d2c

File tree

4 files changed

+81
-23
lines changed

4 files changed

+81
-23
lines changed

‎src/llmq/quorums_instantsend.cpp‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -403,10 +403,16 @@ bool CInstantSendManager::ProcessTx(const CTransaction& tx, const Consensus::Par
403403
}
404404

405405
if (!CheckCanLock(tx, true, params)) {
406+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: CheckCanLock returned false\n", __func__,
407+
tx.GetHash().ToString());
406408
return false;
407409
}
408410

409-
if (IsConflicted(tx)) {
411+
auto conflictingLock = GetConflictingLock(tx);
412+
if (conflictingLock) {
413+
auto islockHash = ::SerializeHash(*conflictingLock);
414+
LogPrintf("CInstantSendManager::%s -- txid=%s: conflicts with islock %s, txid=%s\n", __func__,
415+
tx.GetHash().ToString(), islockHash.ToString(), conflictingLock->txid.ToString());
410416
return false;
411417
}
412418

@@ -421,7 +427,7 @@ bool CInstantSendManager::ProcessTx(const CTransaction& tx, const Consensus::Par
421427
uint256 otherTxHash;
422428
if (quorumSigningManager->GetVoteForId(llmqType, id, otherTxHash)) {
423429
if (otherTxHash != tx.GetHash()) {
424-
LogPrintf("CInstantSendManager::%s -- txid=%s: input %s is conflicting with islock %s\n", __func__,
430+
LogPrintf("CInstantSendManager::%s -- txid=%s: input %s is conflicting with previous vote for tx %s\n", __func__,
425431
tx.GetHash().ToString(), in.prevout.ToStringShort(), otherTxHash.ToString());
426432
return false;
427433
}
@@ -430,19 +436,28 @@ bool CInstantSendManager::ProcessTx(const CTransaction& tx, const Consensus::Par
430436

431437
// don't even try the actual signing if any input is conflicting
432438
if (quorumSigningManager->IsConflicting(llmqType, id, tx.GetHash())) {
439+
LogPrintf("CInstantSendManager::%s -- txid=%s: quorumSigningManager->IsConflicting returned true. id=%s\n", __func__,
440+
tx.GetHash().ToString(), id.ToString());
433441
return false;
434442
}
435443
}
436444
if (alreadyVotedCount == ids.size()) {
445+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: already voted on all inputs, bailing out\n", __func__,
446+
tx.GetHash().ToString());
437447
return true;
438448
}
439449

450+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: trying to vote on %d inputs\n", __func__,
451+
tx.GetHash().ToString(), tx.vin.size());
452+
440453
for (size_t i = 0; i < tx.vin.size(); i++) {
441454
auto& in = tx.vin[i];
442455
auto& id = ids[i];
443456
inputRequestIds.emplace(id);
457+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: trying to vote on input %s with id %s\n", __func__,
458+
tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString());
444459
if (quorumSigningManager->AsyncSignIfMember(llmqType, id, tx.GetHash())) {
445-
LogPrintf("CInstantSendManager::%s -- txid=%s: voted on input %s with id %s\n", __func__,
460+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: voted on input %s with id %s\n", __func__,
446461
tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString());
447462
}
448463
}
@@ -915,6 +930,10 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has
915930

916931
// This will also add children TXs to pendingRetryTxs
917932
RemoveNonLockedTx(islock.txid, true);
933+
934+
// We don't need the recovered sigs for the inputs anymore. This prevents unnecessary propagation of these sigs.
935+
// We only need the ISLOCK from now on to detect conflicts
936+
TruncateRecoveredSigsForInputs(islock);
918937
}
919938

920939
CInv inv(MSG_ISLOCK, hash);
@@ -1036,6 +1055,9 @@ void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx, const CBlock
10361055
nonLockedTxsByInputs.emplace(in.prevout.hash, std::make_pair(in.prevout.n, tx->GetHash()));
10371056
}
10381057
}
1058+
1059+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, pindexMined=%s\n", __func__,
1060+
tx->GetHash().ToString(), pindexMined ? pindexMined->GetBlockHash().ToString() : "");
10391061
}
10401062

10411063
void CInstantSendManager::RemoveNonLockedTx(const uint256& txid, bool retryChildren)
@@ -1048,10 +1070,12 @@ void CInstantSendManager::RemoveNonLockedTx(const uint256& txid, bool retryChild
10481070
}
10491071
auto& info = it->second;
10501072

1073+
size_t retryChildrenCount = 0;
10511074
if (retryChildren) {
10521075
// TX got locked, so we can retry locking children
10531076
for (auto& childTxid : info.children) {
10541077
pendingRetryTxs.emplace(childTxid);
1078+
retryChildrenCount++;
10551079
}
10561080
}
10571081

@@ -1078,6 +1102,9 @@ void CInstantSendManager::RemoveNonLockedTx(const uint256& txid, bool retryChild
10781102
}
10791103

10801104
nonLockedTxs.erase(it);
1105+
1106+
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, retryChildren=%d, retryChildrenCount=%d\n", __func__,
1107+
txid.ToString(), retryChildren, retryChildrenCount);
10811108
}
10821109

10831110
void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx)
@@ -1091,6 +1118,17 @@ void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx)
10911118
}
10921119
}
10931120

1121+
void CInstantSendManager::TruncateRecoveredSigsForInputs(const llmq::CInstantSendLock& islock)
1122+
{
1123+
auto& consensusParams = Params().GetConsensus();
1124+
1125+
for (auto& in : islock.inputs) {
1126+
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
1127+
inputRequestIds.erase(inputRequestId);
1128+
quorumSigningManager->TruncateRecoveredSig(consensusParams.llmqTypeInstantSend, inputRequestId);
1129+
}
1130+
}
1131+
10941132
void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindexChainLock)
10951133
{
10961134
HandleFullyConfirmedBlock(pindexChainLock);
@@ -1131,17 +1169,13 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex)
11311169
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: removed islock as it got fully confirmed\n", __func__,
11321170
islock->txid.ToString(), islockHash.ToString());
11331171

1134-
for (auto& in : islock->inputs) {
1135-
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
1136-
inputRequestIds.erase(inputRequestId);
1137-
1138-
// no need to keep recovered sigs for fully confirmed IS locks, as there is no chance for conflicts
1139-
// from now on. All inputs are spent now and can't be spend in any other TX.
1140-
quorumSigningManager->RemoveRecoveredSig(consensusParams.llmqTypeInstantSend, inputRequestId);
1141-
}
1172+
// No need to keep recovered sigs for fully confirmed IS locks, as there is no chance for conflicts
1173+
// from now on. All inputs are spent now and can't be spend in any other TX.
1174+
TruncateRecoveredSigsForInputs(*islock);
11421175

1143-
// same as in the loop
1144-
quorumSigningManager->RemoveRecoveredSig(consensusParams.llmqTypeInstantSend, islock->GetRequestId());
1176+
// And we don't need the recovered sig for the ISLOCK anymore, as the block in which it got mined is considered
1177+
// fully confirmed now
1178+
quorumSigningManager->TruncateRecoveredSig(consensusParams.llmqTypeInstantSend, islock->GetRequestId());
11451179
}
11461180

11471181
// Find all previously unlocked TXs that got locked by this fully confirmed (ChainLock) block and remove them

‎src/llmq/quorums_instantsend.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ class CInstantSendManager : public CRecoveredSigsListener
149149
void AddNonLockedTx(const CTransactionRef& tx, const CBlockIndex* pindexMined);
150150
void RemoveNonLockedTx(const uint256& txid, bool retryChildren);
151151
void RemoveConflictedTx(const CTransaction& tx);
152+
void TruncateRecoveredSigsForInputs(const CInstantSendLock& islock);
152153

153154
void NotifyChainLock(const CBlockIndex* pindexChainLock);
154155
void UpdatedBlockTip(const CBlockIndex* pindexNew);

‎src/llmq/quorums_signing.cpp‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
259259
}
260260
}
261261

262-
void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey)
262+
void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, bool deleteTimeKey)
263263
{
264264
AssertLockHeld(cs);
265265

@@ -276,7 +276,9 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l
276276
auto k4 = std::make_tuple(std::string("rs_s"), signHash);
277277
batch.Erase(k1);
278278
batch.Erase(k2);
279-
batch.Erase(k3);
279+
if (deleteHashKey) {
280+
batch.Erase(k3);
281+
}
280282
batch.Erase(k4);
281283

282284
if (deleteTimeKey) {
@@ -292,14 +294,27 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l
292294

293295
hasSigForIdCache.erase(std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.id));
294296
hasSigForSessionCache.erase(signHash);
295-
hasSigForHashCache.erase(recSig.GetHash());
297+
if (deleteHashKey) {
298+
hasSigForHashCache.erase(recSig.GetHash());
299+
}
296300
}
297301

302+
// Completely remove any traces of the recovered sig
298303
void CRecoveredSigsDb::RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
299304
{
300305
LOCK(cs);
301306
CDBBatch batch(db);
302-
RemoveRecoveredSig(batch, llmqType, id, true);
307+
RemoveRecoveredSig(batch, llmqType, id, true, true);
308+
db.WriteBatch(batch);
309+
}
310+
311+
// Remove the recovered sig itself and all keys required to get from id -> recSig
312+
// This will leave the byHash key in-place so that HasRecoveredSigForHash still returns true
313+
void CRecoveredSigsDb::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
314+
{
315+
LOCK(cs);
316+
CDBBatch batch(db);
317+
RemoveRecoveredSig(batch, llmqType, id, false, false);
303318
db.WriteBatch(batch);
304319
}
305320

@@ -339,7 +354,7 @@ void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge)
339354
{
340355
LOCK(cs);
341356
for (auto& e : toDelete) {
342-
RemoveRecoveredSig(batch, e.first, e.second, false);
357+
RemoveRecoveredSig(batch, e.first, e.second, true, false);
343358

344359
if (batch.SizeEstimate() >= (1 << 24)) {
345360
db.WriteBatch(batch);
@@ -473,7 +488,8 @@ void CSigningManager::ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredS
473488
return;
474489
}
475490

476-
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- signHash=%s, node=%d\n", __func__, CLLMQUtils::BuildSignHash(recoveredSig).ToString(), pfrom->GetId());
491+
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__,
492+
CLLMQUtils::BuildSignHash(recoveredSig).ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), pfrom->GetId());
477493

478494
LOCK(cs);
479495
pendingRecoveredSigs[pfrom->GetId()].emplace_back(recoveredSig);
@@ -656,6 +672,10 @@ void CSigningManager::ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& re
656672
connman.RemoveAskFor(recoveredSig.GetHash());
657673
}
658674

675+
if (db.HasRecoveredSigForHash(recoveredSig.GetHash())) {
676+
return;
677+
}
678+
659679
std::vector<CRecoveredSigsListener*> listeners;
660680
{
661681
LOCK(cs);
@@ -709,9 +729,9 @@ void CSigningManager::PushReconstructedRecoveredSig(const llmq::CRecoveredSig& r
709729
pendingReconstructedRecoveredSigs.emplace_back(recoveredSig, quorum);
710730
}
711731

712-
void CSigningManager::RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
732+
void CSigningManager::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
713733
{
714-
db.RemoveRecoveredSig(llmqType, id);
734+
db.TruncateRecoveredSig(llmqType, id);
715735
}
716736

717737
void CSigningManager::Cleanup()

‎src/llmq/quorums_signing.h‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ class CRecoveredSigsDb
8585
bool GetRecoveredSigById(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret);
8686
void WriteRecoveredSig(const CRecoveredSig& recSig);
8787
void RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
88+
void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
8889

8990
void CleanupOldRecoveredSigs(int64_t maxAge);
9091

@@ -97,7 +98,7 @@ class CRecoveredSigsDb
9798

9899
private:
99100
bool ReadRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret);
100-
void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey);
101+
void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, bool deleteTimeKey);
101102
};
102103

103104
class CRecoveredSigsListener
@@ -148,7 +149,9 @@ class CSigningManager
148149

149150
// This is called when a recovered signature can be safely removed from the DB. This is only safe when some other
150151
// mechanism prevents possible conflicts. As an example, ChainLocks prevent conflicts in confirmed TXs InstantSend votes
151-
void RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
152+
// This won't completely remove all traces of the recovered sig but instead leave the hash entry in the DB. This
153+
// allows AlreadyHave to keep returning true. Cleanup will later remove the remains
154+
void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
152155

153156
private:
154157
void ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredSig& recoveredSig, CConnman& connman);

0 commit comments

Comments
 (0)