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

do not observe machines that were removed at runtime #2127

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Conversation

leonardoalt
Copy link
Member

No description provided.

@@ -3,14 +3,14 @@ use powdr::Session;
fn main() {
env_logger::init();

let n = 21;
let n = 22;
Copy link
Member Author

Choose a reason for hiding this comment

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

cause one more chunk

multi_table
.tables
.get(name)
.and_then(|table| map.get(&table.degree).map(|entry| &entry.commitment))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.and_then(|table| map.get(&table.degree).map(|entry| &entry.commitment))
.and_then(|table| map[&table.degree].commitment)

if the machine is there, this would skip the commitment if the size is not available. Instead, if the table is there, it's a hard error not to have a commitment for its size.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or a clean error that the machine cannot be proven because the setup doesn't cover it's length

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Schaeff
Copy link
Collaborator

Schaeff commented Nov 21, 2024

I wonder if a similar change should happen in the verifier?

@leonardoalt
Copy link
Member Author

I wonder if a similar change should happen in the verifier?

If I understand it correctly that's already done!

https://github.com/powdr-labs/powdr/blob/main/plonky3/src/verifier.rs#L165

In here, a None get(name) propagates to failing the if Some, afaiu.

Copy link
Collaborator

@georgwiese georgwiese left a comment

Choose a reason for hiding this comment

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

Opps 😬

@georgwiese
Copy link
Collaborator

georgwiese commented Nov 22, 2024

Just checked why this didn't happen in the test I added in #2078 (test_data/asm/block_to_block_empty_submachine.asm). It's because the removed machine doesn't have any fixed columns (after the optimizer run). In Arith, you could change the latch to col fixed latch = [0, 1]*; to provoke the bug. Maybe with a comment :) Could be better than increasing the chunk count of Fibonacci.

@leonardoalt
Copy link
Member Author

Where in Arith? Not sure we should change std machines just to test this.

@leonardoalt leonardoalt added this pull request to the merge queue Nov 22, 2024
Merged via the queue into main with commit 8b6ada6 Nov 22, 2024
14 checks passed
@leonardoalt leonardoalt deleted the p3-bug branch November 22, 2024 10:06
@georgwiese
Copy link
Collaborator

I mean the machine called Arith in test_data/asm/block_to_block_empty_submachine.asm, not the std one. Just opened #2133.

github-merge-queue bot pushed a commit that referenced this pull request Nov 22, 2024
See [this
comment](#2127 (comment)).
This updated test would have caught a bug fixed in #2127.
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.

3 participants