Skip to content

Organize CP MiniZinc parts with their respective meanings#446

Merged
peacker merged 10 commits into
developfrom
refactor/cp-mzn-models-fulfill-model-parts
May 1, 2026
Merged

Organize CP MiniZinc parts with their respective meanings#446
peacker merged 10 commits into
developfrom
refactor/cp-mzn-models-fulfill-model-parts

Conversation

@juaninf
Copy link
Copy Markdown
Collaborator

@juaninf juaninf commented Apr 29, 2026

Organize CP MiniZinc parts with their respective meanings

The scope of this PR is "put the right content in the right attribute". In the next PR we will modify every model to use the transport class MiniZincModelParts introduced in #445. By attributes we mean _model_prefix, _variables_list, _model_constraints, mzn_output_directives. For example, before we were mixing content that must be in _variable_list with _model_prefix.

We needed to update test_assemble_model_preserves_legacy_order and test_assemble_model_accepts_explicit_parts because the first one is not anymore a legacy and the second one because we needed to decouple it from a specific cipher (since the "MiniZinc model parts" should not be attached to a cipher )

We needed to update more tests in order to fulfill "organize CP MiniZinc parts with their respective meanings" refactor. Here the explanation why we need to update each tests.

Why the tests had to change

Before this refactor, several CP MiniZinc model builders mixed parts:

  • _model_prefix collected variable declarations on top of the include directives and helper predicates it was supposed to hold.
  • _variables_list received items returned as constraints by input_* methods.
  • Models then baked the prefix into _model_constraints with self._model_constraints = self._model_prefix + self._model_constraints.

That layout made model_constraints carry items that were not constraints (the includes and helpers, plus the input declarations), and model_variables carry items that were not declarations (the cp_constraints returned from input_* methods).

The refactor moves each fragment to its semantically correct attribute:

Attribute What it should hold
_model_prefix only include directives and helper predicates
_variables_list only variable declarations
_model_constraints only constraints and the solve directive
mzn_output_directives only output [...] directives

MznModel.current_model_parts() now exposes prefix=list(self._model_prefix) so assemble_model() produces the same final MiniZinc source without each builder having to pre-merge the prefix into _model_constraints.

This invalidates two kinds of test assertions that lived in the test suite:

  1. Index-based reads of model_constraints[i] that returned a declaration. Those declarations now live in model_variables, not model_constraints.
  2. Total-count assertions of len(model_constraints) + len(model_variables). The two prefix items (include "globals.mzn"; and the MINIZINC_USEFUL_FUNCTIONS block) no longer appear in model_constraints after
    deduplication, so the combined count drops by exactly 2.

The sections below document each updated test and why.


tests/unit/cipher_modules/models/cp/mzn_model_test.py

test_assemble_model_orders_variables_constraints_outputs (line 31, was test_assemble_model_preserves_legacy_order)

Changes:

  1. Renamed from test_assemble_model_preserves_legacy_order — the new name describes what the test actually verifies now that the prefix is a separate,
    explicitly-cleared part.
  2. Added model._model_prefix = [] immediately after constructing the model.

Reason: MznModel.__init__ calls initialise_model(), which seeds _model_prefix with ['include "globals.mzn";', MINIZINC_USEFUL_FUNCTIONS]. The
test sets _variables_list, _model_constraints, and mzn_output_directives
manually and then asserts the assembled output is exactly:

var int: x;
constraint x = 1;
solve satisfy;
output ["x=", show(x)];

Now that current_model_parts() includes prefix=list(self._model_prefix), the assembled output also contains the seeded include + helper block. The test narrowly exercises the variables → constraints → outputs ordering, so we
explicitly clear _model_prefix to keep the expected string as-is. The assertion itself is unchanged. The "legacy" framing in the old name no longer fit (the test no longer mirrors the pre-refactor _model_constraintsprefix-bake), so the test is renamed to describe what it now checks.

test_build_generic_cp_model_from_dictionary_xor_linear (line ~340)

Change: replaced the legacy "extend prefix with declarations and bake the prefix into constraints" pattern with the new direct placement:

Before:

variables, constraints = model.input_xor_linear_constraints()
model._model_prefix.extend(variables)
model._variables_list.extend(constraints)
model._model_constraints.extend(model.final_xor_linear_constraints(weight))
model._model_constraints = model._model_prefix + model._model_constraints

After:

variables, constraints = model.input_xor_linear_constraints()
model._variables_list.extend(variables)
model._model_constraints.extend(constraints)
model._model_constraints.extend(model.final_xor_linear_constraints(weight))

Reason: the test was reproducing the bug at the call site — declarations were sent to _model_prefix, constraints were sent to _variables_list, and then the prefix was baked into _model_constraints. With current_model_parts() now returning the prefix, repeating that pattern would emit the prefix twice. The new code mirrors what the production builders now do and exercises the same end-to-end model.solve(...) path.

test_minizinc_model_parts_lines_concatenates_in_order (was test_assemble_model_accepts_explicit_parts)

Change: rewrote the test to call MiniZincModelParts.lines() directly, asserting on the returned list. Removed the SpeckBlockCipher /
MznModel(speck) setup.

Before:

def test_assemble_model_accepts_explicit_parts():
    speck = SpeckBlockCipher(number_of_rounds=1)
    model = MznModel(speck)
    parts = MiniZincModelParts(...)
    assert model.assemble_model(parts) == "<concatenated string>"

After:

def test_minizinc_model_parts_lines_concatenates_in_order():
    parts = MiniZincModelParts(...)
    assert parts.lines() == [
        'include "globals.mzn";',
        "var int: x;",
        "constraint x = 1;",
        "solve satisfy;",
        'output ["x=", show(x)];',
    ]

Reason: the assertion only verified that
prefix + variables + constraints + outputs + carries_outputs are returned in that order — pure-function behaviour of the dataclass. The cipher and the MznModel instance were not used by the assembly path when parts is provided (assemble_model_lines(parts) returns parts.lines() without touching self). They were a coupling artefact left over from MznModel.__init__ requiring a cipher. Testing the dataclass directly removes the unnecessary fixture, makes the test faster, and pins the contract on the layer that actually owns it. The MznModel.assemble_model delegation is still exercised by test_assemble_model_orders_variables_constraints_outputs above.


tests/unit/cipher_modules/models/cp/mzn_models/mzn_impossible_xor_differential_model_test.py

test_build_impossible_xor_differential_trail_with_extensions_model (line 11)

Change 1 — total count: 1764 → 1762.

Change 2 — index-based assertions replaced with membership checks:

Before:

assert mzn.model_constraints[99] == "array[0..31] of var 0..2: inverse_plaintext;"
assert mzn.model_constraints[3]  == "array[0..63] of var 0..2: key;"
assert mzn.model_constraints[39] == "array[0..31] of var 0..2: cipher_output_5_12;"

After:

assert "array[0..31] of var 0..2: inverse_plaintext;" in mzn.model_variables
assert "array[0..63] of var 0..2: key;" in mzn.model_variables
assert "array[0..31] of var 0..2: cipher_output_5_12;" in mzn.model_variables

Reason: the strings being asserted are MiniZinc array declarations (start witharray[...] of var ...:). Under the old buggy behavior they ended up in
_model_constraints because input_impossible_constraints_with_extensions returned (declarations, constraints) and the builder put the declarations in
_model_prefix, which then got baked into _model_constraints. After the refactor those declarations live in _variables_list. The exact index they occupy depends on the order in which direct_variables, inverse_variables, key_schedule_variables, constants_variables, and the cp_declarations from input_impossible_constraints_with_extensions are appended, plus the deduplication pass at the bottom of the builder. Membership checks are robust to this ordering and still verify the declarations are produced.

The total drops by 2 because the two prefix items (include "globals.mzn"; and MINIZINC_USEFUL_FUNCTIONS) used to be deduped into _model_constraints along with set_of_constraints. After the refactor the prefix lives only in _model_prefix (added at assembly time via current_model_parts()), so it no longer contributes to model_constraints + model_variables.

test_build_impossible_xor_differential_trail_model (line 30)

Change 1 — total count: 1661 → 1659.

Change 2 — index-based assertions replaced with membership checks (same
pattern as above).

Reason: identical reasoning. The other builder method (build_impossible_xor_differential_trail_model) had the same prefix/variables mix-up and the same prefix-baking. The two prefix items disappear from the combined count, and the declarations move to model_variables.


tests/unit/cipher_modules/models/cp/mzn_models/mzn_hybrid_impossible_xor_differential_model_test.py

test_build_impossible_xor_differential_trail_model (line 10)

Change 1 — total count: 2442 → 2440.

Change 2 — index-based assertions replaced with membership checks against
model_variables:

assert "set of int: ext_domain = 0..2 union { i | i in 10..800 where (i mod 10 = 0)};" in mzn.model_variables
assert "array[0..63] of var ext_domain: plaintext;" in mzn.model_variables
assert "array[0..79] of var ext_domain: key;" in mzn.model_variables
assert "array[0..63] of var ext_domain: inverse_cipher_output_3_19;" in mzn.model_variables

Reason: all four strings are declarations (the set of int: line introduces the ext_domain type alias used by every subsequent array[...] of var ext_domain: ... declaration). They are produced by MznHybridImpossibleXorDifferentialModel.input_constraints as part of cp_declarations. Before the refactor they sat in _model_constraints because the builder pushed them through _model_prefix. After the refactor they live in _variables_list. The combined count drops by 2 for the prefix items, same as in the impossible model tests.


tests/unit/cipher_modules/models/cp/mzn_models/mzn_deterministic_truncated_xor_differential_model_test.py

test_build_deterministic_truncated_xor_differential_trail_model (line 10)

Change 1 — total count: 438 → 436.

Change 2 — index-based assertions replaced with membership checks against
model_variables:

assert "array[0..31] of var 0..2: plaintext;" in mzn.model_variables
assert "array[0..63] of var 0..2: key;" in mzn.model_variables
assert "array[0..15] of var 0..2: rot_0_0;" in mzn.model_variables

Reason: the three strings are declarations returned by
input_deterministic_truncated_xor_differential_constraints. Same story as the
impossible-model tests: they used to be in _model_constraints via the
prefix-bake, now they are in _variables_list.


tests/unit/cipher_modules/models/cp/mzn_models/mzn_wordwise_deterministic_truncated_xor_differential_model_test.py

test_find_one_wordwise_deterministic_truncated_xor_differential_trail (line 9)

Change 1 — total count: 1361 → 1359.

Change 2 — index-based assertions replaced with membership checks against
model_variables:

assert "array[0..15] of var 0..3: key_active;" in mzn.model_variables
assert "array[0..15] of var -2..255: key_value;" in mzn.model_variables
assert "array[0..15] of var 0..3: plaintext_active;" in mzn.model_variables
assert "array[0..15] of var -2..255: plaintext_value;" in mzn.model_variables

Reason: input_wordwise_deterministic_truncated_xor_differential_constraints emits these wordwise declarations. They follow the exact same migration path — out of _model_constraints, into _variables_list — and the two prefix items disappear from the combined count.

The commented-out twin test (lines 22-59) was left untouched because it is
inactive.


Source-code fixes uncovered while running the test suite

The test run also exposed three production bugs that the old prefix-baking
behaviour was hiding. Each one is included in the same PR because the test
suite cannot pass without them.

claasp/cipher_modules/models/cp/mzn_model.pysolve_for_ARX

Change: added prefix=list(self._model_prefix) to the MiniZincModelParts
that solve_for_ARX passes to assemble_model.

 mzn_model_string = self.assemble_model(
     MiniZincModelParts(
+        prefix=list(self._model_prefix),
         variables=list(self._model_constraints),
         constraints=list(self._variables_list),
     )
 )

Why the test failed: solve_for_ARX builds an explicit MiniZincModelParts instead of going through current_model_parts(). Before the refactor, the helper-defining block (MINIZINC_USEFUL_FUNCTIONS, which
defines modadd_linear and friends) was visible to it because every model baked _model_prefix into _model_constraints. The refactor removes that bake, so _model_constraints no longer carries the helpers and the explicit-parts call has to ask for _model_prefix directly. Without that line mzn_xor_linear_model_test.py::test_find_one_xor_linear_trail_with_fixed_weight_solve_with_api
fails with MiniZincError: type error: no function or predicate with name 'modadd_linear' found.

claasp/cipher_modules/models/cp/mzn_model.pywrite_minizinc_model_to_file

Same change for the same reason — the writer constructs an explicit
MiniZincModelParts and was relying on the prefix being baked into
_model_constraints. After the refactor the writer must request the prefix
explicitly.

claasp/cipher_modules/models/cp/mzn_models/mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model.pyinput_xor_differential_constraints override

Change: wrap the constraint string in a list before returning it.

 cp_declarations, cp_constraints = super().input_xor_differential_constraints()
 table = "++".join(self._table_items)
-cp_constraints = f"constraint table({table}, {self.cipher_id}_table_of_solutions);"
+cp_constraints = [f"constraint table({table}, {self.cipher_id}_table_of_solutions);"]
 return cp_declarations, cp_constraints

Why the tests failed: the override returns a (declarations, constraints) tuple just like its parent — except it stuffs a single string into the constraints slot instead of a list. The old caller did self._variables_list.append(constraints), which appends the string as one list element, so the bug was invisible. The refactor switched the caller to self._model_constraints.extend(constraints), and list.extend("constraint …") iterates the string character-by-character, producing a model with c\no\nn\ns\nt\nr\na\ni\nn\nt … after the last real constraint. MiniZinc then rejects line 2027 with Error: syntax error, unexpected identifier, no solutions come back, and every mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.py test fails downstream with KeyError: 'solution1'.

Wrapping the string in a one-element list keeps the contract consistent with the parent (input_xor_differential_constraints returns (list[str], list[str])) and lets extend add one constraint, not 35
characters.

The five tests that started passing again after this fix are:

mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.py::test_find_all_xor_differential_trails_with_fixed_weight
mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.py::test_find_lowest_weight_xor_differential_trail
mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.py::test_find_one_xor_differential_trail
mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.py::test_find_one_xor_differential_trail_with_fixed_weight
mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model_test.py::test_solve_full_two_steps_xor_differential_model

Doctest changes

claasp/cipher_modules/models/cp/mzn_model.pymodel_constraints property docstring

Change:

-    sage: 'constraint rot_0_0[2] = plaintext[11];' == constraints[29]
+    sage: 'constraint rot_0_0[2] = plaintext[11];' in constraints
     True

Reason: before the refactor, _model_constraints started with 'include "globals.mzn";', MINIZINC_USEFUL_FUNCTIONS, and the input declarations from input_xor_differential_constraints. The constraint 'constraint rot_0_0[2] = plaintext[11];' therefore landed at index 29. After the refactor _model_constraints contains only constraints, so the index of any specific constraint shifts. Switching to a membership check matches the property's documented contract ("only the constraints, excluding variable declarations") and is robust to future ordering changes.

The model_variables doctest at line 1181 (variables[0] == 'array[0..15] of var 0..1: pre_modadd_0_1_0;') was left as-is. Component declarations are still appended to _variables_list first by build_xor_differential_trail_model_template; the input-declaration lines from input_xor_differential_constraints are appended after them, so index 0 is unchanged.


Net effect on the combined count

For every "build_*" test that asserts
len(model_constraints) + len(model_variables), the count drops by exactly 2
because:

OLD layout:
  _model_constraints = dedup(_model_prefix + set_of_constraints)
                      = (2 prefix items) + (input declarations) + (real constraints)
  _variables_list    = (component declarations) + (input constraints, wrong place)

NEW layout:
  _model_constraints = dedup(set_of_constraints)
                      = (real constraints) + (input constraints, correctly here)
  _variables_list    = (component declarations) + (input declarations, correctly here)
  _model_prefix      = (2 prefix items) — surfaced via current_model_parts()

The prefix items move out of model_constraints and out of the combined count. Everything else is conserved (same total of declarations, same total of constraints). That is why every count assertion above goes down by exactly 2.

juaninf added 3 commits April 29, 2026 05:19
  After the upcoming source refactor splits prefix/variables/constraints
  into their semantic attributes, several tests that depended on the old
  prefix-bake stop holding.

  Pytest changes
  - Index-based reads of model_constraints[i] that targeted declarations
    switched to membership checks against model_variables (impossible,
    hybrid impossible, deterministic, wordwise builder tests).
  - len(model_constraints)+len(model_variables) totals decrement by 2
    per affected test -  the two prefix items no longer appear in
    model_constraints: 1764->1762, 1661->1659, 2442->2440, 438->436,
    1361->1359.
  - mzn_model_test.test_build_generic_cp_model_from_dictionary_xor_linear
    rewritten to call _variables_list.extend(declarations) and
    _model_constraints.extend(constraints) directly instead of the old
    prefix-bake pattern.
  - test_assemble_model_preserves_legacy_order renamed to
    test_assemble_model_orders_variables_constraints_outputs and clears
    _model_prefix so its narrow ordering check still holds; the
    "legacy" framing no longer matched what the test actually verifies.

  Doctest changes
  - mzn_model.py model_constraints property: switched the
    `constraints[29]` index check to a membership check, since
    model_constraints no longer starts with the include + helpers block.

  See CP_MINIZINC_TEST_CHANGES_EXPLANATION.md for the per-test rationale.
  Builders now place each MiniZinc fragment in the attribute that matches
  its semantic role:

    _model_prefix         → only includes + helper predicates
    _variables_list       → only variable declarations
    _model_constraints    → only constraints + solve directive
    mzn_output_directives → only output directives

  Previously declarations from `input_*` methods were stuffed into
  _model_prefix and constraints from those methods landed in
  _variables_list. Each builder then baked the prefix into
  _model_constraints with `_model_constraints = _model_prefix +
  _model_constraints` so the assembled model came out right by accident.

  Changes
  - MznModel.current_model_parts() now exposes
    `prefix=list(self._model_prefix)` so assemble_model() picks it up
    automatically; builders no longer need the prefix-bake.
  - Every CP builder updated to put declarations in _variables_list and
    constraints in _model_constraints directly (mzn_cipher_model,
    mzn_xor_differential_model, mzn_xor_linear_model,
    mzn_deterministic_truncated_xor_differential_model,
    mzn_semi_deterministic_truncated_xor_differential_model,
    mzn_differential_linear_model, mzn_impossible_xor_differential_model,
    mzn_hybrid_impossible_xor_differential_model,
    mzn_xor_differential_trail_search_fixing_number_of_active_sboxes_model).
  - Custom solve assemblies (impossible, hybrid impossible, two-step
    trail search) now include _model_prefix explicitly since they bypass
    assemble_model().
  - solve_for_ARX and write_minizinc_model_to_file pass
    prefix=list(self._model_prefix) in their explicit MiniZincModelParts;
    without it the helper block (modadd_linear, etc.) was missing once
    the prefix-bake was removed.
  - Trail-search override of input_xor_differential_constraints now
    returns the table constraint wrapped in a list, matching its parent
    contract. The override previously returned a bare string, which the
    old caller append()-ed as one item; the new caller uses extend(),
    which would otherwise iterate the string character by character.
@juaninf juaninf changed the title Refactor/cp mzn models fulfill model parts Organize CP MiniZinc parts with their respective meanings Apr 29, 2026
juaninf and others added 5 commits April 29, 2026 13:28
@juaninf juaninf marked this pull request as ready for review April 29, 2026 16:53
@juaninf juaninf requested a review from peacker April 29, 2026 16:53
@sonarqubecloud
Copy link
Copy Markdown

@peacker peacker merged commit 9fdb415 into develop May 1, 2026
11 checks passed
@peacker peacker deleted the refactor/cp-mzn-models-fulfill-model-parts branch May 1, 2026 05:17
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.

2 participants