Skip to content

fix!: efficient build mnlistdiffs in rotation info #6587

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

Closed
wants to merge 9 commits into from
Closed

fix!: efficient build mnlistdiffs in rotation info #6587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
02c47fd
fix(perf): build mnlistdiffs in rotation info using dynamically the h…
ogabrielides Feb 18, 2025
2d2e3f9
bump guix
knst Feb 18, 2025
a8a4b27
fix: move BuildSimplifiedMNListDiff for block h
UdjinM6 Feb 18, 2025
f1e3af1
fix: don't use `baseBlockIndexes.back()` for the tip
UdjinM6 Feb 18, 2025
08f6f20
fix: sort indexes in GetLastBaseBlockHash
UdjinM6 Feb 18, 2025
03814d7
refactor: clang-format
UdjinM6 Feb 18, 2025
542ac80
refactor: more clang-format
UdjinM6 Feb 19, 2025
5402147
feat: only use efficient qrinfo construction for new shared protocol …
PastaPastaPasta Mar 25, 2025
0fba0c5
fmt: apply clang-format suggestions
PastaPastaPasta Mar 25, 2025
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
119 changes: 96 additions & 23 deletions src/llmq/snapshot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ UniValue CQuorumRotationInfo::ToJson() const
bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
const ChainstateManager& chainman, const CQuorumManager& qman,
const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request,
CQuorumRotationInfo& response, std::string& errorRet)
bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet)
{
AssertLockHeld(cs_main);

Expand All @@ -112,19 +112,23 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
}
baseBlockIndexes.push_back(blockIndex);
}
std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(), [](const CBlockIndex* a, const CBlockIndex* b) {
return a->nHeight < b->nHeight;
});
if (use_legacy_construction) {
std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
[](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });
}
}

const CBlockIndex* tipBlockIndex = chainman.ActiveChain().Tip();
if (!tipBlockIndex) {
errorRet = strprintf("tip block not found");
return false;
}
//Build MN list Diff always with highest baseblock
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, baseBlockIndexes.back()->GetBlockHash(), tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) {
return false;
if (use_legacy_construction) {
// Build MN list Diff always with highest baseblock
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, baseBlockIndexes.back()->GetBlockHash(),
tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) {
return false;
}
}

const CBlockIndex* blockIndex = chainman.m_blockman.LookupBlockIndex(request.blockRequestHash);
Expand All @@ -149,15 +153,19 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
return false;
}

const CBlockIndex* pWorkBlockIndex = hBlockIndex->GetAncestor(hBlockIndex->nHeight - workDiff);
if (!pWorkBlockIndex) {
const CBlockIndex* pWorkBlockHIndex = hBlockIndex->GetAncestor(hBlockIndex->nHeight - workDiff);
if (!pWorkBlockHIndex) {
errorRet = strprintf("Can not find work block H");
return false;
}

//Build MN list Diff always with highest baseblock
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockIndex), pWorkBlockIndex->GetBlockHash(), response.mnListDiffH, errorRet)) {
return false;
if (use_legacy_construction) {
// Build MN list Diff always with highest baseblock
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHIndex, use_legacy_construction),
pWorkBlockHIndex->GetBlockHash(), response.mnListDiffH, errorRet)) {
return false;
}
}

const CBlockIndex* pBlockHMinusCIndex = tipBlockIndex->GetAncestor(hBlockIndex->nHeight - cycleLength);
Expand Down Expand Up @@ -202,8 +210,12 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
const CBlockIndex* pWorkBlockHMinus4CIndex = pBlockHMinus4CIndex->GetAncestor(pBlockHMinus4CIndex->nHeight - workDiff);
//Checked later if extraShare is on

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex), pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) {
return false;
if (use_legacy_construction) {
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex, use_legacy_construction),
pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) {
return false;
}
}

auto snapshotHMinusC = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinusCIndex);
Expand All @@ -214,8 +226,13 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
response.quorumSnapshotAtHMinusC = std::move(snapshotHMinusC.value());
}

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex), pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) {
return false;
if (use_legacy_construction) {
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex,
use_legacy_construction),
pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) {
return false;
}
}

auto snapshotHMinus2C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus2CIndex);
Expand All @@ -226,8 +243,13 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
response.quorumSnapshotAtHMinus2C = std::move(snapshotHMinus2C.value());
}

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex), pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) {
return false;
if (use_legacy_construction) {
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex,
use_legacy_construction),
pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) {
return false;
}
}

auto snapshotHMinus3C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus3CIndex);
Expand Down Expand Up @@ -255,10 +277,15 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
}

CSimplifiedMNListDiff mn4c;
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus4CIndex), pWorkBlockHMinus4CIndex->GetBlockHash(), mn4c, errorRet)) {
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus4CIndex,
use_legacy_construction),
pWorkBlockHMinus4CIndex->GetBlockHash(), mn4c, errorRet)) {
return false;
}

if (!use_legacy_construction) {
baseBlockIndexes.push_back(pWorkBlockHMinus4CIndex);
}
response.mnListDiffAtHMinus4C = std::move(mn4c);
} else {
response.extraShare = false;
Expand Down Expand Up @@ -311,19 +338,65 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
}

CSimplifiedMNListDiff mnhneeded;
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pNeededWorkBlockIndex), pNeededWorkBlockIndex->GetBlockHash(), mnhneeded, errorRet)) {
if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pNeededWorkBlockIndex, use_legacy_construction),
pNeededWorkBlockIndex->GetBlockHash(), mnhneeded, errorRet)) {
return false;
}

if (!use_legacy_construction) {
baseBlockIndexes.push_back(pNeededWorkBlockIndex);
}
response.mnListDiffList.push_back(mnhneeded);
}

if (!use_legacy_construction) {
Copy link
Collaborator

@knst knst Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for simplicity better to invert this condition? if legacy -> return true.

shorter diff, easier to read, and review.

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex,
use_legacy_construction),
pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) {
return false;
}
baseBlockIndexes.push_back(pWorkBlockHMinus3CIndex);

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex,
use_legacy_construction),
pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) {
return false;
}
baseBlockIndexes.push_back(pWorkBlockHMinus2CIndex);

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex, use_legacy_construction),
pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) {
return false;
}
baseBlockIndexes.push_back(pWorkBlockHMinusCIndex);

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHIndex, use_legacy_construction),
pWorkBlockHIndex->GetBlockHash(), response.mnListDiffH, errorRet)) {
return false;
}
baseBlockIndexes.push_back(pWorkBlockHIndex);

if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
GetLastBaseBlockHash(baseBlockIndexes, tipBlockIndex, use_legacy_construction),
tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) {
return false;
}
}
Comment on lines +352 to +388
Copy link

@coderabbitai coderabbitai bot Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Check for duplication between legacy and non-legacy paths.
These lines add a large else-branch to handle non-legacy mode, effectively re-building diffs in ascending block order. Much of it appears to replicate the logic also present in the legacy approach. Removing or unifying shared steps can simplify future maintenance.

return true;
}

uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex)
uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
bool use_legacy_construction)
{
uint256 hash;
if (!use_legacy_construction) {
std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
[](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });
}
for (const auto baseBlock : baseBlockIndexes) {
if (baseBlock->nHeight >= blockIndex->nHeight)
break;
Expand Down
6 changes: 4 additions & 2 deletions src/llmq/snapshot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,10 @@ class CQuorumRotationInfo
bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
const ChainstateManager& chainman, const CQuorumManager& qman,
const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request,
CQuorumRotationInfo& response, std::string& errorRet) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex);
bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet)
EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
bool use_legacy_construction);

class CQuorumSnapshotManager
{
Expand Down
3 changes: 2 additions & 1 deletion src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5200,7 +5200,8 @@ void PeerManagerImpl::ProcessMessage(

llmq::CQuorumRotationInfo quorumRotationInfoRet;
std::string strError;
if (BuildQuorumRotationInfo(*m_dmnman, *m_llmq_ctx->qsnapman, m_chainman, *m_llmq_ctx->qman, *m_llmq_ctx->quorum_block_processor, cmd, quorumRotationInfoRet, strError)) {
bool use_leagcy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;;
Copy link
Collaborator

@knst knst Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/use_leagcy_construction/use_legacy_construction/

if (BuildQuorumRotationInfo(*m_dmnman, *m_llmq_ctx->qsnapman, m_chainman, *m_llmq_ctx->qman, *m_llmq_ctx->quorum_block_processor, cmd, use_leagcy_construction, quorumRotationInfoRet, strError)) {
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QUORUMROTATIONINFO, quorumRotationInfoRet));
} else {
strError = strprintf("getquorumrotationinfo failed for size(baseBlockHashes)=%d, blockRequestHash=%s. error=%s", cmd.baseBlockHashes.size(), cmd.blockRequestHash.ToString(), strError);
Expand Down
2 changes: 1 addition & 1 deletion src/rpc/quorums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ static RPCHelpMan quorum_rotationinfo()
LOCK(cs_main);

if (!BuildQuorumRotationInfo(*CHECK_NONFATAL(node.dmnman), *llmq_ctx.qsnapman, chainman, *llmq_ctx.qman,
*llmq_ctx.quorum_block_processor, cmd, quorumRotationInfoRet, strError)) {
*llmq_ctx.quorum_block_processor, cmd, false, quorumRotationInfoRet, strError)) {
throw JSONRPCError(RPC_INVALID_REQUEST, strError);
}

Expand Down
3 changes: 3 additions & 0 deletions src/version.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ static const int DSQ_INV_VERSION = 70234;
//! Maximum header count for HEADRES2 message was increased from 2000 to 8000 in this version
static const int INCREASE_MAX_HEADERS2_VERSION = 70235;

//! Behavior of QRINFO is changed in this protocol version
static const int EFFICIENT_QRINFO_VERSION = 70236;
Copy link
Collaborator

@knst knst Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should update PROTOCOL_VERSION also

-static const int PROTOCOL_VERSION = 70235;
+static const int PROTOCOL_VERSION = 70236;

Copy link

@UdjinM6 UdjinM6 Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and P2P_VERSION in p2p.py


// Make sure that none of the values above collide with `ADDRV2_FORMAT`.

#endif // BITCOIN_VERSION_H
Loading