Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add expModInteger to specification #6512

Merged
merged 8 commits into from
Oct 30, 2024
Merged

Add expModInteger to specification #6512

merged 8 commits into from
Oct 30, 2024

Conversation

bezirg
Copy link
Contributor

@bezirg bezirg commented Sep 24, 2024

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@bezirg bezirg requested a review from kwxm September 24, 2024 11:39
@bezirg bezirg added the No Changelog Required Add this to skip the Changelog Check label Sep 24, 2024
@bezirg bezirg self-assigned this Sep 24, 2024
@bezirg bezirg requested review from a team and removed request for kwxm September 24, 2024 16:17
@zliu41
Copy link
Member

zliu41 commented Sep 24, 2024

Can you remind me how I can build this locally?

@bezirg
Copy link
Contributor Author

bezirg commented Sep 25, 2024

plutus-core-specification.pdf

Can you remind me how I can build this locally?

make under doc/plutus-core-spec

@zliu41
Copy link
Member

zliu41 commented Sep 25, 2024

make under doc/plutus-core-spec

Ok, this seems to require a separate installation of pdflatex, as it's not provided by nix develop.

@@ -106,6 +106,7 @@ \section{Ledger languages}
\LL{PlutusV2} & 7.0 & Batch 2 \\
\LL{PlutusV2} & 8.0 & Batch 3 \\
\LL{PlutusV3} & 9.0 & Batch 4 \\
\LL{??} & ?? & Batch X \\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table will need to be reworked or discarded. Since we can now add (new or existing) builtins to existing Plutus versions, the table may no longer make sense. This particular builtin will likely be introduced into V1, V2 and V3 at the same time, at Protocol version 11.0.

Copy link
Contributor

@kwxm kwxm Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table will need to be reworked or discarded. Since we can now add (new or existing) builtins to existing Plutus versions, the table may no longer make sense. This particular builtin will likely be introduced into V1, V2 and V3 at the same time, at Protocol version 11.0.

@ZLiu21 There's an issue to fix the explanation of versions here. We should probably do what you mention above as part of that as well.

@kwxm
Copy link
Contributor

kwxm commented Oct 1, 2024

@bezirg I'll look at this more closely next week, but note that the PR to add the bitwise operations to the specification is still unmerged and this will probably have to be rebased on it.

@kwxm kwxm mentioned this pull request Oct 8, 2024
Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, although it could be a bit more precise about what the denotation of the builtin actually is. You should wait for #6426 to be merged first (which I hope will happen soon) since that contains a lot of stuff that goes just before this.

\caption[]{Built-in Functions}
\label{table:built-in-functions-X}
\endlastfoot
\TT{expModInteger} & $[\ty{integer}, \ty{integer}, $ \text{$\;\; \ty{integer}] \to \ty{integer}$}
Copy link
Contributor

@kwxm kwxm Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest making this a bit more precise by making the denotation something like

$ (a,e,m) \mapsto 
  \begin{cases}
     \modfn(a^e,m) & \text{if $m > 0$ and $e \geq 0$}\\
     \min\{r \in \N: ra^{-e} \equiv 1 \pmod{m}\}\  \text{if $m > 0, e < 0$ and $\mathrm{gcd}(a,m) = 1$ }\\
     \errorX & \text{otherwise}
  \end{cases})$ 

but I don't know if that'll fit into the box (and it's pretty much what you say later on). Also, what I wrote has two different things denoted by mod, which might be confusing: the first one's the function defined on pages 32 and 33 and the second one's the usual thing in the congruence relation . Maybe you could add a function like itobs in pages 36-37. The point is that we want to precisely define the denotation as a mathematical function and it should say explicitly what the conditions are that lead to the big X error value, and that's maybe not obvious here unless you already know what's going on.

The operation fails with \texttt{error} in any of the following cases:
\begin{enumerate}
\item $m \le 0$
\item $e < 0$, and integer $b$ does not have a multiplicative inverse w.r.t. $(\modfn m)$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to have a reference to the fact that if e<0 then a^e only exists mod m if gcd(a,m)=1 (and include that condition in the definition). Thats' a standard number-theoretic fact which can be found in eg Proposition I.3.1 of

@book{Koblitz,
      author = "Koblitz, Neal"
      title = "{A Course in Number Theory and Cryptography}",
      publisher = "Springer-Verlag",
      series = "Graduate Texts in Mathematics",
      volume = 114,
      edition = 2,
      year = "1994"
}

@kwxm
Copy link
Contributor

kwxm commented Oct 30, 2024

I've tidied up a couple of things and merged the updates for the bitwise builtins into this branch. I think we can merge this now.

@kwxm kwxm merged commit 983eba5 into master Oct 30, 2024
8 checks passed
@kwxm kwxm deleted the bezirg/expmod-spec branch October 30, 2024 04:43
zeme-wana pushed a commit that referenced this pull request Oct 30, 2024
* Add expModInteger to specification. Closes #6339

* Reconcile with master

* Update reference to last table of builtin tags

* Remove trailing spaces

* batch -> Batch

* Be a bit more precise about the definition of expMod

* Tidying up

---------

Co-authored-by: Nikolaos Bezirgiannis <[email protected]>
Co-authored-by: kwxm <[email protected]>
@kwxm kwxm mentioned this pull request Oct 30, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants