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

Connecting test harness #1

Closed
hoytech opened this issue Sep 12, 2023 · 5 comments
Closed

Connecting test harness #1

hoytech opened this issue Sep 12, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@hoytech
Copy link
Contributor

hoytech commented Sep 12, 2023

Hi! I got the harness working. Unfortunately the test suite isn't passing yet. I'm trying to track it down. Here's my work so far: master...hoytech:rust-negentropy:harness

I added the harness and started comparing the behaviour of the C++ versus the Rust impl. The first thing that was causing me problems was that you used a vector for pending_output, but I used a deque. I just made the Rust one use a deque instead, so I can compare more directly for now. We can roll that back later if not necessary.

The first difference I noticed was in the splitRange function. The C++ function has an iterator curr whereas the rust one doesn't. I added some tracing lines (visible in the above commit) and this is the output I get from rust:

TRACE A 0
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE A 1677970534
TRACE B 18446744073709551615

But this is what I got from the C++:

TRACE A 1677970550
TRACE A 1677970563
TRACE A 1677970577
TRACE A 1677970595
TRACE A 1677970609
TRACE A 1677970622
TRACE A 1677970634
TRACE A 1677970648
TRACE A 1677970657
TRACE A 1677970668
TRACE A 1677970677
TRACE A 1677970687
TRACE A 1677970691
TRACE A 1677970713
TRACE A 1677970725
TRACE A 18446744073709551615
TRACE B 18446744073709551615

So something isn't advancing through the ranges properly in the rust version. You can reproduce this by checking out the two repos side-by-side (make sure to pull latest master in negentropy repo) and running the following command inside the negentropy/test/ directory:

DEBUG=1 RUST_BACKTRACE=1 RECS=200 P1=1 P2=0 P3=0 perl fuzz.pl rust rust
@hoytech hoytech added the bug Something isn't working label Sep 12, 2023
@yukibtc
Copy link
Member

yukibtc commented Sep 12, 2023

Thanks! I replaced the VecDeque with a classical Vec because it's faster. I adapted the rest of the code to use the it instead of the VecDeque but probably something goes wrong. I'll try to revert it and search any other issues.

@hoytech
Copy link
Contributor Author

hoytech commented Sep 13, 2023

Got it working! See my PR here: #2

I'm sure some of the code is not idiomatic Rust -- sorry about that. But the test suite passes using all language combinations: Rust/C++, C++/Rust, Rust/JS, etc. I confirmed the wire output of the Rust version is identical to the C++.

You might be able to convert pending_outputs back to a Vec. If you try that, let me know and I'll make sure it passes the test suite again. If so, I might make a similar change to the C++ version.

yukibtc added a commit that referenced this issue Sep 13, 2023
… test suite, fix issues found by test suite
@yukibtc yukibtc closed this as completed Sep 13, 2023
@yukibtc
Copy link
Member

yukibtc commented Sep 13, 2023

Thanks for the fix!

Days ago I added a unit test that check if the output of the reconciliation was the same that I received from C++ (and effectively it was the same), but using the harness test you found an issue...
Would be awesome to have another automated test to check if the C++ and Rust version match. Have you any idea how to do that? Or how to improve the current unit test?

https://github.com/yukibtc/rust-negentropy/blob/master/negentropy/src/lib.rs#L767

@yukibtc
Copy link
Member

yukibtc commented Sep 13, 2023

The code seems fast as before also with the VecDeque: ~100 ms per 1 million ids, ~40 ms without the hex encoding.

@hoytech
Copy link
Contributor Author

hoytech commented Sep 13, 2023

Glad to hear! I agree it would be good to incorporate the reference test suite into the rust library, but it's quite a lot of extra stuff that would need to get pulled into your repo. And you'd need to compile the C++ code too. Maybe there is some more minimal integration we can do, I will think about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants