Skip to content

fix: efficient build mnlistdiffs in rotation info #6622

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 11 commits into from
Apr 3, 2025
Merged

fix: efficient build mnlistdiffs in rotation info #6622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e8bbfd2
fix(perf): build mnlistdiffs in rotation info using dynamically the h…
ogabrielides Feb 18, 2025
2a12ff6
bump guix
knst Feb 18, 2025
ec78465
fix: move BuildSimplifiedMNListDiff for block h
UdjinM6 Feb 18, 2025
810ecd8
fix: don't use `baseBlockIndexes.back()` for the tip
UdjinM6 Feb 18, 2025
1f16cf4
fix: sort indexes in GetLastBaseBlockHash
UdjinM6 Feb 18, 2025
ff16e68
refactor: clang-format
UdjinM6 Feb 18, 2025
71112dc
refactor: more clang-format
UdjinM6 Feb 19, 2025
bdfd597
feat: only use efficient qrinfo construction for new shared protocol …
PastaPastaPasta Mar 25, 2025
7432879
fmt: apply clang-format suggestions
PastaPastaPasta Mar 25, 2025
606719a
nit: s/use_leagcy_construction/use_legacy_construction/
PastaPastaPasta Mar 31, 2025
03c15a3
fix: actually bump protocol version
PastaPastaPasta Apr 2, 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)
Comment on lines 87 to +90
Copy link

@coderabbitai coderabbitai bot Apr 1, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Ensure this API change is documented in the project

The function signature has been updated to include the new parameter bool use_legacy_construction. This is a significant change that affects how quorum rotation information is built. Make sure this change is properly documented and that callers of this function are updated accordingly.

🏁 Script executed:

#!/bin/bash
# Check all callers of BuildQuorumRotationInfo to ensure they've been updated
rg "BuildQuorumRotationInfo\s*\(" --type cpp -A 3 -B 3

Length of output: 2565

API Change: Update Documentation for New Parameter

The updated function signature in src/llmq/snapshot.cpp and its corresponding header now includes the new parameter bool use_legacy_construction. Verification shows that the callers in both src/net_processing.cpp and src/rpc/quorums.cpp have been updated accordingly.

Please ensure that this significant API change is also reflected in the project documentation (e.g., API docs, changelog, or README) so that downstream users are aware of the new behavior.

{
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) {
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;
}
}
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; });
}
Comment on lines +396 to +399
Copy link

@coderabbitai coderabbitai bot Apr 1, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Unify sorting strategy.
Currently, we sort here only if !use_legacy_construction, while lines 115-118 sort only if use_legacy_construction. Evaluate whether it’s possible or preferable to sort unconditionally at a single point to reduce confusion.

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 @@ -5203,7 +5203,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_legacy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;;
if (BuildQuorumRotationInfo(*m_dmnman, *m_llmq_ctx->qsnapman, m_chainman, *m_llmq_ctx->qman, *m_llmq_ctx->quorum_block_processor, cmd, use_legacy_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
5 changes: 4 additions & 1 deletion src/version.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/


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

//! initial proto version, to be increased after version/verack negotiation
static const int INIT_PROTO_VERSION = 209;
Expand Down 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;

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

#endif // BITCOIN_VERSION_H
2 changes: 1 addition & 1 deletion test/functional/test_framework/p2p.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
MIN_P2P_VERSION_SUPPORTED = 60001
# The P2P version that this test framework implements and sends in its `version` message
# Version 70235 increased max header count for HEADERS2 message from 2000 to 8000
P2P_VERSION = 70235
P2P_VERSION = 70236
# The services that this test framework offers in its `version` message
P2P_SERVICES = NODE_NETWORK | NODE_HEADERS_COMPRESSED
# The P2P user agent string that this test framework sends in its `version` message
Expand Down
Loading