Skip to content

fix: authenticate every retained BLS signature individually #7840

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

Draft
PastaPastaPasta wants to merge 1 commit into
base: develop
Choose a base branch
from
+288 −349
Draft

fix: authenticate every retained BLS signature individually #7840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
179 changes: 25 additions & 154 deletions src/bls/bls_batchverifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@

#include <bls/bls.h>

#include <array>
#include <map>
#include <set>
#include <tuple>
#include <vector>

template<typename SourceId, typename MessageId>
Expand All @@ -21,11 +24,12 @@ class CBLSBatchVerifier
CBLSPublicKey pubKey;
};

using MessageMap = std::map<MessageId, Message>;
using MessageKey = std::tuple<MessageId, uint256, std::array<uint8_t, CBLSSignature::SerSize>,
std::array<uint8_t, CBLSPublicKey::SerSize>>;
using MessageMap = std::map<MessageKey, Message>;
using MessageMapIterator = typename MessageMap::iterator;
using MessagesBySourceMap = std::map<SourceId, std::vector<MessageMapIterator>>;

bool secureVerification;
bool perMessageFallback;
size_t subBatchSize;

Expand All @@ -37,8 +41,8 @@ class CBLSBatchVerifier
std::set<MessageId> badMessages;

public:
CBLSBatchVerifier(bool _secureVerification, bool _perMessageFallback, size_t _subBatchSize = 0) :
secureVerification(_secureVerification),
// Individual verification is safe in both modes; retain the parameter for existing callers.
CBLSBatchVerifier(bool /*secureVerification*/, bool _perMessageFallback, size_t _subBatchSize = 0) :
perMessageFallback(_perMessageFallback),
subBatchSize(_subBatchSize)
{
Expand All @@ -48,7 +52,9 @@ class CBLSBatchVerifier
{
assert(sig.IsValid() && pubKey.IsValid());

auto it = messages.emplace(msgId, Message{msgId, msgHash, sig, pubKey}).first;
// Only identical verification inputs may share a verdict.
const MessageKey key{msgId, msgHash, sig.ToBytes(false), pubKey.ToBytes(false)};
auto it = messages.emplace(key, Message{msgId, msgHash, sig, pubKey}).first;
messagesBySource[sourceId].emplace_back(it);

if (subBatchSize != 0 && messages.size() >= subBatchSize) {
Expand All @@ -70,162 +76,27 @@ class CBLSBatchVerifier

void Verify()
{
std::map<uint256, std::vector<MessageMapIterator>> byMessageHash;

for (auto it = messages.begin(); it != messages.end(); ++it) {
byMessageHash[it->second.msgHash].emplace_back(it);
}

if (VerifyBatch(byMessageHash)) {
// full batch is valid
return;
}

// revert to per-source verification
for (const auto& [from, message_map] : messagesBySource) {
bool batchValid = false;

// no need to verify it again if there was just one source
if (messagesBySource.size() != 1) {
byMessageHash.clear();
for (auto it = message_map.begin(); it != message_map.end(); ++it) {
byMessageHash[(*it)->second.msgHash].emplace_back(*it);
}
batchValid = VerifyBatch(byMessageHash);
}
if (!batchValid) {
badSources.emplace(from);

if (perMessageFallback) {
// revert to per-message verification
if (message_map.size() == 1) {
// no need to re-verify a single message
badMessages.emplace(message_map[0]->second.msgId);
} else {
for (const auto& msgIt : message_map) {
if (badMessages.count(msgIt->first)) {
// same message might be invalid from different source, so no need to re-verify it
continue;
}

const auto& msg = msgIt->second;
if (!msg.sig.VerifyInsecure(msg.pubKey, msg.msgHash)) {
badMessages.emplace(msg.msgId);
}
}
}
}
// Aggregate validity does not authenticate the individual signatures retained by callers.
const bool legacy_scheme = bls::bls_legacy_scheme.load();
std::set<const Message*> invalid_messages;
for (const auto& [key, msg] : messages) {
if (!msg.sig.VerifyInsecure(msg.pubKey, msg.msgHash, legacy_scheme)) {
invalid_messages.emplace(&msg);
}
}
Comment on lines 77 to 86

@thepastaclaw thepastaclaw Oct 7, 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: Measure loaded share-path throughput before marking the PR ready

Verify() now performs an individual verification for every distinct record, including honest same-hash signature shares that previously used an aggregate public key. This path processes batches of up to 32 shares on the 1–4-thread signing worker pool, with at most four unverified batches outstanding (net_signing.cpp:163 and 358–377). Those bounds limit queued work but do not establish that recovery latency remains acceptable under sustained signing load. The PR documents individual-verification measurements and estimated session costs, but explicitly leaves loaded throughput unmeasured. Before marking this draft ready, record share-path throughput, queueing, and recovery latency under representative InstantSend and ChainLocks load, including the one-worker configuration. This validates the availability cost without weakening individual authentication; no operational slowdown was measured in this review.

source: muse-spark-1.3-contributor (phase1-reviewer: general)

@PastaPastaPasta PastaPastaPasta Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We also ran a local honest-input share-path harness, with linked production share/recovery code and a mirrored dispatcher. For a 40-session InstantSend burst, completion rates were 9.6 vs 28.0 sessions per elapsed second with one worker, and 29.5 vs 66.2 with four (PR vs develop share verifier). With one worker and 20 sessions/s offered for six seconds, the PR reported 120 successful recoveries for 120 offered sessions in about 12.1 seconds, with 3.66-second median latency, 6.2-second maximum, and up to 3,692 pending shares; the comparison median was 32.6 ms. ChainLocks recovery over three repetitions was 542.5–550.0 ms vs 131.2–133.0 ms with one worker, and 208.9–219.0 ms vs 97.6–99.9 ms with four. Every final sample passed the zero-bad-source and expected-recovery-count assertions; those are aggregate counts, not an independent per-ID audit.

These are finite local observations on a shared M4 Pro, with load average reaching about 75. Each comparison pair ran PR first; pair-specific host load was not measured. Both comparison paths use the PR's unconditional recovery verification. The dispatcher is mirrored, not the production NetSigning object, and excludes its pending-map caps/drops and network workload. Elapsed worker-task duration is not CPU time. The invalid scheme attempt and crashing harness teardown are excluded; no production-crash claim is made. The PR body records these qualifications.

The results show a local availability cost; they do not establish sustained capacity, mainnet headroom, or a universal saturation threshold. Representative hardware, combined load and soak measurements remain open. #7840 stays draft, with both acceptance-boundary coverage and readiness unresolved.

🤖 Posted autonomously by Codex on behalf of pasta.

}

private:
// All Verify methods take ownership of the passed byMessageHash map and thus might modify the map. This is to avoid
// unnecessary copies

bool VerifyBatch(std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
{
if (secureVerification) {
return VerifyBatchSecure(byMessageHash);
} else {
return VerifyBatchInsecure(byMessageHash);
}
}

bool VerifyBatchInsecure(const std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
{
std::vector<CBLSSignature> sigsToAggregate;
std::vector<uint256> msgHashes;
std::vector<CBLSPublicKey> pubKeys;
std::set<MessageId> dups;

msgHashes.reserve(messages.size());
pubKeys.reserve(messages.size());
sigsToAggregate.reserve(messages.size());

std::vector<CBLSPublicKey> pubKeysToAggregate;
for (const auto& [msgHash, vec_message_it] : byMessageHash) {
pubKeysToAggregate.clear();
pubKeysToAggregate.reserve(vec_message_it.size());

for (const auto& msgIt : vec_message_it) {
const auto& msg = msgIt->second;

if (!dups.emplace(msg.msgId).second) {
for (const auto& [source, source_messages] : messagesBySource) {
for (const auto& msg_it : source_messages) {
const auto& msg = msg_it->second;
if (invalid_messages.count(&msg) == 0) {
continue;
}

sigsToAggregate.push_back(msg.sig);
pubKeysToAggregate.push_back(msg.pubKey);
}

CBLSPublicKey aggPubKey = CBLSPublicKey::AggregateInsecure(pubKeysToAggregate);

if (!aggPubKey.IsValid()) {
// only duplicates for this msgHash
continue;
}

msgHashes.emplace_back(msgHash);
pubKeys.emplace_back(aggPubKey);
}

if (msgHashes.empty()) {
return true;
}

CBLSSignature aggSig = CBLSSignature::AggregateInsecure(sigsToAggregate);
return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes);
}

bool VerifyBatchSecure(std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
{
// Loop until the byMessageHash map is empty, which means that all messages were verified
// The secure form of verification will only aggregate one message for the same message hash, even if multiple
// exist (signed with different keys). This avoids the rogue public key attack.
// This is slower than the insecure form as it requires more pairings
while (!byMessageHash.empty()) {
if (!VerifyBatchSecureStep(byMessageHash)) {
return false;
}
}
return true;
}

bool VerifyBatchSecureStep(std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
{
std::vector<CBLSSignature> sigsToAggregate;
std::vector<uint256> msgHashes;
std::vector<CBLSPublicKey> pubKeys;
std::set<MessageId> dups;

msgHashes.reserve(messages.size());
pubKeys.reserve(messages.size());
sigsToAggregate.reserve(messages.size());

for (auto it = byMessageHash.begin(); it != byMessageHash.end(); ) {
const auto& msgHash = it->first;
auto& messageIts = it->second;
const auto& msg = messageIts.back()->second;

if (dups.emplace(msg.msgId).second) {
msgHashes.emplace_back(msgHash);
pubKeys.emplace_back(msg.pubKey);
sigsToAggregate.push_back(msg.sig);
}

messageIts.pop_back();
if (messageIts.empty()) {
it = byMessageHash.erase(it);
} else {
++it;
badSources.emplace(source);
if (perMessageFallback) {
badMessages.emplace(msg.msgId);
}
}
}

assert(!msgHashes.empty());

CBLSSignature aggSig = CBLSSignature::AggregateInsecure(sigsToAggregate);
return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes);
}
};

Expand Down
61 changes: 3 additions & 58 deletions src/bls/bls_worker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,15 +790,6 @@ void CBLSWorker::AsyncVerifySig(const CBLSSignature& sig, const CBLSPublicKey& p

std::unique_lock<std::mutex> l(sigVerifyMutex);

bool foundDuplicate = std::ranges::any_of(sigVerifyQueue,
[&msgHash](const auto& job) { return job.msgHash == msgHash; });

if (foundDuplicate) {
// batched/aggregated verification does not allow duplicate hashes, so we push what we currently have and start
// with a fresh batch
PushSigVerifyBatch();
}

sigVerifyQueue.emplace_back(std::move(doneCallback), std::move(cancelCond), sig, pubKey, msgHash);
if (sigVerifyBatchesInProgress == 0 || sigVerifyQueue.size() >= SIG_VERIFY_BATCH_SIZE) {
PushSigVerifyBatch();
Expand All @@ -823,56 +814,10 @@ void CBLSWorker::PushSigVerifyBatch()
{
auto f = [this](int threadId, const std::shared_ptr<std::vector<SigVerifyJob> >& _jobs) {
auto& jobs = *_jobs;
if (jobs.size() == 1) {
const auto& job = jobs[0];
const bool legacy_scheme = bls::bls_legacy_scheme.load();
for (const auto& job : jobs) {
if (!job.cancelCond()) {
bool valid = job.sig.VerifyInsecure(job.pubKey, job.msgHash);
job.doneCallback(valid);
}
std::unique_lock<std::mutex> l(sigVerifyMutex);
sigVerifyBatchesInProgress--;
if (!sigVerifyQueue.empty()) {
PushSigVerifyBatch();
}
return;
}

CBLSSignature aggSig;
std::vector<size_t> indexes;
std::vector<CBLSPublicKey> pubKeys;
std::vector<uint256> msgHashes;
indexes.reserve(jobs.size());
pubKeys.reserve(jobs.size());
msgHashes.reserve(jobs.size());
for (size_t i = 0; i < jobs.size(); i++) {
auto& job = jobs[i];
if (job.cancelCond()) {
continue;
}
if (pubKeys.empty()) {
aggSig = job.sig;
} else {
aggSig.AggregateInsecure(job.sig);
}
indexes.emplace_back(i);
pubKeys.emplace_back(job.pubKey);
msgHashes.emplace_back(job.msgHash);
}

if (!pubKeys.empty()) {
bool allValid = aggSig.VerifyInsecureAggregated(pubKeys, msgHashes);
if (allValid) {
for (size_t i = 0; i < pubKeys.size(); i++) {
jobs[indexes[i]].doneCallback(true);
}
} else {
// one or more sigs were not valid, revert to per-sig verification
// TODO this could be improved if we would cache pairing results in some way as the previous aggregated verification already calculated all the pairings for the hashes
for (size_t i = 0; i < pubKeys.size(); i++) {
const auto& job = jobs[indexes[i]];
bool valid = job.sig.VerifyInsecure(job.pubKey, job.msgHash);
job.doneCallback(valid);
}
job.doneCallback(job.sig.VerifyInsecure(job.pubKey, job.msgHash, legacy_scheme));
}
}

Expand Down
Loading
Loading