Skip to content

Avoid propagating InstantSend related old recovered sigs #3145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 16, 2019
Merged

Avoid propagating InstantSend related old recovered sigs #3145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dff0f96
More/better logging for InstantSend
codablock Oct 9, 2019
45f6fdb
Implement CRecoveredSigsDb::TruncateRecoveredSig
codablock Oct 9, 2019
2528201
Truncate recovered sigs for ISLOCKs instead of completely removing them
codablock Oct 9, 2019
55678ff
Also truncate recovered sigs for freshly received ISLOCKs
codablock Oct 10, 2019
39fc3c5
Fix comment
codablock Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 47 additions & 13 deletions src/llmq/quorums_instantsend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,10 +403,16 @@ bool CInstantSendManager::ProcessTx(const CTransaction& tx, const Consensus::Par
}

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

if (IsConflicted(tx)) {
auto conflictingLock = GetConflictingLock(tx);
if (conflictingLock) {
auto islockHash = ::SerializeHash(*conflictingLock);
LogPrintf("CInstantSendManager::%s -- txid=%s: conflicts with islock %s, txid=%s\n", __func__,
tx.GetHash().ToString(), islockHash.ToString(), conflictingLock->txid.ToString());
return false;
}

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

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

LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: trying to vote on %d inputs\n", __func__,
tx.GetHash().ToString(), tx.vin.size());

for (size_t i = 0; i < tx.vin.size(); i++) {
auto& in = tx.vin[i];
auto& id = ids[i];
inputRequestIds.emplace(id);
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: trying to vote on input %s with id %s\n", __func__,
tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString());
if (quorumSigningManager->AsyncSignIfMember(llmqType, id, tx.GetHash())) {
LogPrintf("CInstantSendManager::%s -- txid=%s: voted on input %s with id %s\n", __func__,
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s: voted on input %s with id %s\n", __func__,
tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString());
}
}
Expand Down Expand Up @@ -915,6 +930,10 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has

// This will also add children TXs to pendingRetryTxs
RemoveNonLockedTx(islock.txid, true);

// We don't need the recovered sigs for the inputs anymore. This prevents unnecessary propagation of these sigs.
// We only need the ISLOCK from now on to detect conflicts
TruncateRecoveredSigsForInputs(islock);
}

CInv inv(MSG_ISLOCK, hash);
Expand Down Expand Up @@ -1036,6 +1055,9 @@ void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx, const CBlock
nonLockedTxsByInputs.emplace(in.prevout.hash, std::make_pair(in.prevout.n, tx->GetHash()));
}
}

LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, pindexMined=%s\n", __func__,
tx->GetHash().ToString(), pindexMined ? pindexMined->GetBlockHash().ToString() : "");
}

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

size_t retryChildrenCount = 0;
if (retryChildren) {
// TX got locked, so we can retry locking children
for (auto& childTxid : info.children) {
pendingRetryTxs.emplace(childTxid);
retryChildrenCount++;
}
}

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

nonLockedTxs.erase(it);

LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, retryChildren=%d, retryChildrenCount=%d\n", __func__,
txid.ToString(), retryChildren, retryChildrenCount);
}

void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx)
Expand All @@ -1091,6 +1118,17 @@ void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx)
}
}

void CInstantSendManager::TruncateRecoveredSigsForInputs(const llmq::CInstantSendLock& islock)
{
auto& consensusParams = Params().GetConsensus();

for (auto& in : islock.inputs) {
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
inputRequestIds.erase(inputRequestId);
quorumSigningManager->TruncateRecoveredSig(consensusParams.llmqTypeInstantSend, inputRequestId);
}
}

void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindexChainLock)
{
HandleFullyConfirmedBlock(pindexChainLock);
Expand Down Expand Up @@ -1131,17 +1169,13 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex)
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: removed islock as it got fully confirmed\n", __func__,
islock->txid.ToString(), islockHash.ToString());

for (auto& in : islock->inputs) {
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
inputRequestIds.erase(inputRequestId);

// no need to keep recovered sigs for fully confirmed IS locks, as there is no chance for conflicts
// from now on. All inputs are spent now and can't be spend in any other TX.
quorumSigningManager->RemoveRecoveredSig(consensusParams.llmqTypeInstantSend, inputRequestId);
}
// No need to keep recovered sigs for fully confirmed IS locks, as there is no chance for conflicts
// from now on. All inputs are spent now and can't be spend in any other TX.
TruncateRecoveredSigsForInputs(*islock);

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

// Find all previously unlocked TXs that got locked by this fully confirmed (ChainLock) block and remove them
Expand Down
1 change: 1 addition & 0 deletions src/llmq/quorums_instantsend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ class CInstantSendManager : public CRecoveredSigsListener
void AddNonLockedTx(const CTransactionRef& tx, const CBlockIndex* pindexMined);
void RemoveNonLockedTx(const uint256& txid, bool retryChildren);
void RemoveConflictedTx(const CTransaction& tx);
void TruncateRecoveredSigsForInputs(const CInstantSendLock& islock);

void NotifyChainLock(const CBlockIndex* pindexChainLock);
void UpdatedBlockTip(const CBlockIndex* pindexNew);
Expand Down
36 changes: 28 additions & 8 deletions src/llmq/quorums_signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
}
}

void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey)
void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, bool deleteTimeKey)
{
AssertLockHeld(cs);

Expand All @@ -276,7 +276,9 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l
auto k4 = std::make_tuple(std::string("rs_s"), signHash);
batch.Erase(k1);
batch.Erase(k2);
batch.Erase(k3);
if (deleteHashKey) {
batch.Erase(k3);
}
batch.Erase(k4);

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

hasSigForIdCache.erase(std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.id));
hasSigForSessionCache.erase(signHash);
hasSigForHashCache.erase(recSig.GetHash());
if (deleteHashKey) {
hasSigForHashCache.erase(recSig.GetHash());
}
}

// Completely remove any traces of the recovered sig
void CRecoveredSigsDb::RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
{
LOCK(cs);
CDBBatch batch(db);
RemoveRecoveredSig(batch, llmqType, id, true);
RemoveRecoveredSig(batch, llmqType, id, true, true);
db.WriteBatch(batch);
}

// Remove the recovered sig itself and all keys required to get from id -> recSig
// This will leave the byHash key in-place so that HasRecoveredSigForHash still returns true
void CRecoveredSigsDb::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
{
LOCK(cs);
CDBBatch batch(db);
RemoveRecoveredSig(batch, llmqType, id, false, false);
db.WriteBatch(batch);
}

Expand Down Expand Up @@ -339,7 +354,7 @@ void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge)
{
LOCK(cs);
for (auto& e : toDelete) {
RemoveRecoveredSig(batch, e.first, e.second, false);
RemoveRecoveredSig(batch, e.first, e.second, true, false);

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

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

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

if (db.HasRecoveredSigForHash(recoveredSig.GetHash())) {
return;
}

std::vector<CRecoveredSigsListener*> listeners;
{
LOCK(cs);
Expand Down Expand Up @@ -709,9 +729,9 @@ void CSigningManager::PushReconstructedRecoveredSig(const llmq::CRecoveredSig& r
pendingReconstructedRecoveredSigs.emplace_back(recoveredSig, quorum);
}

void CSigningManager::RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
void CSigningManager::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
{
db.RemoveRecoveredSig(llmqType, id);
db.TruncateRecoveredSig(llmqType, id);
}

void CSigningManager::Cleanup()
Expand Down
7 changes: 5 additions & 2 deletions src/llmq/quorums_signing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class CRecoveredSigsDb
bool GetRecoveredSigById(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret);
void WriteRecoveredSig(const CRecoveredSig& recSig);
void RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);

void CleanupOldRecoveredSigs(int64_t maxAge);

Expand All @@ -97,7 +98,7 @@ class CRecoveredSigsDb

private:
bool ReadRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret);
void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey);
void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, bool deleteTimeKey);
};

class CRecoveredSigsListener
Expand Down Expand Up @@ -148,7 +149,9 @@ class CSigningManager

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

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