Feat/modular inverse

[veigajoao]

PR checklist:

• Test coverage for the proposed changes
• PR description contains example output from repl interaction or a snippet from unit test output
• Documentation has been updated if new natives or FV properties have been added. To generate new documentation, issue cabal run tests. If they pass locally, docs are generated.
• Any changes that could be relevant to users have been recorded in the changelog
• In case of changes to the Pact trace output (pact -t), make sure pact-lsp is in sync.

Additionally, please justify why you should or should not do the following:

• Confirm replay/back compat
  No need, no functionalities were changed, just added and entire previous test suite runs smoothly
• Benchmark regressions
• (For Kadena engineers) Run integration-tests against a Chainweb built with this version of Pact

[jmcardon]

Hi!

We appreciate the PR and the new contribution to Pact.

That said, we have a process for feature requests like this. Please open a KIP first at https://github.com/kadena-io/kips for us to proceed with this PR.

Logistics aside, this PR also needs a lot more gas analysis than simply charging 13 gas for the native and calling it a day, particularly because the input modulus and the input integer are arbitrarily large.

For now, I think it would be fine to leave it open (Though preferably, draft it), but please proceed with a KIP first.

[veigajoao]

Hi @jmcardon thanks for the comments. I submitted the PR here after talking to the grant's team about this milestone, wasn't aware of the KIP procedure, sorry for that. I am going to draft a document and submit it to the KIP repo.

Before I send this to draft, can I just get some input on the gas testing best practices /requirements? I couldn't find any contribution guides for the repo so we just replicated what was found in some previously approved PRs that added Pact builtins (more specifically this).

Here is the procedure we followed to determine the gas quantity and test it:

1. We added the tests here:
   

2. We ran the tests:

pact/executables$ cabal run gasmodel -- -f keccak256-bs

3. It generated a CSV with the following result:
   

4. We added it to the gas table:
   

Are there any extra steps that we missed in the gas testing procedure? Or should we just try to simulate larger numbers? Are there any exemples of proper tests that you can point me to?

[emilypi]

I submitted the PR here after talking to the grant's team about this milestone, wasn't aware of the KIP procedure, sorry for that. I am going to draft a document and submit it to the KIP repo.

Absolutely no worries here. We're internally migrating to a more community-oriented approach, so this fact isn't written down anyway, but we are steering any additional natives, including our own additions, to be proposed through the KIP process.

As far as Gas testing goes, I defer to Jose 😄

[veigajoao]

@jmcardon just openned the PR on the KIP repo here.

Regarding the gas tests, could you clarify the necessary tests that I asked about above?

[jwiegley]

@veigajoao Your gas testing looks correct to me. The next step would be to generate many valid inputs to your keccak256-bs function and ensure that the gas price remains valid across all samples (where valid means, "it takes 13 * 2.5µs for the calculation to run"). If there are no outliers, then you have done the correct work.