test: check the BLS proof of possession - #3708
Merged
Merged
Conversation
CIP-0164 makes two statements about the proof that accompanies a BLS key in a pool registration certificate, and they land differently. It "is mandatory and verified at registration" - that one is not implemented. The Dijkstra `POOL` rule reuses `ShelleyPoolPredFailure` unchanged and has no BLS predicate failure at all, so a registration carrying a proof that belongs to another key is accepted and the pair is stored verbatim. `test_mismatched_pop_is_not_verified` pins that down, so that the day the ledger starts rejecting it is a day a test says so. "Only a key with a valid proof of possession may occupy a committee seat or contribute to a certificate" - that one is implemented, in `mkLeiosCommittee`, which verifies the proof and admits the seat keyless when it fails. `test_mismatched_pop_is_not_seated` checks it: the pool keeps its seat and the ledger keeps reporting the key, but the seat is not voting. An aged-out key looks the same from the outside, so the test also checks the key is well inside its lifetime. The proof cannot be generated on its own - the CLI derives it from the signing key and emits it only inside a registration certificate - so the mismatch is built by splicing the proof of a second certificate into the first, shared by both tests in `register_pool_with_mismatched_pop`. `BLS_ACTIVATION_EPOCHS`, `get_committee_seat` and `get_max_key_age` move to `bls.py`, now that more than one test module needs them.
A pool name is limited to 50 characters, and a test ID eats most of that on its own - it is the test function name plus the cluster ID plus a random string, 44 to 52 characters for the tests in these two modules. Name the pools after a short random string instead, the way `test_pool_registration_conway_cert` already does. The derived names were the worse offenders. `rotate_bls_key` appended the whole transaction name to a pool name that already contained the test ID, so the ID appeared twice and the certificate of `test_drop_and_restore_bls_key` was named with 95 characters. The rotation now takes a short `cert_suffix` for the pool name, while `tx_name` keeps naming the transaction, where nothing is constrained. The longest name left is 18 characters. Nothing reads a pool name except the generated file names, and none of these pools carry metadata, so the limit was never enforced on them - they were over it all the same.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add a cluster-era guard for the Dijkstra-dependent test class.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds BLS proof-of-possession coverage and shortens generated pool names.
Changes:
- Tests mismatched proof registration and committee non-voting behavior.
- Centralizes shared BLS helpers.
- Separates certificate suffixes from transaction names.
| File | Summary |
|---|---|
cardano_node_tests/tests/test_bls_rotation.py |
Updates shared helper usage and pool naming. |
cardano_node_tests/tests/test_bls_keys.py |
Adds mismatched proof-of-possession tests. |
cardano_node_tests/tests/bls.py |
Centralizes BLS timing and committee helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

CIP-0164 makes two statements about the proof of possession that
accompanies a BLS key in a pool registration certificate, because BLS
aggregate signatures are otherwise open to rogue-key attacks. They land
differently, so there is a test for each.
"Mandatory and verified at registration" - not implemented. The
Dijkstra
POOLrule reusesShelleyPoolPredFailureunchanged and has noBLS predicate failure at all, so a registration carrying a proof that
belongs to another key is accepted and the pair is stored verbatim.
test_mismatched_pop_is_not_verifiedpins that down, so the day theledger starts rejecting such a registration is a day a test says so,
rather than a silent change of behaviour.
"Only a key with a valid proof of possession may occupy a committee
seat or contribute to a certificate" - implemented, one layer below the
ledger rules, in
mkLeiosCommittee:test_mismatched_pop_is_not_seatedchecks the observable consequence:the pool keeps its committee seat and the ledger keeps reporting the key
it registered, but the seat is not voting, so the key can neither vote
nor contribute to a certificate. An aged-out key looks identical from the
outside, so the test also asserts the key is well inside its
maxKeyAgelifetime and age cannot be the reason.
The proof cannot be generated on its own - the CLI derives it from the
signing key and emits it only inside a registration certificate - so the
mismatch is built by splicing the proof out of a second certificate into
the first, checking that the re-encoded certificate differs from the
original in nothing but the proof. Both tests share that setup in
register_pool_with_mismatched_pop.BLS_ACTIVATION_EPOCHS,get_committee_seatandget_max_key_agemovefrom
test_bls_rotation.pytobls.py, now that more than one testmodule needs them (-53/+13 there, all call sites mechanical).
test_mismatched_pop_is_not_seatedwaits two epoch boundaries, so it ismarked
longand, unlike its sibling, nottestnets. It is markedleiosinstead, becauseregression-leios.yamlis the only workflowthat routinely runs a Dijkstra cluster and it selects with
markexpr: testnets or leios- without the marker the test would beselected nowhere.
test: keep pool names inside the 50 char limit
A pool name is limited to 50 characters, and a test ID eats most of that
on its own: the test function name plus the cluster ID plus a random
string comes to 44-52 characters for the tests in these two modules. The
pools are now named after a short random string, the way
test_pool_registration_conway_certalready does.The derived names were the worse offenders.
rotate_bls_keyappended thewhole transaction name to a pool name that already contained the test ID,
so the ID appeared twice and the certificate of
test_drop_and_restore_bls_keywas named with 95 characters. Therotation now takes a short
cert_suffixfor the pool name, whiletx_namekeeps naming the transaction, where nothing is constrained.The longest name left is 18 characters. Nothing reads a pool name except
the generated file names, and none of these pools carry metadata, so the
limit was never enforced on them - they were over it all the same.