Skip to content

perf: keep verified DKG contributions cached through the commit phase #8135

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
PastaPastaPasta merged 2 commits into from
Nov 12, 2026
Merged

perf: keep verified DKG contributions cached through the commit phase #8135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49d9ed5
bench: measure the DKG commit step at llmq_400_85 size
knst Nov 12, 2026
898351d
perf(llmq): keep verified DKG contributions cached for the session
knst Nov 12, 2026
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
71 changes: 69 additions & 2 deletions src/bench/bls_dkg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
#include <bench/bench.h>

#include <bls/bls_worker.h>
#include <clientversion.h>
#include <hash.h>
#include <llmq/options_args.h>
#include <streams.h>
#include <util/helpers.h>

#include <random.h>
Expand Down Expand Up @@ -59,7 +62,12 @@ class DKG
}

public:
explicit DKG(int quorumSize)
explicit DKG(int quorumSize) :
DKG(quorumSize, quorumSize / 2 + 1)
{
}

DKG(int quorumSize, int threshold)
{
members.reserve(quorumSize);
ids.reserve(quorumSize);
Expand All @@ -73,7 +81,7 @@ class DKG

blsWorker.Start(llmq::DEFAULT_WORKER_COUNT);
for (auto& member : members) {
blsWorker.GenerateContributions(quorumSize / 2 + 1, ids, member.vvec, member.skShares);
blsWorker.GenerateContributions(threshold, ids, member.vvec, member.skShares);
}
}
~DKG()
Expand All @@ -90,6 +98,48 @@ class DKG
});
}

// The work of ActiveDKGSession::SendCommitment. A cold run decodes every verification vector from its
// database encoding first, as CDKGSessionManager::GetVerifiedContributions does on a cache miss.
void Bench_BuildCommitment(benchmark::Bench& bench, bool cold)
{
ReceiveVvecs();
ReceiveShares(0);
CDataStream db(SER_DISK, CLIENT_VERSION);
for (const auto& vvec : receivedVvecs) {
db << *vvec;
}

bench.epochs(3).run([&] {
auto vvecs = receivedVvecs;
if (cold) {
CDataStream s{db};
for (auto& vvec : vvecs) {
vvec = std::make_shared<std::vector<CBLSPublicKey>>();
s >> *vvec;
}
}
const auto vvec = blsWorker.BuildQuorumVerificationVector(vvecs);
const auto skShare = blsWorker.AggregateSecretKeys(receivedSkShares);
const auto sig = skShare.Sign(::SerializeHash(*vvec), false);
assert(sig.IsValid());
});
}

// The per-sender work of CDKGSession::ReceiveMessage(CDKGPrematureCommitment) once the vvec is built
void Bench_VerifyCommitment(benchmark::Bench& bench)
{
ReceiveVvecs();
ReceiveShares(0);
const uint256 hash = ::SerializeHash(*quorumVvec);
const auto sig = blsWorker.AggregateSecretKeys(receivedSkShares).Sign(hash, false);

bench.run([&] {
const bool valid = ::SerializeHash(*quorumVvec) == hash &&
sig.VerifyInsecure(CBLSWorker::BuildPubKeyShare(quorumVvec, ids[0]), hash, false);
assert(valid);
});
}

void Bench_VerifyContributionShares(benchmark::Bench& bench, int invalidCount, bool aggregated, uint32_t epoch_iters)
{
ReceiveVvecs();
Expand Down Expand Up @@ -190,6 +240,23 @@ static void BLSDKG_InitDKG(benchmark::Bench& bench, uint32_t epoch_iters, int qu
} \
BENCHMARK(BLSDKG_VerifyContributionShares_##name##_##quorumSize, benchmark::PriorityLevel::HIGH)

// llmq_400_85
static void BLSDKG_BuildCommitment_cold_400(benchmark::Bench& bench)
{
DKG(bench.output() ? 400 : 2, bench.output() ? 340 : 2).Bench_BuildCommitment(bench, /*cold=*/true);
}
static void BLSDKG_BuildCommitment_cached_400(benchmark::Bench& bench)
{
DKG(bench.output() ? 400 : 2, bench.output() ? 340 : 2).Bench_BuildCommitment(bench, /*cold=*/false);
}
static void BLSDKG_VerifyCommitment_400(benchmark::Bench& bench)
{
DKG(bench.output() ? 400 : 2, bench.output() ? 340 : 2).Bench_VerifyCommitment(bench);
}
BENCHMARK(BLSDKG_BuildCommitment_cold_400, benchmark::PriorityLevel::HIGH)
BENCHMARK(BLSDKG_BuildCommitment_cached_400, benchmark::PriorityLevel::HIGH)
BENCHMARK(BLSDKG_VerifyCommitment_400, benchmark::PriorityLevel::HIGH)

BENCH_GenerateContributions(simple, 50, 50);
BENCH_GenerateContributions(simple, 100, 5);

Expand Down
17 changes: 15 additions & 2 deletions src/llmq/dkgsessionmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,22 @@ void CDKGSessionManager::WriteVerifiedVvecContribution(Consensus::LLMQType llmqT
s << CBLSPublicKeyVersionWrapper(pubkey, false);
}
db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), Span{s});

// Reading the vvec back at commit time costs a point decompression and subgroup check per coefficient
LOCK(contributionsCacheCs);
auto& entry = contributionsCache[{llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash}];
entry.entryTime = SteadyClock::now();
entry.vvec = vvec;
Comment on lines +132 to +136

@thepastaclaw thepastaclaw Nov 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 Blocking: Preserve subgroup validation before caching legacy-decoded vectors

This insertion bypasses a validation boundary when a contribution was decoded under the legacy BLS scheme. CDKGContribution::Unserialize permits that scheme, and G1Element::FromBytes explicitly skips CheckValid() for legacy decoding. Although the underlying G1Element::IsValid() checks curve and subgroup membership, CBLSPublicKey::IsValid() is a different method: it returns only the wrapper's fValid flag, which SetBytes sets after legacy decoding without checking subgroup membership. Both PreVerifyMessage's VerifyVerificationVector and BuildQuorumVerificationVector inspect that flag and duplicates, so neither supplies the missing check. Consequently, the new cache can return coefficients that the existing basic-scheme database reader rejects, making commitment reconstruction depend on whether the cache is populated. Legacy operation remains supported, including through the regtest v19 activation-height override. Pass the recorded decoding scheme into the write path and retain the cold-read behavior for legacy-decoded vectors, or perform equivalent validation before caching them.

source: gpt-6.1-sol (phase2-reviewer: general)

@PastaPastaPasta PastaPastaPasta Nov 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The difference is real but narrow. vvecLegacyScheme is only set while the node's tip is below the v19 height (mainnet 1899072, testnet 850100, devnets 2), so on mainnet and testnet a node taking part in DKGs never decodes a vector that way. It is reachable on regtest with -testactivationheight=v19@N.

The smallest change that keeps the old behaviour there is to pass qc.vvecLegacyScheme into WriteVerifiedVvecContribution and skip the cache write when it is set, so those vectors keep going through the DB read. The later WriteVerifiedSkContribution then finds no entry and is a no-op for the cache, which is already handled.

If useful, both are available to take from PastaPastaPasta/dash, branch backup/dkg-commit-contrib-cache-68de86bf: 96b68d0 plumbs that parameter (it does more than needed; only the parameter and the skip matter here), and 04e93df has manager-level unit tests for the cache (covers the second finding, though some cases assume that commit's eviction rule and would need adjusting).

Whatever lands here, I'll redo the pick in #8128 to match.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Nov 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agreed that the affected path is narrow, rather than a normal synced-mainnet DKG path. Passing qc.vvecLegacyScheme into WriteVerifiedVvecContribution and skipping cache insertion after the existing DB write when it is true resolves this concern: those vectors retain the basic-scheme cold-read validation, and WriteVerifiedSkContribution already handles a cache miss safely. I’m satisfied with that minimal fix; the additional hash and eviction changes in 96b68d07e88e aren’t needed for this finding. This remains pending until the fix lands on the PR.

The finding stands on this commit until that change is pushed; the push gets a full review.

@PastaPastaPasta PastaPastaPasta Nov 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Following up on my comment above after looking at when the flag can actually be legacy: I don't think this should block.

  • vvecLegacyScheme is only set if the process-wide flag is legacy when the contribution is decoded. That needs a tip below the v19 height, or a background chainstate connecting pre-v19 blocks under a snapshot, and mainnet and testnet have no snapshot parameters. A node in that state is not taking part in DKGs with the rest of the network either way.
  • The re-check this removes was a side effect of reading the vector back from the DB in basic format, not a designed validation step. Where the session needs to know about legacy-decoded vectors it already does so explicitly (anyLegacyVvec in VerifyPendingContributions).
  • Passing the flag into WriteVerifiedVvecContribution would thread more legacy-scheme state through the manager just before the planned removal of the BLS globals. The better place to close this is at decode time, as part of that work, so a DKG message is never decoded under the legacy scheme at all.

So: fine to merge as is from my side; the suggestion above can be ignored.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Nov 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think the activation argument removes the issue: the DKG session code itself permits pre-v19 sessions (UpdatedBlockTip and the handler gate on DIP0003, while ActiveDKGSession sets m_use_legacy_bls before v19). In that path, PreVerifyMessage checks only CBLSPublicKey::IsValid(), which is the wrapper flag, and WriteVerifiedVvecContribution immediately places the legacy-decoded vector in the cache; the old commit-time DB read instead re-decodes the stored basic encoding and performs the subgroup check. Thus the cache changes behavior for a supported legacy/regtest path, regardless of whether a currently synced mainnet node reaches it, and the proposed future removal of the global scheme does not make this commit behavior-preserving.

}

void CDKGSessionManager::WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution)
{
db->Write(std::make_tuple(DB_SKCONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), skContribution);

LOCK(contributionsCacheCs);
if (auto it = contributionsCache.find({llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash}); it != contributionsCache.end()) {
it->second.skContribution = skContribution;
}
}

void CDKGSessionManager::WriteEncryptedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSIESMultiRecipientObjects<CBLSSecretKey>& contributions)
Expand Down Expand Up @@ -226,8 +237,10 @@ void CDKGSessionManager::CleanupCache() const
{
LOCK(contributionsCacheCs);
const auto curTime = SteadyClock::now();
std::erase_if(contributionsCache,
[&](const auto& entry) { return curTime - entry.second.entryTime > MAX_CONTRIBUTION_CACHE_TIME; });
// A vvec still referenced elsewhere (by its DKG session) costs nothing to keep and is expensive to reload
std::erase_if(contributionsCache, [&](const auto& entry) {
return entry.second.vvec.use_count() == 1 && curTime - entry.second.entryTime > MAX_CONTRIBUTION_CACHE_TIME;
Comment on lines +240 to +242

@thepastaclaw thepastaclaw Nov 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Add regression coverage through the actual contribution cache

The new benchmarks operate on local vectors and CBLSWorker; they never instantiate CDKGSessionManager or exercise its modified write, read, or cleanup paths. Existing functional DKG tests exercise successful commitments and justification, but do not directly assert ownership-dependent expiry or compare warm-cache results with disk-loaded results. Extend the existing DKG unit suite with manager-level checks that retain an aged entry while an external vector owner exists, expire it after that owner releases it, expose a subsequently written secret share, and preserve cached-versus-cold behavior. Include the legacy validation boundary described above so future performance changes cannot silently remove it again.

source: gpt-6.1-sol (phase2-reviewer: general)

});
}

void CDKGSessionManager::CleanupOldContributions() const
Expand Down
6 changes: 4 additions & 2 deletions src/llmq/dkgsessionmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,10 @@ class CDKGSessionManager
bool GetPrematureCommitment(const uint256& hash, CDKGPrematureCommitment& ret) const;

// Contributions are written while in the DKG
void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec);
void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution);
void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec)
EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs);
void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution)
EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs);
bool GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex,
const std::vector<bool>& validMembers, std::vector<uint16_t>& memberIndexesRet,
std::vector<BLSVerificationVectorPtr>& vvecsRet,
Expand Down
Loading