Skip to content

feat(aggregator)!: support multiple payer-signer pairs - #23

Merged
tmigone merged 6 commits into
mainfrom
tmigone/multi-signer-payer
Sep 24, 2026
Merged

tmigone merged 6 commits into
mainfrom
tmigone/multi-signer-payer

Conversation

@tmigone

@tmigone tmigone commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Overview

The aggregator held one signing key and signed every RAV with it, whatever payer the receipts named. It now picks the key belonging to that payer, and refuses payers it holds no key for.

 fn check_and_aggregate_receipts(receipts, previous_rav, signers) {
+    let payer = receipts.first().payer;
+    let (signing_key, accepted) = signers.resolve(payer)
+        .ok_or("no signing key configured for payer")?;
+
-    verify_each(receipts,     &accepted_addresses)?;   // one global set
-    verify(previous_rav,      &accepted_addresses)?;
+    verify_each(receipts,     accepted)?;              // scoped to this payer
+    verify(previous_rav,      accepted)?;
     ...
-    sign(rav, &wallet)                                 // one key, any payer
+    sign(rav, signing_key)
 }

The payer is read before any signature check, because it selects both the signing key and the accepted-signer set. grafana/aggregator.json is a new dashboard and accounts for most of the line count; it needs no review.

Approach

  • Accepted signers are scoped per payer rather than one global set. A signer accepted for one payer must not vouch for another's receipts.
  • A payer with no configured key is refused, not signed with whatever key is loaded. The collector requires a RAV's signer to be authorized for that RAV's payer.
  • No single-payer mode is kept. A key alone cannot say which payer it serves, so single-payer is one map entry.

Configuration

  • GRAPH_TALLY_SIGNERS — new and required. Every deployment fails to start until set. Format: ;-separated <payer address>=<signer private key>.
  • GRAPH_TALLY_ACCEPTED_SIGNERS — new, optional. No action needed.
  • GRAPH_TALLY_PRIVATE_KEY — removed, superseded by GRAPH_TALLY_SIGNERS. Left set, it is now ignored.
  • GRAPH_TALLY_PUBLIC_KEYS — removed, superseded by GRAPH_TALLY_ACCEPTED_SIGNERS.

Motivation

The RAV carries the payer named in its receipts, but was signed with this process's own key. Across a payer migration that produced RAVs carrying the old payer signed by the new signer — rejected by indexers, uncollectable on chain.

Cross-component

The JSON-RPC handler now keys its gateway_ravs Kafka record by payer instead of the signing wallet, matching the gRPC handlers — downstream consumers of that topic see a changed key on that path. Separately, a request for an unconfigured payer now errors where an invalid RAV was returned, a semantic change no signature reveals. Not verified against consumers; confirm before merging.

Risk assessment

  • blast_radius: cross-component — every deployment must be reconfigured before it starts, and one Kafka record key changes for downstream consumers.
  • reviewer_effort: careful — the logic is ~200 lines, but the ordering and the per-payer scoping carry the correctness; the rest of the diff is a dashboard.
  • primary_concern — accepted_signers is trusted from config with no on-chain check, so anything listed can have receipts aggregated against that payer's escrow.
  • touches_access_control — changes which keys sign RAVs and which receipt signers are accepted, per payer.
  • breaks_api — the config surface changes incompatibly: one new required variable and two removed. The title carries ! so release-please marks it breaking.

Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
@tmigone
tmigone force-pushed the tmigone/multi-signer-payer branch from f400442 to 9e70917 Compare September 23, 2026 19:38
…signers

`run_server` now takes a `SignerRegistry` instead of a wallet plus a flat
accepted-signer set. The registry is keyed by payer, so the test helper
builds a one-entry map for the `payer` fixture that every receipt in these
tests carries.

Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coveralls

coveralls commented Sep 23, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 36008634461

Coverage increased (+4.7%) to 48.854%

Details

  • Coverage increased (+4.7%) from the base build.
  • Patch coverage: 19 uncovered changes across 3 files (264 of 283 lines covered, 93.29%).
  • 1 coverage regression across 1 file.

Uncovered Changes

File Changed Covered %
crates/bin/aggregator/src/main.rs 93 76 81.72%
crates/bin/aggregator/src/aggregator.rs 43 42 97.67%
crates/bin/aggregator/src/server.rs 6 5 83.33%
Total (4 files) 283 264 93.29%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File Lines Losing Coverage Coverage
crates/bin/aggregator/src/main.rs 1 50.67%

Coverage Stats

Coverage Status
Relevant Lines: 2837
Covered Lines: 1386
Line Coverage: 48.85%
Coverage Strength: 130.08 hits per line

💛 - Coveralls

Maikol
Maikol previously approved these changes Sep 23, 2026
.map(|(payer, key)| {
let payer: Address = payer
.parse()
.with_context(|| format!("GRAPH_TALLY_SIGNERS: parse payer address {payer:?}"))?;

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.

Are we leaking the private key in logs here?

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.

mmm no, why do you suggest that?

Comment thread crates/bin/aggregator/README.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Cross-payer signer reuse and malformed configuration entries are not safely rejected, and migration documentation is inconsistent.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Adds payer-scoped signing to prevent invalid cross-payer RAVs.

Changes:

  • Introduces per-payer signer registries and validation.
  • Updates configuration, Kafka keys, documentation, and tests.
  • Adds an aggregator Grafana dashboard.
File Description
grafana/​aggregator.json Adds operational dashboard.
crates/​integration_tests/​tests/​showcase.rs Uses payer-scoped registry.
crates/​bin/​aggregator/​tests/​aggregate_test.rs Updates aggregation setup.
crates/​bin/​aggregator/​src/​signers.rs Implements signer registry.
crates/​bin/​aggregator/​src/​server.rs Integrates registry and payer Kafka keys.
crates/​bin/​aggregator/​src/​main.rs Adds new configuration parsing.
crates/​bin/​aggregator/​src/​lib.rs Exports signer module.
crates/​bin/​aggregator/​src/​aggregator.rs Selects keys and accepted signers by payer.
crates/​bin/​aggregator/​README.md Documents configuration and migration.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/bin/aggregator/src/signers.rs Outdated
Comment thread crates/bin/aggregator/src/main.rs Outdated
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The public unchecked registry constructor can bypass the core cross-payer authorization invariant.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Update stale startup description for signer registry parsing

grafana/​aggregator.json:639

This description still refers to the removed single private-key parsing path and stale line numbers. The current startup sequence starts metrics before parsing the signer registry, so update the text to describe that behavior without referencing a nonexistent private key.

Comment thread crates/bin/aggregator/src/signers.rs Outdated
Comment thread crates/bin/aggregator/src/main.rs Outdated
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The JSON-RPC Kafka key change causes the escrow-manager consumer to discard its RAV records.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Instances Up panel shows total outage as healthy

grafana/​aggregator.json:705

The “Instances Up” panel colors a total outage green: its only threshold maps value 0 to green. Add a red threshold at zero and switch to green at one so a down deployment is not presented as healthy.

Low severity Description uses incorrect line references and obsolete key terminology

grafana/​aggregator.json:639

This new description documents the removed single-private-key flow and already has incorrect line references: main.rs:105 parses arguments, while metrics starts at line 110 and the signer registry is parsed at lines 112-121. Describe the ordering without brittle source locations or the obsolete “private key” wording.

Comment thread crates/bin/aggregator/src/server.rs
@tmigone
tmigone merged commit b832ac8 into main Sep 24, 2026
14 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants