fix: maintain ordering of return value witnesses when constructing ABI

[TomAFrench]

Related issue(s)

Resolves #1174

Description

Summary of changes

This PR switches the evaluator to track the witnesses for the return value in a Vec<Witness> rather than a BTreeSet<Witness>. While it's correct to store them as a set inside the Circuit, we rely on the ordering of these witnesses for proper ABI decoding and so we need to maintain this when we're constructing the ABI.

See #1174 for an example of this causing improper ABI decoding.

Dependency additions / changes

Test additions / changes

Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt with default settings.
• I have linked this PR to the issue(s) that it resolves.
• I have reviewed the changes on GitHub, line by line.
• I have ensured all changes are covered in the description.

Documentation needs

• This PR requires documentation updates when merged.

Additional context

BEGIN_COMMIT_OVERRIDE
fix: maintain ordering of return value witnesses when constructing ABI (#1177)
END_COMMIT_OVERRIDE

[rahul-kothari]

can you add some tests to ensure future versions don't run into this?

[kevaundray]

Can you also add an example to ensure we don't regress?

[rahul-kothari]

how did we regress in the first place? I think this is new to v0.4 - curious how it happened.

[kevaundray]

how did we regress in the first place? I think this is new to v0.4 - curious how it happened.

I was saying that we should add an example to ensure we don't regress in the future

[TomAFrench]

The issue is the integration tests make use of the "prove then immediately verify" feature so we're not loading the public inputs from Verifier.toml. We switched to this to cut down CI times but I can revert that.

[kevaundray]

The issue is the integration tests make use of the "prove then immediately verify" feature so we're not loading the public inputs from Verifier.toml. We switched to this to cut down CI times but I can revert that.

Was it a large effect on CI times?

[TomAFrench]

It was a pretty sizable percentage afaik, i.e. double digits.

[TomAFrench]

I've made the changes to integration tests in #1179 as that dwarfs the changes in this PR.

[kevaundray]

It was a pretty sizable percentage afaik, i.e. double digits.

oof RIP productivity, lets try to get a larger github runner to counteract this

[kevaundray]

LGTM - I think if separating prove and verify add 10 minutes, then we should have the larger runners in place, before merging the other PR