util/render: two reproduced value-type defects that ship by default — Quantity provenance chains and unvalidated locale group-separator stripping

[Yaraslaut]

Consolidates #538 (F20) and #539 (F21) from the #518 sweep. Both are closed in favour of this one. Both are reproduced, both are defects in shipped behaviour, and both are small enough to land in one PR.

They are merged because they are the same architectural mistake in two value types: the type silently produces a result instead of reporting a fact to the layer that owns the policy — invariant 3 of the triage skill. One returns a wrong number, the other returns an unbounded structure. Neither gives the caller a way to find out.

Part A — Quantity provenance is on by default: 5.5x slower, 6.9x memory, stack overflow in Debug

MORPH_QUANTITY_PROVENANCE defaults to 1 (quantity.hpp:38-40). Every leaf construction and every binary operation allocates an ASTNode (sizeof == 200) holding shared_ptrs to its operands (quantity.hpp:833-839), so the ordinary running-total pattern builds a linear chain that is never collapsed:

Quantity<eur> total{Rational{0, DecimalPlaces{2}}};
for (int i = 0; i < n; ++i) total = total + one;    // n retained ASTNodes

Reproduced. Revision: master @ 4017228d. n = 200,000:

build	max RSS	wall
MORPH_QUANTITY_PROVENANCE=1 (default), -O2	54,156 KB	0.033 s
MORPH_QUANTITY_PROVENANCE=0, -O2	7,884 KB	0.006 s

Destruction is recursive (~shared_ptr -> ~ASTNode -> ~shared_ptr -> ...), and at -O0 it segfaults:

$ ./prov0 200000
total = 200000EUR (chain of 200000 provenance nodes retained)
destroying...
$ echo $?
139                 # SIGSEGV, stack overflow in the destructor chain

At -O2 clang tail-call-optimises the chain away, so this crashes in Debug and survives in Release — the worst possible signature for finding it.

Not verified: the exact depth at which the Debug crash begins was not determined, nor whether equation() (quantity_equation.hpp:91-110, :163-210) overflows at the same depth. It recurses over the same DAG, so it should, but it was not measured.

Why it matters: 5.5x slower and 6.9x the memory for a loop that adds integers, by default, in a framework whose Quantity sits in model state and on the wire path. If the loop bound comes from wire input — a ledger replay, a batch of rows — this is a remote memory-exhaustion and crash vector.

Fix, in order of value

1. Flip the default to 0. Provenance is a debugging/explanation feature.
2. Make node destruction iterative — a ~ASTNode that walks the left spine into a local worklist and releases it flat — so depth cannot overflow the stack regardless of the toggle. This is the standard fix for a shared_ptr list and should land even if (1) is rejected.
3. Make equation()'s traversal iterative for the same reason.
4. Consider a depth cap: past N nodes, collapse to a named leaf holding the value. An explanation 200,000 steps deep is not an explanation.

Part B — normalizeLocaleNumber strips group separators without validation: "1.5" becomes 15

render/locale_format.hpp:71-74 strips every occurrence of groupSeparator unconditionally, before the decimal check and with no check that groups are three digits or that the separator appears before the decimal point.

Reproduced. Revision: master @ 4017228d.

normalize("1.5",     dec=",", grp=".")  = "15"     <-- de-DE form, US-style decimal typed
normalize("1.50",    dec=",", grp=".")  = "150"
normalize("1.2.3.4", dec=",", grp=".")  = "1234"
normalize("1,5",     dec=".", grp=",")  = "15"     <-- en-US form, EU-style decimal typed
normalize("1.5",     dec=".", grp=".")  = "15"     <-- separators equal, no diagnostic
normalize("-5" with U+2212 MINUS)       = NULLOPT

Not verified: whether the QML mirror in src/qt/forms/qml/DynamicForm.qml behaves identically. Check this as part of the fix — if it does, the two edges are consistently wrong rather than inconsistently, which is a smaller problem but still a wrong value, and both need fixing.

Why it is a defect and not a design choice: a German user typing 1.5 into a price field submits 15. This is the single most common real-world locale entry mistake, and the function converts it into a valid-looking value rather than rejecting it — so nothing downstream can catch it. The user is charged ten times with no diagnostic anywhere.

Fix

Validate group placement before stripping: a group separator must be preceded by 1-3 digits, followed by exactly three digits, and must not appear after the decimal separator. "1.5" then correctly returns std::nullopt and the caller can tell the user to fix the entry. Two smaller fixes while there:

• assert(decimalSeparator != groupSeparator) — today the decimal is silently eaten.
• A negativeSign parameter, so a locale using U+2212 MINUS can round-trip.

Note on scope

#537 (INT64_MIN UB in four Rational operations) is the third defect of this family from the same sweep. It is deliberately excluded — PR #561 is already open for it. Do not touch rational.hpp in this PR; rebase on #561 if it lands first.

What would change the verdict

• Close A(1) if provenance-by-default is a deliberate product decision — but A(2) and A(3) should land regardless, since a stack overflow is not an acceptable failure mode for either setting.
• Close B if the function is documented as accepting only already-canonical input — but its whole purpose is to normalise user-typed text at the control edge.

Acceptance — and the test that actually tests

Per AGENTS.md: a check that would still pass with the feature removed is not evidence. Both of these have a specific trap:

• A: build a chain of 100,000 nodes and destroy it at -O0. A test at -O2 passes with or without the fix, because clang tail-call-optimises the chain away. Confirm the test fails on unfixed code before claiming it covers anything.
• B: tests/test_render_locale_format.cpp has 18 cases and none of them is normalize("1.5", ",", "."). Add it, plus the equal-separators case. A single-locale test passes either way.

[Yaraslaut]

Triage verdict: valid

Both parts reproduced from scratch on 8858450d (current master), not carried forward from the 4017228d filing.

Part B — normalizeLocaleNumber

Standalone program against the real header, g++ 16.2.1 -std=c++23 -O2:

normalize(       1.5, dec=  ,, grp=  .) = 15
normalize(      1.50, dec=  ,, grp=  .) = 150
normalize(   1.2.3.4, dec=  ,, grp=  .) = 1234
normalize(       1,5, dec=  ., grp=  ,) = 15
normalize(       1.5, dec=  ., grp=  .) = 15
normalize(  1.234,56, dec=  ,, grp=  .) = 1234.56

Every line matches the issue. A de-DE user typing 1.5 still submits 15, and the equal-separator case still returns a value with no diagnostic. The last line is mine, not the issue's: correctly-grouped input works, which is what makes the failure quiet — the function is right on canonical input and silently wrong on the most common entry mistake.

The mechanism is visible at locale_format.hpp:71-74, unchanged, with its own comment stating the intent: // grouping is display-only; never accepted back on entry. It strips before the decimal check and without validating group placement.

Part A — Quantity provenance

MORPH_QUANTITY_PROVENANCE still defaults to 1 (quantity.hpp:38-40). 200,000-element running total, four builds:

build	result
-O2, PROVENANCE=1 (default)	rc=0, wall 0.037s
-O2, PROVENANCE=0	rc=0, wall 0.003s
-O0, PROVENANCE=1 (default)	SIGNAL 11 — SIGSEGV
-O0, PROVENANCE=0	rc=0, wall 0.020s

The Debug-only crash is real and reproduces today. The stack overflow in the recursive shared_ptr destructor chain kills the process at -O0 and vanishes at -O2 where the chain is tail-call-optimised — exactly the signature the issue calls the worst possible one for finding it. That asymmetry, not the 5.5x, is what makes this a defect rather than a tuning question.

Invariant 3 is the reason both belong in one ticket

The value type reports the fact; the layer decides the policy. Part B is the textbook case: normalizeLocaleNumber fabricates a plausible value where it should report std::nullopt and let the control edge tell the user. Part A is the same shape — the type imposes an unbounded process-wide policy (allocate provenance always) that only the application can reasonably decide. The consolidation is on solid ground.

Note on scope, confirmed

#537 remains out of scope and PR #561 is still open for it. rational.hpp should not be touched by this ticket.

What would change the verdict

Unchanged from the issue, and both acceptance traps are confirmed to be real traps:

• Part A's regression test must run at -O0 — my -O2 runs exit 0 with the defect fully present, so an -O2 test measures nothing.
• Part B's suite has 18 cases and none is the cross-locale one, so a single-locale test passes either way.

Close A(1) if provenance-by-default is a deliberate product decision — but A(2), iterative destruction, should land regardless, because a stack overflow is not an acceptable failure mode under either setting.

(A lane is implementing this now. The verdict is recorded for the next reader, not to gate that work.)