Skip to content

Refactor .py signatures infra#7904

Open
maxtropets wants to merge 10 commits into
microsoft:mainfrom
maxtropets:f/refactor-py-sig-infra
Open

Refactor .py signatures infra#7904
maxtropets wants to merge 10 commits into
microsoft:mainfrom
maxtropets:f/refactor-py-sig-infra

Conversation

@maxtropets
Copy link
Copy Markdown
Collaborator

@maxtropets maxtropets commented May 20, 2026

Motto: consolidate signature verification in one place, and benefit from it greatly when introducing PQC COSE signatures in the future.

I still plan to keep a split for verifying classical/cose, but for cose we'll extend the table itself in the future and will iterate over multiple cose signatures instead, and will try to get rid of classical signatures somewhere in the future.

@maxtropets maxtropets self-assigned this May 20, 2026
@maxtropets maxtropets added the run-long-test Run Long Test job label May 20, 2026
@maxtropets maxtropets removed the run-long-test Run Long Test job label May 21, 2026
@maxtropets maxtropets marked this pull request as ready for review May 21, 2026 10:12
@maxtropets maxtropets requested a review from a team as a code owner May 21, 2026 10:12
Copilot AI review requested due to automatic review settings May 21, 2026 10:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the Python-side ledger signature handling by introducing a dedicated ccf.signatures module and updating tests/utilities to use a shared “is this a signature transaction?” predicate and shared parsing/verification helpers. It keeps external compatibility by re-exporting legacy names from ccf.ledger.

Changes:

  • Added python/src/ccf/signatures.py to centralize signature table constants, parsing, and primitive verification helpers.
  • Refactored python/src/ccf/ledger.py’s LedgerValidator signature validation to use the new shared helpers, while re-exporting prior public symbols for backwards compatibility.
  • Updated multiple tests and Python utilities to detect signature transactions via is_signature_transaction() rather than ad-hoc table checks/string matching.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/recovery.py Uses ccf.signatures.is_signature_transaction() for signature tx detection in recovery corruption/selection helpers.
tests/reconfiguration.py Switches signature parsing to parse_classical_signatures() and uses structured payload fields.
tests/governance_history.py Refactors signature schema/position checks to use shared signature-tx detection + structured parsing.
tests/e2e_operations.py Consolidates signature transaction detection via ccf.signatures.is_signature_transaction().
python/src/ccf/split_ledger.py Uses shared signature-tx detection when splitting ledgers around signature transactions.
python/src/ccf/signatures.py New module defining signature table registry, parsers, and primitive verifiers.
python/src/ccf/ledger.py Re-exports signature APIs for compatibility and refactors validator signature verification flow.
python/src/ccf/ledger_viz.py Uses shared signature-tx detection for labeling signature entries in visualization.

Comment thread tests/reconfiguration.py
Comment thread python/src/ccf/ledger.py Outdated
Comment thread python/src/ccf/ledger.py Outdated
Comment thread python/src/ccf/ledger.py Outdated
Comment thread python/src/ccf/ledger.py Outdated
Comment thread python/src/ccf/ledger.py Outdated
Comment thread python/src/ccf/signatures.py Outdated
Comment thread python/src/ccf/signatures.py
Comment thread python/src/ccf/signatures.py
Comment thread python/src/ccf/signatures.py
Comment thread python/src/ccf/signatures.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread python/src/ccf/ledger.py
Comment on lines +687 to +701
payload = signatures.parse_raw_signature_from_tx(tables)
if payload is not None:
self._verify_signing_node_status(payload.signing_node)
cert = self.node_certificates[payload.signing_node]
if payload.embedded_cert is not None:
assert signatures.spki_from_cert(
cert
) == signatures.spki_from_cert(
payload.embedded_cert
), f"Mismatch in public key for node {payload.signing_node}"
signatures.verify_raw_root_signature(
cert, payload.root, payload.signature
)
signatures.verify_merkle_root(self.merkle, payload.root)
verified_count += 1
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@copilot how come if the check in l688 does exactly that?

Comment thread CHANGELOG.md Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@maxtropets maxtropets added the run-long-test Run Long Test job label May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-long-test Run Long Test job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants