feat: support definitions version 2 - #105
Conversation
obrusvit
commented
Aug 28, 2026
obrusvit
commented
- version 2 requires only one signature
- version 2 requires only one signature
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds support for “definitions format version 2”, making signing/verification and generation version-aware (including the updated signature threshold and versioned output directories/artifacts).
Changes:
- Introduces format v2 as an active version and documents behavioral differences (payload header
trzd2, fewer required CoSi signatures). - Updates scripts/CLI flows to sign and generate outputs per definitions format version.
- Adds/extends tests to validate version handling and payload header encoding.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates signing procedure documentation for versioned workflows. |
| do_update.sh | Generates signed outputs for all active definitions versions. |
| do_sign.sh | Signs and generates outputs for a specified definitions version. |
| definitions/test_crypto.py | Adds tests around version-specific signature-key requirements. |
| definitions/test_common.py | Improves version validation tests and adds payload header assertions. |
| definitions/sign.py | Makes signature verification version-aware. |
| definitions/generate.py | Makes generation output directory default version-specific and verifies signatures per version. |
| definitions/crypto.py | Makes verification depend on version-specific required signature count. |
| definitions/common.py | Activates version 2 and introduces versioned generated output directory + signature requirements mapping. |
| .gitignore | Ignores versioned generated output directories (definitions-latest-v*). |
Suppressed comments (1)
definitions/test_crypto.py:24
- After switching
_combine_public_keyto raiseInvalidSignaturefor an insufficientsigmask, this test should be updated to assert that exception type (rather thanAssertionError) so CLI behavior and unit tests are consistent.
def test_v1_requires_two_keys_v2_one_key():
# one key set in sigmask
assert _combine_public_key(0b001, 2)
with pytest.raises(AssertionError):
_combine_public_key(0b001, 1)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| assert len(selected_keys) >= SIGNATURES_REQUIRED[version] | ||
| return cosi.combine_keys(selected_keys) |
| import pytest | ||
|
|
||
| from .common import SIGNATURES_REQUIRED | ||
| from .crypto import _combine_public_key | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("version", sorted(SIGNATURES_REQUIRED)) | ||
| def test_combine_public_key_accepts_required_key_count(version): | ||
| sigmask = (1 << SIGNATURES_REQUIRED[version]) - 1 | ||
| assert _combine_public_key(sigmask, version) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("version", sorted(SIGNATURES_REQUIRED)) | ||
| def test_combine_public_key_rejects_too_few_keys(version): | ||
| sigmask = (1 << (SIGNATURES_REQUIRED[version] - 1)) - 1 | ||
| with pytest.raises(AssertionError): | ||
| _combine_public_key(sigmask, version) |
| - get the signature and provide it as an argument to `do_sign.sh`, e.g. `./do_sign.sh abcd...` | ||
| - get the signature and provide it as an argument to `do_sign.sh`, e.g. `./do_sign.sh 2 abcd...` (version first, signature second) | ||
| - the results should look something like this signing commit - https://github.com/trezor/definitions/commit/42d3093e83c85dade59af92a37fb3c33d3b047eb | ||
| - `definitions.tar.gz` file should also be created, containing signed definitions, ready for deployment |
| return version | ||
|
|
||
|
|
||
| def resolve_default_version() -> int: |
There was a problem hiding this comment.
It can be nice to encapsulate all version-related information in a new type, e.g. similar to enum DefsVersion in Rust.
@dataclass
class DefsVersion:
signatures_required: int
generated_definitions_dir: Path
@classmethod
def create(version: int) -> Self:
# will first validate and then create `DefsVersion` instanceWDYT?
|
|
||
|
|
||
| def _combine_public_key(sigmask: int) -> bytes: | ||
| def _combine_public_key(sigmask: int, version: int) -> bytes: |
There was a problem hiding this comment.
Needs a docstring or a comment explaining this.
There was a problem hiding this comment.
We have 3 public keys. sigmask signalizes which ones are used for signing, so for version 1, it can be e.g. 110 (first two chosen), or 101 (first and last), or 111 (all of them). For version 2, it can e.g. 100 (only 1st), 010 (only 2nd) etc. The added assert checks that the sigmask was correctly chosen.
