Skip to content

Adaptations for the upcoming change to the new iterator interface.#48

Merged
jaylorch merged 5 commits into
microsoft:mainfrom
parno:main
May 12, 2026
Merged

Adaptations for the upcoming change to the new iterator interface.#48
jaylorch merged 5 commits into
microsoft:mainfrom
parno:main

Conversation

@parno
Copy link
Copy Markdown
Contributor

@parno parno commented Apr 28, 2026

When we release the new approach to specifying iterators, this PR should get the repo back into a verifying state.

@parno
Copy link
Copy Markdown
Contributor Author

parno commented Apr 28, 2026

@microsoft-github-policy-service agree company="CMU"

@jaylorch
Copy link
Copy Markdown
Member

jaylorch commented May 8, 2026

Thanks! When a vstd crate with this feature is available on crates.io, could you please update this PR to include the requisite updates to the various Cargo.toml files, so they point to that version?

@parno
Copy link
Copy Markdown
Contributor Author

parno commented May 10, 2026

I updated the Cargo.toml files. At least on my local version two of the three projects now verify. However, with CapybaraKV, I'm seeing:

capybaraKV/capybarakv $ cargo verus focus --no-default-features

thread '<unnamed>' (12115053) panicked at vir/src/sst_to_air.rs:172:25:
abstract datatype should be boxed Datatype(Path(Path(vstd, ["resource" :: "ghost_var" :: "GhostVar"])), [Datatype(Path(Path(capybarakv, ["kv2" :: "concurrentspec_t" :: "ConcurrentKvStoreView"])), [TypParam("K"), TypParam("I"), TypParam("L")], [])], [])
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread '<unnamed>' (12115053) panicked at rust_verify/src/verifier.rs:424:17:
dropped, expected call to `into_inner` instead

Does that seem at all familiar?

@jaylorch
Copy link
Copy Markdown
Member

It doesn't look familiar to me. Maybe I can recreate it. Are you using Linux or MacOS to get this error?

@jaylorch
Copy link
Copy Markdown
Member

Oh, I'm able to recreate it on Windows.

@jaylorch
Copy link
Copy Markdown
Member

To answer your question, no, it doesn't look familiar to me.

@parno
Copy link
Copy Markdown
Contributor Author

parno commented May 11, 2026

I isolated the issue leading to the panic, and pushed a potential fix in this PR. It was a problem with how recommends are handled when a proof fails. With that out of the way, I could see the actual failure, which was a failed loop invariant inside of setup in shardkv_v.rs. In 9cf8ada, I added a spinoff prover directive that seems to nudge things back into a verifying state.

@parno parno marked this pull request as ready for review May 11, 2026 20:26
@jaylorch
Copy link
Copy Markdown
Member

jaylorch commented May 11, 2026

Thanks for figuring it out! Now I'm trying to figure out how to fix the error (without spinning off the prover, which masks it). I'm finding that even if I assert or assume the loop condition at the end of the loop (after taking into account the change to idx that's about to occur), I still see the error. And even if I rewrite the loop invariant to avoid using the loop variable idx, I still find I can assume it at the end of the loop and Verus can't verify that the condition is preserved by the loop body. Is there some way I can see the "desugared" form of the new iterator loop so I can see what's happening between the end of the loop body and the end of the desugared loop body?

@jaylorch
Copy link
Copy Markdown
Member

Never mind. It seems to have nothing to do with the loop. I'm investigating the triggering issue.

@jaylorch
Copy link
Copy Markdown
Member

jaylorch commented May 11, 2026

I found the problem. There was a missing invariant in the loop. I'll push my fix to this PR, if it lets me.

@jaylorch
Copy link
Copy Markdown
Member

jaylorch commented May 11, 2026

Hmm, it's not letting me. Here's the diff you need:

diff --git a/capybaraKV/capybarakv/src/kv2/shardkv_v.rs b/capybaraKV/capybarakv/src/kv2/shardkv_v.rs
index 45db90ff..14aaa91e 100644
--- a/capybaraKV/capybarakv/src/kv2/shardkv_v.rs
+++ b/capybaraKV/capybarakv/src/kv2/shardkv_v.rs
@@ -238,7 +238,6 @@ where
         self.inv@.constant().combined_id
     }

-    #[verifier::spinoff_prover]
     exec fn setup(
         nshards: usize,
         Tracked(shard_res): Tracked<Map<int, GhostVar<ConcurrentKvStoreView::<K, I, L>>>>,
@@ -271,6 +270,7 @@ where

         for idx in 0..nshards
             invariant
+                nshards >= 1,
                 pred.shard_ids.len() == idx,
                 pred.combined_id == shardstates.combined.id(),
                 pred.combined_id == combined_res.id(),

@jaylorch
Copy link
Copy Markdown
Member

jaylorch commented May 11, 2026

I wonder why the new iterator system requires this invariant nshards >= 1. After all, nshards doesn't change in the body of the loop.

@jaylorch
Copy link
Copy Markdown
Member

jaylorch commented May 11, 2026

It doesn't seem to need invariants about shard_res_old, such as these that seem useful:

forall |shard| 0 <= shard < nshards ==> #[trigger] shard_res_old.contains_key(shard),
forall |shard| #[trigger] shard_res_old.contains_key(shard) ==> {
    &&& shard_res_old[shard]@.ps == ps
    &&& shard_res_old[shard]@.pm_constants == pm_constants
    &&& shard_res_old[shard]@.kv == RecoveredKvStore::<K, I, L>::init(ps).kv
},

It seems to be able to infer these, as expected, because shard_res_old doesn't change in the body of the loop.

@jaylorch
Copy link
Copy Markdown
Member

The safest, most stable thing to do is to add those invariants about shard_res_old as well, and then we're not dependent on the vagaries of how Verus chooses to treat variables unmodified in the loop body.

@jaylorch jaylorch self-requested a review May 12, 2026 00:30
Copy link
Copy Markdown
Member

@jaylorch jaylorch left a comment

Choose a reason for hiding this comment

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

Thanks!

@jaylorch jaylorch merged commit 5e4d4ce into microsoft:main May 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants