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

Upstream simplified Optimism test #369

Merged
merged 41 commits into from
Mar 17, 2024
Merged

Upstream simplified Optimism test #369

merged 41 commits into from
Mar 17, 2024

Conversation

palinatolmach
Copy link
Collaborator

@palinatolmach palinatolmach commented Feb 14, 2024

Closes #366.

This PR upstreams a simplified Optimism test PortalTest that can be used for performance profiling:

/// @custom:kontrol-array-length-equals _withdrawalProof: 1,
/// @custom:kontrol-bytes-length-equals _withdrawalProof: 32,
function test_withdrawal_paused(
    Types.WithdrawalTransaction memory _tx,
    uint256 _l2OutputIndex,
    Types.OutputRootProof calldata _outputRootProof,
    bytes[] calldata _withdrawalProof
)
    external
{
    vm.expectRevert();
    portalContract.proveWithdrawalTransaction(_tx, _l2OutputIndex, _outputRootProof, _withdrawalProof);
}

It also adds a set of lemmas needed for this test (pausability-lemmas.k), and renames an existing file with lemmas for the sumToN test (sum-to-n-lemmas.k).

The execution of this test on my machine takes:

  • 5m19s using latest (v0.1.204) Kontrol
  • 7m40s using v0.1.178
  • 15m15s using v0.1.176
  • 11m40s using v0.1.176 with booster overwritten to d299753 (with simplifier cache enabled)

@palinatolmach palinatolmach self-assigned this Feb 14, 2024
@palinatolmach
Copy link
Collaborator Author

palinatolmach commented Feb 14, 2024

The CI job with a new test got killed (https://github.com/runtimeverification/kontrol/actions/runs/7898491231/attempts/1?pr=369, and then timed out in 2 hours), and on my machine it also takes > 1 hour — I'll look into how it can be further minimized while still being illustrative of the performance changes.

@palinatolmach palinatolmach changed the title Upstream PortalTest.test_withdrawal_paused [NO-MERGE] Upstream PortalTest.test_withdrawal_paused Feb 16, 2024
@palinatolmach
Copy link
Collaborator Author

palinatolmach commented Feb 16, 2024

The following test (which is too simple to be illustrative of the performance difference between backend versions):

    /// @custom:kontrol-length-equals _withdrawalProof: 3,
    /// @custom:kontrol-length-equals data: 32,
    /// @custom:kontrol-length-equals _withdrawalProof[]: 32,
    function test_withdrawal_paused(
        Types.WithdrawalTransaction calldata _tx,
        uint256 _l2OutputIndex,
        Types.OutputRootProof calldata _outputRootProof,
        bytes[] calldata _withdrawalProof
    )
        external
    {
        portalContract.pause();
        vm.expectRevert();
        portalContract.proveWithdrawalTransaction(_tx, _l2OutputIndex, _outputRootProof, _withdrawalProof);
    }
}

takes ~6 minutes to run locally on my machine and ~8 minutes — on CI when executed by a faster runner. It seems that we're getting very different execution time on booster tests depending on the runner, e.g.,

Making the test more challenging (and lemma-dependent) is needed to ensure that we can observe the difference in performance, however, it seems that it would also cause time outs in many CI runs. With @JuanCoRo, we've also realised that a cleanup of lemmas used to run this test is needed, and it seems that removing some of the lemmas improves the execution time w/o causing any issues — that's what I'm looking into now.

Update: while updating lemmas file and the test, both @JuanCoRo and I are encountering the following issue with Kontrol hanging at the end of the execution while seemingly being unable to stop the server: #375.

@palinatolmach palinatolmach changed the title [NO-MERGE] Upstream PortalTest.test_withdrawal_paused [NO-MERGE] Upstream simplified Optimism test Mar 4, 2024
@palinatolmach palinatolmach marked this pull request as ready for review March 4, 2024 09:28
@palinatolmach
Copy link
Collaborator Author

I've pushed an updated test as well as contracts (OptimismPortal, Types) it's calling. The test demonstrates ~10% performance diffs between Kontrol/backend versions taking

However, the test is the current format uses a different solc version from the rest of the test suite (0.8.15) and can be minimized further. These changes (like any other changes to the Solidity code) require modification of the copy-memory-to-memory lemma, that includes the following steps (instruction courtesy of @PetarMax):

  • when hitting the first branching, get to 200 steps or so backward
  • execute with --break-every-step for ~200 iterations, which should reveal the copying loop
  • update the rule according to where the loop starts, where it ends, and what is the word stack structure at both of those points

@ehildenb
Copy link
Member

@palinatolmach not sure, but can we get this merged? One of the next tasks for Qian is to investigate loop invariant inference using the merge-nodes functionality, so perhaps having this merged will make it easier for her. #448

@palinatolmach
Copy link
Collaborator Author

@ehildenb ok! I'll get it up to date and will merge in the morning. I think I can also remove the lemma that's causing a rewrite in another contract. I agree that it's probably good to merge it now, it would hopefully also help us avoid performance regressions while #442 is being investigated.

@palinatolmach
Copy link
Collaborator Author

palinatolmach commented Mar 15, 2024

Temporarily removed automerge to let #355 go through first.

I couldn't remove the lemmas causing the rewrite in another contract w/o affecting PortalTest performance, but the related investigation is factored out into #442. Otherwise, it's ready to be merged.

@rv-jenkins rv-jenkins merged commit 2fbf98c into master Mar 17, 2024
12 checks passed
@rv-jenkins rv-jenkins deleted the upstream-op-test branch March 17, 2024 10:41
qian-hu pushed a commit that referenced this pull request Mar 18, 2024
* Upstream `PortalTest.test_withdrawal_paused` with lemmas, update expected output

* Set Version: 0.1.160

* Set Version: 0.1.161

* Simplify `test_withdrawal_paused` to speed it up

* Experimental: temp increase CI timeout

* Set Version: 0.1.165

* Set Version: 0.1.172

* Set Version: 0.1.177

* Set Version: 0.1.179

* Optimism lemmas cleanup

* Update simplified OptimismTest corresponding to the copying loop lemma

* Set Version: 0.1.180

* Set Version: 0.1.185

* Upstreamed simplified `PortalTest`

* Cleanup updated `contracts.k` and `foundry.k`

* Update output for `testFail_assume_true` w/simplifications, cosmetic changes in `foundry.k`

* Set Version: 0.1.188

* Update output for `test_assume_false`

* Update revert message in `Portal`

Co-authored-by: Andrei Văcaru <[email protected]>

* Set Version: 0.1.189

* Set Version: 0.1.190

* Set Version: 0.1.192

* Remove reflexivity lemma

* Update bytecode for `Portal`

* Set Version: 0.1.205

* Revert `lemmas.k` renaming

* Formatting in tests

* Set Version: 0.1.210

---------

Co-authored-by: devops <[email protected]>
Co-authored-by: Andrei Văcaru <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add simplified OP test to Kontrol test suite
4 participants