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

fix(castf): handle range check correctly for padding rows #1206

Merged
merged 2 commits into from
Jan 11, 2025

Conversation

lispc
Copy link
Contributor

@lispc lispc commented Jan 11, 2025

Fixes the bug below. The root cause was that the range check interaction in CASTF did not condition the send on row validity.

 $ cargo test --package openvm-native-circuit --lib -- castf::tests::castf_rand_test --exact --show-output

running 1 test
test castf::tests::castf_rand_test ... FAILED

successes:

successes:

failures:

---- castf::tests::castf_rand_test stdout ----
Bus 4 failed to balance the multiplicities for fields=[0, 8]. The bus connections for this were:
   Air idx: 2, Air name: VmAirWrapper<ConvertAdapterAir<1, 4>, CastFCoreAir>, interaction type: Send, count: 1
   Air idx: 2, Air name: VmAirWrapper<ConvertAdapterAir<1, 4>, CastFCoreAir>, interaction type: Send, count: 1
   Air idx: 2, Air name: VmAirWrapper<ConvertAdapterAir<1, 4>, CastFCoreAir>, interaction type: Send, count: 1
Bus 4 failed to balance the multiplicities for fields=[0, 6]. The bus connections for this were:
   Air idx: 2, Air name: VmAirWrapper<ConvertAdapterAir<1, 4>, CastFCoreAir>, interaction type: Send, count: 1
thread 'castf::tests::castf_rand_test' panicked at /Users/zhangzhuo/.cargo/git/checkouts/stark-backend-bcfa8b09bc28ca79/8c9f94b/crates/stark-backend/src/air_builders/debug/check_constraints.rs:164:9:
LogUp multiset equality check failed.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    castf::tests::castf_rand_test

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 15 filtered out; finished in 0.13s

error: test failed, to rerun pass `-p openvm-native-circuit --lib`

I found this bug during debuging kernel codes, some simple codes like

VmOpcode(293) 44 16775749 0 1 5 0 0 // castf native_as[16775749] to x11 register
VmOpcode(293) 44 16775748 0 1 5 0 0 // repeat 
VmOpcode(293) 44 16775741 0 1 5 0 0 // repeat 

will fail. So i managed to reproduce it using provided unittest.

(repeating castf twice will be ok, 3 times will trigger logup err)

This comment has been minimized.

@yi-sun
Copy link
Collaborator

yi-sun commented Jan 11, 2025

@lispc: Fixed in 1ec509d -- the issue was that range check multiplicities for padding rows were not handled correctly in CASTF.

Can you check to see if this resolves your kernel code issues?

@yi-sun yi-sun changed the title bug(castf): LogUp multiset equality check failed fix(castf): handle range check correctly for padding rows Jan 11, 2025
Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair 3,812 746,333 30,013,314 - - -
fibonacci_program (+62 [+1.0%]) 6,121 1,500,137 51,505,102 - - -
regex_program (+538 [+2.9%]) 19,060 4,190,904 165,028,173 - - -
ecrecover_program 2,607 285,401 15,092,297 - - -

Commit: 1ec509d

Benchmark Workflow

@yi-sun yi-sun marked this pull request as ready for review January 11, 2025 19:05
@yi-sun yi-sun requested a review from jonathanpwang January 11, 2025 19:06
@jonathanpwang jonathanpwang merged commit e1c9244 into main Jan 11, 2025
17 checks passed
@jonathanpwang jonathanpwang deleted the bug/zzhang/castf branch January 11, 2025 19:11
@lispc
Copy link
Contributor Author

lispc commented Jan 12, 2025

fixed!

@lispc
Copy link
Contributor Author

lispc commented Jan 12, 2025

is this correct?

VmOpcode::from_usize(CastfOpcode::CASTF as usize),

should it be VmOpcode::with_default_offset(CastfOpcode::CASTF)?

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