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

Compatibility enforcements between Rust and Solidity #337

Open
storojs72 opened this issue Feb 20, 2024 · 3 comments
Open

Compatibility enforcements between Rust and Solidity #337

storojs72 opened this issue Feb 20, 2024 · 3 comments
Assignees
Labels
A-2 Ethereum verification CI Continuous Integration related issues

Comments

@storojs72
Copy link
Member

storojs72 commented Feb 20, 2024

Nova verifier implemented in Arecibo consists from a bunch of cryptographic blocks (Poseidon, KeccakTranscript, IPA, etc.) each having its Solidity counterpart in solidity-verifier project. Historically, Solidity codebase growing started from lurk-lab/Nova. Currently we have a situation when Solidity building blocks guarantee compatibility only with some specific (various) versions of their Rust references, for example:

In order to prevent our Solidity codebase obsoleting, it is necessary to enforce Rust <-> Solidity compatibility per building blocks. Apart from e2e integration testing that we are performing in feature branches of solidity-verifier (where we have "terminal" code in a form of contracts) that guarantees complex compatibility of the whole verification flow (which is now obsolete and hence unfortunately is not able to consider frequent low-level changes happening in Arecibo) we can have in Arecibo bunch of unit-tests (one per each building block that we have in Solidity) - each of them will produce correspondent solidity unit-test with input/output data dynamically generated using particular Arecibo commit. This can be achieved with a help of Solidity templating in Rust (initial experiments for IPA are here). E.g. we can establish branch-protection rule at Arecibo repository that prevents merging PR if failure occurs on CI job with following set of steps (for single IPA case, which can be extended for every building block):

  • Run compatibility unit-test for IPA and generate solidity counterpart unit-test;
  • Fetch main branch of solidity-verifier;
  • Execute unit tests (which should include generated compatibility IPA unit-test) and fail if solidity-verifier' tests are not succeeded;

As Arecibo is reference - it should dominate in making decision whether accept or not the particular PR that breaks compatibility. But with these new settings - such breaking changes should be thoroughly justified.

I would start from enforcing single block - IPA compatibility and see how it will work.

@samuelburnham, happy to communicate on overall idea or its lower details more. Your additional input is highly appreciated!

@storojs72 storojs72 added A-2 Ethereum verification CI Continuous Integration related issues labels Feb 20, 2024
@storojs72 storojs72 self-assigned this Feb 20, 2024
@samuelburnham
Copy link
Member

This is great, automated compatibility between the two repos would be amazing. I think we can hash out the CI details after the unit tests are set up, but my first thought is to have CI open a noisy issue in Solidity-verifier and/or Arecibo when breaking changes are merged rather than having to bypass required status checks.

@storojs72
Copy link
Member Author

Ok, PR for IPA is up, once we merge it, we are ready to making appropriate CI changes.

As it is mentioned in PR description, once IPA compatibility test is generated:

cargo test test_solidity_compatibility_ipa --release -- --ignored --nocapture > ipa-compatibility.t.sol
catipa-compatibility.t.sol | sed '1,3d' | sed -n -e :a -e '1,4!{P;N;D;};N;ba' > tmp.file && mv tmp.file ipa-compatibility.t.sol

next, ipa-compatibility.t.sol can be added to the solidity-verifier, and executed via forge

mv ipa-compatibility.t.sol solidity-verifier/test
forge test --match-path test/ipa-compatibility.t.sol -vv

the result of execution - failed/successful - will indicate whether IPA in Rust is incompatible/compatible with IPA in Solidity.

@storojs72
Copy link
Member Author

my first thought is to have CI open a noisy issue in Solidity-verifier and/or Arecibo when breaking changes are merged rather than having to bypass required status checks

@samuelburnham, I think noisy issue should appear in Solidity-verifier. At the same time, I would say we need some explicit blocker or warning at Arecibo's PRs that break compatibility in order to force PR' author to think twice if the task can be solved without breaking changes. If the answer is NO and potential PR's benefit exceeds necessity of extra-work at Solidity side of things - then this extra-work should happen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-2 Ethereum verification CI Continuous Integration related issues
Projects
None yet
Development

No branches or pull requests

2 participants