search

interledger / interledger-rs

An easy-to-use, high-performance Interledger implementation written in Rust
http://interledger.rs
Other
201 stars 70 forks source link

Bytes 05 upgrade #690

Closed koivunej closed 3 years ago

koivunej commented 3 years ago

Fixes #626 as far as I can see. This builds upon #688. The OER stuff is a lot of pain and confusion but I managed to remove btp::oer module altogether, meaning that packet::oer extensions will now be used all over the workplace. There are quite the number of fuzzing failures in packet and btp this PR does not fix the next will. I did quite a bit off fuzzing guided testing to make sure not a lot of differences in errors were introduced, and panics were fixed.

This is a breaking change on the whole as public api of interledger-packet is modified at least by bytes version. Removals in interledger-btp didn't touch public items.

After this and the follow-up PR(s) it will be easier but not easy to continue with the bytes = "1.0" upgrade towards tokio-1.0.

koivunej commented 3 years ago

The whole change in this PR is a breaking change but there may have been others. I could mark the commits if they are commented on in review?

koivunej commented 3 years ago

Turned this into a draft as there are likely some serialization breaking changes already, would probably need to handle the cleanups and removals first in a separate PR and add more tests.

koivunej commented 3 years ago

There's a bit of semi-avoidable churn in the commits as I didn't see it form beginning that interledger-btp::oer could be removed altogether.

This only adds the test cases found with fuzzing, not the fuzzing support itself. I've been fuzzing using roundtrip tests, which parse some bytes as a type (ilp or btp), if the parsing succeeds, it will serialize them back to bytes and make sure the bytes match original input. Some parts of this work are already included, like btp contenttype becoming roundtrippable in 5985cf4. The remaining non-roundtripping allowing features are related to reading varuints which use excessive leading bytes; those would always be written out with the minimal form but the reader side does not care if excess bytes are being used.

The reason why I added so much testing during this work was to do as good as due diligence as possible, since bytes04 and bytes05 are different in a single major way: bytes05 panics if the offset received by split_* fns does not exist as in the buffer is short. bytes04 just creates an empty Bytes and allows this. Please see 7753f5c and b914b26.

koivunej commented 3 years ago

Audit failure awaits for #689.

koivunej commented 3 years ago

@pradovic could you give a new review pass? I did cut some stuff out of the tail, but also head lost some rework. This no longer has any of the fuzzing related work, but I'll PR them in right after this.

koivunej commented 3 years ago

Thanks! For me this is good to merge on monday. I'll open the follow-up PR with fuzzing next.

koivunej commented 3 years ago

Assuming enough time has passed, merging.