N1 full compute - #140
Open
BDonnot wants to merge 194 commits into
Open
Conversation
…lit apart in the majority of cases Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
…grid and pypowsybl Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
…nected component Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
BDonnot
commented
Jun 25, 2026
| def _olf_provider_params(): | ||
| """Return ``{name: set(possible_values) or None}`` for OpenLoadFlow, or | ||
| ``None`` if the installed pypowsybl does not expose the metadata.""" | ||
| global _OLF_PROVIDER_PARAMS |
Collaborator
Author
There was a problem hiding this comment.
Need to find an alternative if possible
| Explicit version: operates on the passed solver / control / accumulators so it | ||
| can run concurrently (one solver per thread). The member Bbus_ is read-only here. | ||
| **/ | ||
| bool BaseBatchSolverSynch::compute_one_powerflow( |
Collaborator
Author
There was a problem hiding this comment.
This function should be const now
| return conv; | ||
| } | ||
|
|
||
| bool BaseBatchSolverSynch::warmup_solver( |
Collaborator
Author
There was a problem hiding this comment.
probably const too now
| if(better){ best_strand = strand; best_weight = weight; best_bus = bus; } | ||
| } | ||
|
|
||
| // 4) reorder slack_ids_me_ so the chosen reference is index 0 (the NR uses |
Collaborator
Author
There was a problem hiding this comment.
Mark (if not already) a comment in the NR and DC to tell that they MUST keep the ref slack as the first slack otherwise this breaks (and I don't even want to start debugging it)
| } | ||
| } | ||
| // the system matrix actually handed to the linear solver | ||
| Eigen::SparseMatrix<real_type> & sys_mat = has_mask ? masked_mat : dcYbus_noslack_; |
Collaborator
Author
There was a problem hiding this comment.
const ref here too maybe ?
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
… cpp side Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
|
…security analysis Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
BDonnot
commented
Jul 3, 2026
| participate. An earlier version of this function restricted participants to | ||
| the country hosting the most buses; that was a guess, not something OLF | ||
| actually does, and it was found to materially skew which generators absorb | ||
| the slack mismatch on real multi-country RTE grids -- removed; |
Collaborator
Author
There was a problem hiding this comment.
Try to avoid any references to RTE grid and just keep "real" grids. I note it here but there are other places like this
BDonnot
commented
Jul 3, 2026
| Real full grids never have any dangling line -- they only appear when | ||
| zooming into a sub-area with | ||
| ``network.reduce_by_ids_and_depths(..., with_boundary_lines=True)`` | ||
| (*eg* in ``reduce_and_compare.py`` / ``validate_olf_vs_lightsim.py``), |
Collaborator
Author
There was a problem hiding this comment.
Do not mention these scripts directly. They are not part of lightsim2grid (yet?)
BDonnot
commented
Jul 3, 2026
| ``df_dl`` carries two extra columns, ``boundary_bus_id`` / | ||
| ``boundary_vl_id``, used later to attach the equivalent branch + load. | ||
| """ | ||
| df_dl = net.get_dangling_lines() |
Collaborator
Author
There was a problem hiding this comment.
If using net, and not net_pu the unit might be wrong.
It's best to rely on pypowsybl per unit system rather than to perform the per unit here (also valid for SVC)
Loads MATPOWER cases (.m via matpowercaseframes, .mat via scipy, or an already-parsed mpc dict/object) directly into LSGrid from the raw bus/gen/branch matrices, without ever constructing a pandapower net. This sidesteps a known bug in pandapower's own matpower converter that mishandles grids with several generators connected to the same bus: lightsim2grid.network.init_from_matpower keeps every mpc.gen row as an independent generator, including at the reference bus (distributed slack across co-located generators). Verified against the real 70,000-bus case_ACTIVSg70k.m (1944 buses with multiple generators, up to 17 on one bus): parses in ~1.8s and converts/powerflows correctly. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
- support mpc.dcline: HVDC lines are now converted via model.init_dclines, using the same loss/sign conventions already validated by the pandapower loader (PF is negated to match lightsim2grid's "received at side 1" convention, LOSS1 * 100 -> loss_percent, LOSS0 -> loss_mw). Optional: falls back to no HVDC line when mpc.dcline is absent. - remove the n_sub parameter: there is always exactly one substation per matpower bus. n_busbar_per_sub now accepts any value >= 1 (previously forced to 1); the extra busbar sections it creates are deactivated since matpower has no data to populate them with. - clarify the "several reference buses" warning mentions equal weighting. - add matpowercaseframes to requirements_compile_ci.txt so CI actually exercises the ".m" file loading path instead of silently skipping it. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
Some real grids have ill-conditioned clusters (high R, heavily loaded,
little local generation) where plain NR diverges and the existing step-
scaling policies (Iwamoto, MaxVoltageChange) only rescale the same bad
Newton direction rather than fixing it. LM regularizes the direction
itself by damping J's diagonal, with lambda adapted via an accept/reject
loop on the actual mismatch reduction of each trial step.
- Core: add NRSystem::update_trailing_feature_values(count, deltas), a
small, generic method that directly updates the last `count` declared
feature entries in J_.valuePtr(), bypassing fill_internal_variables()/
fill_J(). Not diagonal- or LM-specific; any extension needing a fast
partial refresh of its own trailing feature slots can use it.
- examples/lm_algorithm/: a new out-of-tree plugin, following the same
pattern as examples/dist_slack_algorithm/ and examples/external_algorithm/
(standalone CMakeLists.txt, AlgorithmRegistry self-registration, not
wired into the main build/CI).
- DiagonalDamping.hpp: DiagonalEntry, a minimal NRSystem extension that
reserves one feature slot per existing unknown (for the diagonal
load), plus with_diagonal_entry<NRSystem> to inject it as the last
extension of an existing SingleSlackNRSystem/MultiSlackNRSystem.
- LMNRAlgo.hpp/.tpp: the algorithm itself, a BaseAlgo sibling to NRAlgo
(not built on it, since the inner accept/reject loop doesn't fit
NRAlgo's single solve-per-iteration flow or its RefactorPolicy/
ScalingPolicy abstractions).
- LMNRAlgo_plugin.cpp + CMakeLists.txt: registers "NR_LM_SparseLU" and
"NR_LM_KLU".
- test_plugin.py: registration/load smoke test, same scope as the
existing external_algorithm one.
Verified: examples/lm_algorithm builds cleanly (incl. -Wall -Wextra) with
only the expected core symbols left undefined (Base/Hvdc/VoltageControl,
SparseLULinearSolver, AlgorithmRegistry), and the existing
dist_slack_algorithm example still builds against the modified NRSystem.
Signed-off-by: Claude <noreply@anthropic.com>
…gorithm-ffv04y Add Levenberg-Marquardt Newton-Raphson as an out-of-tree example plugin
Fix core/bindings -march=native ABI mismatch, add runtime ABI-drift guard
Follow-up to the const/const-ref sweep (#151), picking up member functions and return types the earlier pass missed. All changes are read-only markers or copy-enabling; no behavioural change. Verified: src/core builds and the C++ unit test suite passes (1201 assertions, 49 cases). const on read-only member functions (lets them be called on a const object / through a const ref, and documents intent): - LSGrid: get_bus1_powerline, get_bus2_powerline, get_bus1_trafo, get_bus2_trafo, get_Ybus_solver, get_dcYbus_solver - PandaPowerConverter: get_line_param, get_line_param_legacy, get_trafo_param_pp2, get_trafo_param_pp3, _check_init (whole class is read-only apart from the two setters) - BaseAlgo::is_linear_solver_valid (reads err_ only) drop top-level const on by-value return types (readability-const-return-type): returning `const T` by value blocks move construction / move assignment at the call site (e.g. the pybind copy-to-Python path) and buys nothing. Applied to the freshly-computed getters in LSGrid: get_Ybus, get_dcYbus, get_Sbus, get_dcSbus, get_pv[_numpy], get_pq[_numpy], get_slack_ids[_numpy], get_slack_ids_dc[_numpy], get_slack_weights, get_V, get_Va, get_Vm. misc: std::endl -> '\n' in GeneratorContainer::cout_v (avoid the per-line stream flush; performance-avoid-endl). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LY1xhzf8cUjqFoNNj7y2Gc Signed-off-by: Claude <noreply@anthropic.com>
Plugins previously self-registered from a static AlgorithmRegistrar constructor firing during dlopen(). Its two failure modes -- an ABI-tag mismatch and a duplicate solver name -- throw, and an exception escaping a static constructor while the dynamic loader runs its init routines unwinds into C loader frames with no handler and calls std::terminate(), aborting the interpreter uncatchably (ctypes cannot translate it either). Registration now runs from an explicitly-called entry point: - Plugins write a void(PluginRegistrar&) function and expose it with the new LS2G_PLUGIN_ENTRY macro, which generates an exported ls2g_register_plugin symbol (LS2G_PLUGIN_EXPORT handles dllexport/visibility). - load_algorithm_plugin() is now a pybind11 C++ function that dlopen/dlsym's the plugin and calls the entry point inside try/catch, so failures surface as catchable Python RuntimeError instead of terminating the process. A rejected plugin is dlclose'd. - AlgorithmRegistry::register_all() commits a whole batch atomically (copy + noexcept swap): ABI-checked once, all-or-nothing, strong exception guarantee, so a plugin exposing several solvers can never half-register. - The entry-point helper is noexcept and copies diagnostics into the caller's buffer with an explicit, overflow-proof bounded write (null/zero-length safe). Tests: - src/tests/test_plugin_registration.cpp (Catch2, runs under valgrind in the cpp_unit_tests workflow): atomic rollback, ABI rejection, entry-point return codes vs exceptions, and short/null error-buffer handling. - lightsim2grid/tests/test_plugin_registration.py + a new plugin_registration CI workflow: build the example plugin against the installed package and load it for real, asserting bad loads raise (and do not crash) the interpreter. The three example plugins and docs/solver_plugin.rst are updated to the new mechanism; load_algorithm_plugin() now raises RuntimeError (not OSError) and rejects loading a plugin whose name is already registered (incl. loading the same plugin twice). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J97bpvJk9BS1qXnGPGYnGp Signed-off-by: Claude <noreply@anthropic.com>
Follow-up to the plugin-registration rework: the two example test_plugin.py scripts still called the deprecated available_solver_names() alias and had a couple of stale references (an "external_solver" build path that should read "external_algorithm", and a "change_solver(...)" log line for a change_algorithm(...) call). Switch to available_algorithm_names() and fix the wording so the examples match the current API. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J97bpvJk9BS1qXnGPGYnGp Signed-off-by: Claude <noreply@anthropic.com>
lightsim2grid/tests/test_solver_registry.py::TestPluginLoading was silently skipping instead of exercising the plugin: it looked for the example under examples/external_solver/ (the directory is examples/external_algorithm/) and imported a load_solver_plugin symbol that does not exist (it is load_algorithm_plugin). Run against the pip-installed package its relative path could not reach the source-tree examples/ either. - Fix the directory name and the import, and make the plugin path resolvable via LS2G_TEST_PLUGIN / LS2G_TEST_PLUGIN_DIR so it works against the installed package (whose __file__ is in site-packages). The plugin load is now idempotent w.r.t. other test modules in the same process, since loading a plugin whose solver name is already registered now raises. - Apply the same env-based finder to test_plugin_registration.py. - Run both modules in the existing main.yml `test_plugin_against_installed` job (ubuntu/windows/macos), which already builds the plugin after installing lightsim2grid -- so TestPluginLoading genuinely runs, and the crash-safety tests get cross-platform coverage (incl. the Windows LoadLibrary path). - Drop the now-redundant standalone plugin_registration.yml workflow; its test run moved into the job above. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J97bpvJk9BS1qXnGPGYnGp Signed-off-by: Claude <noreply@anthropic.com>
Address two review points on load_algorithm_plugin_impl's diagnostic buffer: - Name the 512 magic number: `static const std::size_t max_errbuf_size = 512`. - Guarantee the host can never over-read the buffer, by design and independently of the plugin: after calling the plugin entry point, force a NUL into the last slot (errbuf[max_errbuf_size - 1] = '\0') before reading it as a C string, so even a plugin that ignores the size contract cannot make the read run past the buffer. Refactored the load so the entry call + buffer handling happen exactly once (via an `unload` closure) instead of being duplicated per platform. The write side is already bounded by construction for plugins using LS2G_PLUGIN_ENTRY (ls2g_run_plugin_entry's write_err respects errbuf_len). Add the explicit test the reviewer asked for: a >512-char solver name driven through the real entry helper with the same 512-byte buffer the host uses, asserting truncation + in-bounds NUL termination (and valgrind-verified as out-of-bounds-write-free in the cpp_unit_tests workflow). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J97bpvJk9BS1qXnGPGYnGp Signed-off-by: Claude <noreply@anthropic.com>
…stration-00qm6e Make solver-plugin registration exception-safe at load time
Release wheels are built -O3 -DNDEBUG, so Eigen and std bounds checks are compiled out. Index fields carried by the grid (element bus ids, substation ids, position in the topology vector, generator slack and remote-regulated bus references) are used to index internal vectors on the powerflow and topology-update hot paths without range checks. set_state() (used by both the pickle and the fast binary format) previously validated only array lengths, not values -- so a well-formed but semantically poisoned state (e.g. an out-of-range bus id) loaded cleanly and then caused an out-of-bounds read/write on the next powerflow. Add a whole-grid validator: - GenericContainer gains a virtual check_valid() (default no-op) and a static check_all_finite() helper (real and complex). - OneSideContainer / OneSideContainer_PQ / GeneratorContainer / TwoSidesContainer / TwoSidesContainer_rxh_A override check_valid() to range-check bus_id / subid / pos_topo_vect, slack weights and regulated_bus_id, and to reject NaN/Inf in the electrical inputs (r, x, half-line shunts, thermal limits, p/q/vm/min_q/max_q targets). subid and pos_topo_vect are optional and only checked when populated. - LSGrid::check_grid() drives the per-container checks with the grid-wide bus/substation totals, then verifies the collected pos_topo_vect entries form a permutation of [0, dim_topo) (in range + unique). check_grid() is called at the end of LSGrid::set_state(), so loading a pickle or binary file that is well-formed but inconsistent now raises a clean std::out_of_range / std::runtime_error instead of causing an out-of-bounds access later. It runs in O(number of elements), off the solver hot path. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Bind LSGrid::check_grid() as GridModel.check_grid() (with a docstring in the DocLSGrid help namespace), and call model.check_grid() at the end of each grid loader -- from_pandapower, from_powermodels (which also covers matpower, routed through it) and from_pypowsybl -- right after the grid is built. set_state() already validates grids loaded from a pickle or the binary format; this covers the construction path (the loaders build via init_*, not set_state) and surfaces a format-specific error early when a source grid is inconsistent. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
A converged solve reads V/Va/Vm back from the algorithm and indexes them, without a size or finiteness check, in compute_results() and _get_results_back_to_orig_nodes(). Built-in solvers always honour the contract, but a plugin / custom solver reached through the same BaseAlgo interface may not: a wrong-sized V would cause an out-of-bounds read, and non-finite values would silently propagate NaN/Inf into every element result. Add LSGrid::_check_solver_output(), called at the top of process_results() right after a solve reports convergence: - V/Va/Vm not sized to the solver's bus count = a broken contract -> throw std::runtime_error naming the algorithm (catchable in Python); - non-finite values -> mark the solve non-converged (ErrorType::InifiniteValue) and return false, so results are reset instead of propagating NaN. A well-behaved solver already reports this itself; this covers the ones that do not. Adds a small BaseAlgo::set_error() (forwarded by AlgorithmSelector) so the non-convergence can be recorded. The check is O(nb_bus) once per solve and covers built-in and plugin solvers alike. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
New test_check_grid.cpp (Catch2), wired into the lightsim2grid_unit_tests binary so it runs under ctest and valgrind (cpp_unit_tests workflow): - a valid grid, and its get_state/set_state round-trip, are accepted; - a state poisoned with an out-of-range or negative bus id, a connected element on the -1 bus, a slack generator with a non-positive weight, an out-of-range remote-regulated bus id, or a NaN physical value is rejected by set_state() with std::out_of_range / std::runtime_error -- never a crash; - pos_topo_vect is accepted as a valid permutation and rejected when duplicated or out of range; - a solver returning a wrong-sized voltage vector makes the powerflow throw (instead of an out-of-bounds read), and one returning non-finite voltages is reported as non-converged (empty result), with no NaN leaking out. Running the whole binary under valgrind is what proves the poisoned inputs cause no out-of-bounds access. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
New lightsim2grid/tests/test_check_grid.py, added to the module list run by the asan_ubsan job in .github/workflows/sanitizers.yml so it executes under AddressSanitizer + UndefinedBehaviorSanitizer: - a real pandapower grid (case14) loads and passes check_grid() -- the loader calls it, so a false positive would make the loader itself raise; - check_grid() is reachable from Python and rejects a duplicated, out-of-range or negative topology-vector position with a clean IndexError / RuntimeError; - a valid grid round-trips through pickle (which runs check_grid on load). The deep set_state value-poison cases (bus / substation ids, slack weights, NaN inputs) are covered by the C++ suite, which also runs under valgrind. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Add docs/security.rst (linked from the technical-documentation toctree): a "Security and trust boundaries" page stating plainly that unpickling a grid and loading a solver plugin are trusted-input-only by design (arbitrary code execution), while the binary format is hardened against untrusted input and validated with check_grid() on load. It documents check_grid() as public API and the solver-output contract for plugin authors. Update CHANGELOG.rst with the check_grid() additions, the automatic validation on load / in the loaders, the solver-output sanity check, and the new security page. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
test_LSGrid_out_of_bounds.py is an existing memory-safety regression test (out-of-range element ids / mis-shaped arrays must raise a clean IndexError / RuntimeError instead of reaching unchecked C++ indexing). Add it to the module list the asan_ubsan job runs, so it executes in the environment that would actually catch an out-of-bounds access. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
check_grid() now runs when a grid is loaded from the binary format (via set_state), and it raises IndexError (C++ std::out_of_range) for an out-of-range bus / substation / topology-vector index. TestCorruptionSweep in test_binary_serialization.py only accepted RuntimeError, so a byte corruption that produced a byte-wise well-formed but semantically inconsistent grid (an out-of-range index) made load_binary raise IndexError and failed the sweep. Keep the clearer IndexError contract for out-of-range indices and widen the sweep instead: TestCorruptionSweep._check_load now accepts RuntimeError (byte layer) OR IndexError (semantic layer, check_grid) as a clean rejection, and still fails on anything else (a crash or a different exception). A local byte-flip sweep over a saved grid gives 1335 RuntimeError, 32 IndexError and 0 disallowed outcomes. Document the semantic-validation layer in docs/binary_serialization.rst and in the load_binary docstring (binary_helpers.hpp): load_binary now also rejects a well-formed-but-inconsistent grid via check_grid. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
check_grid() rejected any NaN / Inf in the physical input arrays (r, x, half-line shunts, thermal limits, p/q/vm/min_q/max_q). That produced false positives on legitimate grids: pypowsybl IEEE cases (ieee14/30/57/118) encode "no thermal limit" as a non-finite limit_a_ka, and unbounded generator reactive limits are commonly stored as +/-Inf. init_from_pypowsybl therefore raised on every one of those grids (CI: test_legacy_pypowsybl and the full test suite). Finiteness of these floats is not a memory-safety concern -- they are not used as indices, so a NaN/Inf cannot cause an out-of-bounds access; it only yields a NaN result, which the solver-output check at the algorithm->grid seam already turns into a reported non-convergence. Remove the input-finiteness checks and keep check_grid focused on index-safety (bus / substation / topology-vector indices, slack and remote-regulated bus references), which is the actual OOB guard. The generator slack-weight finiteness check stays (a weight is never legitimately non-finite), as does the solver-output finiteness check. Removes the now-unused GenericContainer::check_all_finite helper, the OneSideContainer_PQ / TwoSidesContainer_rxh_A check_valid overrides (they only added finiteness; index checks are inherited), the NaN test case, and updates the docs/CHANGELOG accordingly. Verified: pypowsybl ieee14/30/118 and pandapower / grid2op grids all load; C++ suite (70) + valgrind clean. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Review feedback from PR #155: - Only run the solver-output sanity check for *external* solvers. This was the reviewer's question ("is it possible to run this check only for external plugins?") and the answer is yes, for free: AlgorithmType::Custom already means "not one of the built-in solvers" (name_to_algo_type returns it for any name outside the built-in set), so process_results now gates the check on it. Built-in solvers, which the test suite already covers, pay nothing. - docs/security.rst: * grid2op environments are a code-execution channel too (grid2op reads and executes python shipped with the environment): add them to the table, to the section title and to the rule ("do not load a pickle or a plugin, nor call grid2op.make, on something you do not trust"); * mention grid2op among the ways a grid gets built and validated; * describe the binary format as the "least dangerous" channel rather than "the channel to use for untrusted data", with an explicit warning that least dangerous is not safe -- loading untrusted data stays a bad idea; * drop the deprecated GridModel alias, LSGrid is the current name; * "relatively cheap" rather than "cheap". - Add substation-id (*_to_subid) tests mirroring the pos_topo_vect ones: out-of-range, negative, off-by-one (n_sub itself) and a valid mapping, for both loads and generators. - Make the external-solver coupling explicit in the C++ seam tests (assert the test solver is AlgorithmType::Custom). Verified: C++ suite (70) + valgrind clean; 376 python tests pass across test_check_grid, test_LSGrid_out_of_bounds, test_ACPF, test_DCPF, test_solver_control, test_solver_registry and the two pypowsybl suites. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
`refactor_every_n` is used as `iter % refactor_every_n_` in NRAlgo::should_refactor_policy, but nothing validated it. Setting it to 0 therefore caused an integer division by zero: a SIGFPE that kills the interpreter instead of raising a catchable error. Two ways to reach it, both confirmed (exit code 136): # 1. the python solver API (lightsim2grid.algorithm, ie the newtonpf entry point) solver.set_refactor_policy(RefactorPolicyType.EveryN) solver.set_refactor_every_n(0) solver.solve(...) # -> SIGFPE # 2. the serialized state: AlgoConfig is part of LSGrid::StateRes, so the # value survives a pickle / binary round-trip (check_grid does not cover # it -- it is not an index) and crashes on the next powerflow grid.set_ac_algo_config(poisoned); pickle.loads(pickle.dumps(grid)).ac_pf(...) Validate the NR policy parameters where they enter, so both paths are covered: set_refactor_every_n() requires >= 1, and set_config() -- the deserialization path, fed by an AlgoConfig that can come straight from a file -- runs the same checks instead of assigning raw. Also validate the other policy parameters whose contract is unambiguous: ls_c / ls_rho in (0, 1), ls_max_iter >= 0, max_dVa / max_dVm finite and > 0, iw_mu_min / iw_mu_max finite. Tests: C++ cases feeding a poisoned AlgoConfig through set_state (0, negative, out-of-range ls_rho, non-finite max_dVa) plus a valid round-trip, and python cases for the setters and for set_ac_algo_config. Existing callers all use sane values, so nothing else changes. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
A grid records which AC / DC algorithm it was using. That was stored as an
AlgorithmType enum, and every external (plugin) solver collapses onto the
catch-all AlgorithmType::Custom, so the plugin's identity was lost at save
time. The consequences were worse than "the plugin is missing":
* get_state() silently succeeded, producing a file that could NEVER be
loaded again -- even with the plugin loaded, since the name was gone;
* set_state() always threw "AlgorithmType::Custom is not a concrete
solver; use the string-based change_algorithm(name) overload", a message
aimed at a library developer, not at whoever saved the grid;
* copy() threw the same way, so a grid using a plugin could not be copied
at all.
Store the registry *name* instead (StateRes now holds two std::string), and
restore by name. AlgorithmSelector keeps the name it was selected with
(get_name()), which the enum cannot express for plugins. copy() goes through
the name too. This changes the serialized layout, so BINARY_FORMAT_VERSION is
bumped 3 -> 4 and the reference fixture is regenerated (it now stores
'NR_SparseLU', making it loadable by builds without KLU as well).
When the saved solver is not registered on load, the error now names it,
distinguishes a plugin from a built-in needing an optional backend
(KLU / NICSLU / CKTSO), says what to do about each, and lists the solvers that
are available. The name read from the file is escaped with printable() (now
exported from BinaryArchive.hpp) before being embedded in the message:
a corrupted one holds arbitrary bytes, which made pybind11 raise
UnicodeDecodeError instead of the intended RuntimeError -- caught by the
existing corruption sweep.
Add LSGrid.load_binary_without_algorithm(path) (set_state gains a
`restore_algorithm` flag): loads the grid data and keeps the default solvers,
so a grid saved with an unavailable solver is still usable. Every other check
(format, corruption, check_grid) is applied identically.
Document that the solver is persisted *by name only*: a name is not a strong
identity, so two plugins sharing a name silently substitute for each other,
and plugin authors should prefix their solver names.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
A solver name is an identity, not a label: it is written into every serialized
grid and is what re-selects the solver on load. Restrict it to
first character : A-Z a-z _
other characters : A-Z a-z 0-9 _ .
length : 1 to 64 characters
(ls2g::is_valid_solver_name, in the installed AlgorithmRegistry.hpp).
Why each part:
* no non-ASCII -- otherwise a plugin can register a name visually identical
to a built-in one ("NR_SparseLU" with a Cyrillic 'a') and masquerade as it
in every error message, doc and available_algorithm_names() listing while
being a different registry key;
* no control characters -- names are printed into error messages and logs,
where a newline allows log injection and an ESC byte terminal-escape
injection;
* bounded length -- bounds what goes into a file, into an error message and
into the diagnostic buffer a plugin is handed;
* '.' allowed (not first) for namespacing ("acme.NR_fast"), the recommended
way to avoid the name collisions documented last commit; digits allowed
(not first) so "solver_v2" works without a name looking like a number.
The set is deliberately minimal -- a whitelist can be widened later without
breaking anyone, but not narrowed. Hyphen is left out: '_' covers the need.
Enforced where names enter: AlgorithmRegistry::register_solver and
register_all (so a bad plugin name fails at load_algorithm_plugin() time, and
register_all still rolls the whole batch back), and LSGrid::set_state, where a
name that cannot even be a solver name means the state is corrupted and is
reported as such rather than sending the reader after a plugin that never
existed. Registry messages now escape the offending name with printable(),
which moves from BinaryArchive to Utils so both can use it.
Every existing name complies: the 25 built-ins, the 5 names the shipped example
plugins register, and every name used in the tests. One test is adapted:
test_plugin_registration's overflow case registered a 1000-character name,
which is now refused -- it exercises the rejection path instead, and asserts
the bounded, NUL-terminated diagnostic directly (the message no longer
necessarily fills the buffer, since printable() truncates the name).
Verified end to end with the shipped example plugin: it builds, loads,
registers 'DummyExternal', and a grid using it saves, reloads (with and
without the algorithm), pickles and copies.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
The copy test only asserted the copied grid's solver *name*. That is a stored
string, so it would still have passed if copy() had carried the name across
without building the solver. Assert what actually matters:
* the copy reports the plugin solver (name + AlgorithmType::Custom);
* it holds a live instance of it -- proven by running a powerflow on the
copy, which only converges if the registry factory was actually called;
* the copy is independent: re-selecting on the copy leaves the original on
the plugin solver.
Also cover the DC side, which was untested: LSGrid::change_algorithm routes on
BaseAlgo::IS_AC, so a DC plugin lands in the other selector and needs copy()
and get_state/set_state to carry *that* one by name. Adds DcPluginLikeAlgo
(IS_AC == false, implements compute_pf_dc) and tests copy + state round-trip
for it, including a DC powerflow to prove the restored instance is live.
Checked these are not vacuous: reverting copy() to the previous enum-based
version makes both copy tests fail, and they pass again on the fix.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
…ation-oahhjd core: const-correctness and copy-elimination low-hanging fruit
…7p8lbd Harden grid loading: whole-grid validation, solver-output checks, trust-boundary docs
Audit of every entry point that consumes data from outside the C++ core (python bindings, pickle, the binary format, the newtonpf/solver entry point), in the spirit of the refactor-every-n SIGFPE fix. Seven issues, all confirmed with a proof of concept before and after (SIGSEGV / "malloc(): unsorted double linked list corrupted" turning into a clean exception). Release wheels are -O3 -DNDEBUG, so neither Eigen's nor the standard library's own bounds checks are there to catch any of these. SubstationContainer::set_state did no validation whatsoever, and it restores the root of the grid's index space: nb_bus() is the bound check_grid() validates every element bus id against, bus_status_ is the vector those same ids then index, and n_sub_ is both the substation-id bound and a modulo divisor. A file declaring 4000 buses with a 1-entry bus_status passed check_grid() and corrupted the heap on the next init_bus_status(). n_sub_ == 0 reached `bus_id % n_sub_` (SIGFPE), and n_sub_ * nmax_busbar_per_sub_ could overflow the int holding it. set_ls_to_orig_internal sizes the reverse mapping from lpNorm<Infinity>() -- the maximum *absolute* value -- then indexes it with the values themselves, so an entry of -5 sized the vector from 5 and wrote at index -5. Reachable from the _ls_to_orig property, a pickle and a binary file alike. _init_kwargs is serialized as two parallel vectors with independently stored lengths; set_state walked the keys and indexed the values with the same counter, over-reading a vector<string>. SubstationInfo read sub_names_[id] bounded only against the substation count, but the names are optional and the pandapower / matpower / powermodels loaders never set them: iterating get_substations() on any such grid segfaulted, with no crafted input at all. update_topo and update_slack_weights never checked the length of the arrays handed to them, though both index them (by topology position and by generator id) with unchecked Eigen operator(). Also: an out-of-range policy enum in an AlgoConfig is now rejected rather than silently falling into a default: branch and round-tripping back out, set_config validates before mutating anything, and set_orig_to_ls now actually builds the inverse mapping it claims to. check_grid() covers the substation container and the bus-id mapping vectors, which it previously ignored; docs/security.rst says so. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015CfJVFAd2ifp4qyZxp5Ec5
Follow-up on the substation container, prompted by the question "shouldn't sub_vn_kv be set whenever bus_vn_kv is?". It is not, and cannot be made mandatory: sub_vn_kv_ is only ever filled by init_sub() or the two-argument constructor, neither of which is called anywhere. The live path is the default constructor followed by init_bus(), which fills bus_vn_kv_ only -- so sub_vn_kv is empty on every grid produced by init_from_pandapower / _pypowsybl / _matpower / _powermodels, on every already-saved binary file, and on this repo's own format-4 compatibility fixture. Requiring it would reject all of them. The field stays optional and now says so where it is declared. What that question did surface: the invariant it appeals to -- every busbar of a substation carries that substation's nominal voltage -- was never actually enforced. A gridmodel bus id is `sub_id + (local_bus_id - 1) * n_sub` and LocalBusId runs 1..nmax_busbar_per_sub, but init_bus's check looped local ids [1, nmax_busbar_per_sub): it opened by comparing the reference bus with itself and stopped one busbar short. At the usual nmax_busbar_per_sub == 2 it compared bus sub_id with bus sub_id and checked nothing whatsoever. Fixed to run local ids 2..nmax, and lifted into check_valid() so it is re-checked on load -- set_state bypasses init_bus entirely, so a pickle or a binary file could always carry a grid violating it. When sub_vn_kv IS present it is now checked against bus_vn_kv too. Separately, init_sub() itself wrote n_sub values into sub_vn_kv_ with an unchecked operator[] without ever sizing it -- an out-of-bounds heap write on the only construction path that exists. Latent (it is uncalled and not bound to python) but wrong for whoever wires it up next, so it now sizes its destinations. reset_bus_status() likewise iterates the vector it writes rather than the separately-stored n_bus_max_. Verified: the violating grids are now rejected, and case14 / case118 / case300 via pandapower, ieee14 via pypowsybl and the shipped format-4 fixture all still pass check_grid(). C++ suite 108 cases green, [check_grid] clean under valgrind (0 errors), and the python run shows the same 123 pre-existing environment failures as the unmodified tree -- no new ones. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015CfJVFAd2ifp4qyZxp5Ec5
…b_vn_kv
Rewrite check_bus_vn_kv_uniform_per_sub as the plain index relation, with no
LocalBusId / local_to_gridmodel indirection:
for all 0 <= I < n, for all 0 <= j < k: bus_vn_kv[I] == bus_vn_kv[I + j*n]
(n = n_sub, k = nmax_busbar_per_sub). Same checks as before, stated directly.
The stride is n: consecutive entries of bus_vn_kv are different SUBSTATIONS,
which is why the shipped case14 fixture legitimately has bus_vn_kv[4] == 138
next to bus_vn_kv[5] == 20 -- a stride of 1 would reject every grid with more
than one voltage level. The comment records why the old formulation went
wrong, so it does not come back.
Also document sub_vn_kv_ as the dead state it is, with a TODO on the member
and an entry in the CHANGELOG's TODO section: neither of its two writers
(init_sub, the two-argument constructor) is called anywhere, nothing reads it
back, so it is empty on every grid every loader builds and in every binary
file saved so far -- yet it is serialized into all of them. It is also
redundant, being bus_vn_kv restricted to the first n_sub ids, which
check_valid() now enforces when it is present. Either drop it from StateRes
(needs a format bump) or have init_bus fill it and require it (changes what
newly-saved files contain); both notes say so, so the choice is not lost.
Verified: violating grids rejected at k == 2 and k == 3; case14 / case118 /
case300 (pandapower), ieee14 (pypowsybl) and the format-4 fixture all still
pass check_grid(). C++ 108 cases green, [check_grid] clean under valgrind
(0 errors), python run shows the same 123 pre-existing environment failures
as the unmodified tree and no others.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015CfJVFAd2ifp4qyZxp5Ec5
Decide "built-in or plugin?" with the registry instead of AlgorithmType. AlgorithmType is a fixed enum of serialized solver identities, and a built-in only appears in it if a member was added for it -- the NRRefactorRetry_KLU/_NICSLU/_CKTSO family never got one, so name_to_algo_type() reported those three in-tree solvers as AlgorithmType::Custom exactly like a plugin, and they were paying for the external-solver output check (voltage size + finiteness) on every single solve, which built-ins are explicitly supposed to cost nothing. The registry now records a SolverOrigin at registration time, where the answer is actually known; External is the default, so a solver registered without saying anything stays validated. AlgorithmSelector caches the flag on change_algorithm so the hot path is a bool read, and every built-in registration goes through one tagging helper, so a solver added later cannot be silently misclassified by forgetting the tag. Check that a nominal voltage is finite and strictly positive -- for bus_vn_kv / sub_vn_kv only, never bus_vmin_kv / bus_vmax_kv, which use NaN as the documented "no limit set" sentinel (the same distinction that made the earlier element-level finiteness checks false-positive on real grids). Give init_bus and init_sub the overflow-safe n_sub * nmax_busbar_per_sub they were missing: all three entry points, set_state included, now share a checked_nb_bus helper that multiplies in 64 bits and refuses a product that does not fit the int it is stored in and indexed with. Drop the noexcept on set_ls_to_orig_internal: it assigns one Eigen vector and allocates another, either of which throws std::bad_alloc, and a throw out of a noexcept function is std::terminate() rather than something a caller can handle. Nothing needed the guarantee. Restore the "check sizes / TODO dev switches" comment in SubstationContainer::set_state, on the size checks it refers to, and lowercase the loop variable in check_bus_vn_kv_uniform_per_sub. Verified: NaN / 0 / negative vn_kv and the overflowing product are rejected; case14 / case118 / case300 (pandapower), ieee14 (pypowsybl) and the format-4 fixture all still pass check_grid(). C++ 110 cases green, [check_grid] clean under valgrind (0 errors), python run shows the same 123 pre-existing environment failures as the unmodified tree and no others. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015CfJVFAd2ifp4qyZxp5Ec5
…dit-xtmd23 Fix memory-safety holes on the untrusted-input paths
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
2 participants
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.
No description provided.