spec: ECSM#655
Conversation
Codex Code ReviewFindings
No other concrete PR-diff issues found in this pass. |
Code Review — ECSM Accelerator SpecGood overall structure. The three-chip decomposition (ECSM → ECDAS → EC_SCALAR) is clean, and the soundness argument for why the point-at-infinity can never appear during double-and-add is well-reasoned. Issues foundMathematical error (spec/ecsm.typ:23) Typo (spec/ecsm.typ:158) Ref typo (spec/src/ecdas.toml:316) Assumption inconsistency (spec/src/ecsm.toml:199) Minor observations
|
Indeed. Fixed it.
Fixed it |
Technically, this is already addressed: the introduction mentions that it accelerates points in
Fixed
It is in the same document, but a section earlier. Disregarding the comment. |
RobinJadoul
left a comment
There was a problem hiding this comment.
- I do wonder how much potential optimization we're leaving on the table by treating
pas a generic prime, rather than using the structure it has - A more descriptive name for the
BITandSERVE_Kinteractions may be in order - The complex arith constraints for equalities mod p tend to have a different order between their "constraint" and "poly", which makes checking correspondence a bit tedious and error-prone
- I have some thoughts about the presentation of the "theory"/background, but trying to focus on the more technical things first
| desc = "$(#`addr_xG` mod 2^32) + 24 < 2^32$" | ||
| ref = "ec:a:addr_xG_alignment" | ||
|
|
||
| [[assumptions]] | ||
| desc = "$(#`addr_k` mod 2^32) + 31 < 2^32$" | ||
| ref = "ec:a:addr_k_alignment" | ||
|
|
||
| [[assumptions]] | ||
| desc = "$(#`addr_xR` mod 2^32) + 24 < 2^32$" | ||
| ref = "ec:a:addr_xR_alignment" |
There was a problem hiding this comment.
How safe is this? The fact that this comes from an ECALL means that it is not an assumption we can ensure at the spec level, and rather something that is pushed down into the guest program.
There was a problem hiding this comment.
How safe is this?
I seem to recall that one of our strategies is to range check (address, value, timestamp) for all writes. As a result, all reads can now be assumed safe.
When we adhere to this rule, I believe the tagged assumption to be safe.
Reasoning:
Suppose addr_xG (or addr_k or addr_R for that matter) violates the tagged assumption.
That would mean that at least one of the interactions in ec:c:read_xG would read a value from an invalid address.
To balance the LogUp, one would have had to (previously) perform a write-only interaction for this same, illegal address.
Under the assumption that all writes range check their address, this should be impossible.
Note: performing a read&write-interaction would not cut it, as this would just move the problem.
that is pushed down into the guest program
You're right that this is indeed pushed down to the guest program.
Note that this is not the first time this is done; the SHA256-C3 and SHA256-C7 essentially leverage the same trick: if the addresses for h_addr and m_addr don't align, C2 and C6 will fail.
There was a problem hiding this comment.
I think the SHA256 version actually works regardless of the alignment?
SHA256-C3 and SHA256-C7 both use a full ADD so there's no requirement on being small enough, since ADD can deal with overlow correctly. And if it's not dword-aligned, then you get the fallback to slow version of MEMW, but besides performance, there's no safety or correctness assumption on the guest that I see there.
Please do correct me if I'm reading that wrong.
I think it is just a completeness thing and indeed not soundness as you argue, as long as we keep the initialization of addresses correctly range-checked, which is already a hard requirement due to MEMW.
We can make this assumption, but then I would put it more visibly, as this becomes critical knowledge for consumers, and not just for implementing the spec.
There was a problem hiding this comment.
- You're right: SHA is alignment-agnostic
- As discussed, we're sticking with having no soundness gaps for now. I updated the spec accordingly.
| [[constraints.yR]] | ||
| kind = "interaction" | ||
| tag = "IS_HALF" | ||
| input = [["+", ["idx", "c2", "i"], 16320]] |
There was a problem hiding this comment.
Can we maybe move these magic constant columns into a constant column of the chip? It's nicer to have them all in the same place to check correspondence with the analysis and make sure everything is up-to-date if we do change the analysis at some point (e.g. because we get Half limbs)
| kind = "interaction" | ||
| tag = "BIT" | ||
| input = ["timestamp", "round"] | ||
| multiplicity = ["-", "next_op"] |
There was a problem hiding this comment.
I'd keep the positive here and the negative in EC_SCALAR, to keep with the usual interpretation of μ = -1 meaning that you've "shown" the relation to hold
| [[constraints.send]] | ||
| kind = "template" | ||
| tag = "IS_BIT" | ||
| input = ["next_op"] | ||
| ref = "ecdas:c:range_next_op" |
There was a problem hiding this comment.
The LogUp should take care of it, but probably good to still bit-check op itself too
edffcf6 to
32117c5
Compare
RobinJadoul
left a comment
There was a problem hiding this comment.
Mostly concerned about the idx_k thing, the rest is either a quick edit or double checking calculations
| - *`ECSM` (Elliptic Curve Scalar Multiply)*. | ||
| This chip is responsible for | ||
| - loading $k$ from memory and verifying that it is contained in $[1, N)$, | ||
| - loading inputs $x_G$ and reconstructing $y_G$, |
There was a problem hiding this comment.
and verifying x_G < p
| - `addr_xG[0]`, `addr_k[0]` and `addr_xR[0]` could be `DWordWL`s rather than `HL`s. | ||
| We use `HL`s as conventient notation. | ||
| This modification saves 6 columns. | ||
| - the design of these chip is generic, and makes no assumptions on the parameters $b$, $p$ and $N$. |
There was a problem hiding this comment.
| - the design of these chip is generic, and makes no assumptions on the parameters $b$, $p$ and $N$. | |
| - the design of these chip is generic, and makes no assumptions on the parameters $a$, $b$, $p$ and $N$. |
|
|
||
| [[variables.constant]] | ||
| name = "id" | ||
| type = "Bit" |
There was a problem hiding this comment.
Technically not a bit if we want it to be extensible
| #render_constraint_table(ecsm_chip, config, groups: "xG2") | ||
|
|
||
| Next, we restrict the witness pair $(y_G, #`q1`)$. | ||
| Note there that @ec:c:c1_0 and @ec:c:c1_i multiply `B` and `P` by `μ` to simplify the padding; there are no other side-effects to this since $#`μ` = 1$ on non-padding rows (@ec:c:mu_isbit). |
There was a problem hiding this comment.
I think we're trading off padding simplicity for degree 3 constraints where otherwise we only need degree 2. Minor, but potentially worth a mention in the notes
| === Subroutine | ||
| With point $G$ and scalar $k$ fully constructed, we serve scalar `k` bit-by-bit to the `ECDAS` chip. | ||
| On this chip, we do capture the index of the most significant 1-bit of `k` in `idx_k`, to instruct the `ECDAS` chip where to start. | ||
| Note: if the prover decides to capture a lesser significant bit here, the LogUp will not balance, as the skipped bits will never taken off the bus. |
There was a problem hiding this comment.
But what about a greater idx_k? We'd probably need to check that k[idx_k] = 1, which seems annoying.
| Applying @limbs:cor:carry-upper-bound with $(L, n) = (2^8, 32)$, we find that | ||
| $ | ||
| #`c0`_i &in [-8160, 8159],\ | ||
| #`c1`_i &in [-24477, 24478].\ |
There was a problem hiding this comment.
I think you're off by a few from what I tried to compute quickly, but it's wider than what I got, so should be safe anyway
| Leveraging @limbs:cor:carry-upper-bound with $(L, n) = (2^8, 33)$ and maximizing for the value of $#`op` in {0, 1}$, we gather | ||
| $ | ||
| #`c0`_i &in [-33657, 25242],\ | ||
| #`c1`_i &in [-8416, 16828], text("and")\ |
There was a problem hiding this comment.
I got -8418 (μ=1, α = 5, δ=1)
| $ | ||
| #`c0`_i + #`carry_offsets[0]` &in [0, 58899] subset.eq [2^16],\ | ||
| #`c1`_i + #`carry_offsets[1]` &in [0, 25244] subset.eq [2^16],text("and")\ | ||
| #`c2`_i + #`carry_offsets[2]` &in [0, 33688] subset.eq [2^16]. |
This PR introduces the
ECSM(Elliptic Curve Scalar Multiplication) accelerator.