Add support for importing custom gates in Quirk URLS #2473
Conversation
- Define `CompositeCell` - Add abstract `with_qubits` and `gate_count` methods to `Cell` - Add `_replace_qubit[s]` utility methods to `Cell` - Included defenses against billion laughs attack, test that they work - Add QFT example test
| gate_count = sum(0 if cell is None else cell.gate_count() | ||
| for col in parsed_cols | ||
| for cell in col) | ||
|
|
There was a problem hiding this comment.
Can we raise ValueError for billion laughs here instead of l.208? Then we never have to instantiate a CompositeCell with dangerous cell structure and don't need the extra complexity to ensure lazy traversals.
There was a problem hiding this comment.
what's l.208? Later?
I tried doing this, and the runtime of the test to detect the attack increased from a fraction of a section to tens of seconds. The problem is that this approach performs O(MAX_OP_COUNT) work instead of O(INPUT LENGTH) work when defending against an attack, because the largest composite gate under the limit is still built.
There was a problem hiding this comment.
Sorry, by l.208 I meant line 208 above:
raise ValueError(
f'Quirk URL specifies a circuit with {comp.gate_count()} '
f'operations, but max_operation_count={max_operation_count}.')You're right, though about my suggestion above. It doesn't solve the problem.
The reason I raised that suggestion was that the current API of CompositeCell bothers me a little. Basically, the fact that we have to add the caution warnings in the docstring suggests that our abstraction isn't great.
WDYT about making the max operation limit a class property of CompositeCell and raising the exception in CompositeCell._transform_cells()? Then we could remove the caution warnings and the requirement that sub_cell_cols_generator be a generator (it could be any iterable whether fully materialized or not). We could also remove associated machinery to support the lazy processing, i.e. _iterator_to_iterable.
| gate_count = sum(0 if cell is None else cell.gate_count() | ||
| for col in parsed_cols | ||
| for cell in col) | ||
|
|
There was a problem hiding this comment.
Sorry, by l.208 I meant line 208 above:
raise ValueError(
f'Quirk URL specifies a circuit with {comp.gate_count()} '
f'operations, but max_operation_count={max_operation_count}.')You're right, though about my suggestion above. It doesn't solve the problem.
The reason I raised that suggestion was that the current API of CompositeCell bothers me a little. Basically, the fact that we have to add the caution warnings in the docstring suggests that our abstraction isn't great.
WDYT about making the max operation limit a class property of CompositeCell and raising the exception in CompositeCell._transform_cells()? Then we could remove the caution warnings and the requirement that sub_cell_cols_generator be a generator (it could be any iterable whether fully materialized or not). We could also remove associated machinery to support the lazy processing, i.e. _iterator_to_iterable.
I would prefer for it to be configurable at call time. I think the only proper way to remove the generator shenanigans is to split parsing into two phases: size estimation then circuit construction. That would be quite a lot of work. |
Strilanc
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
CompositeCellwith_qubitsandgate_countmethods toCell_replace_qubit[s]utility methods toCell