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

Add bitswap block encoding/decoding to ocaml #10579

Merged
merged 4 commits into from
Apr 28, 2022

Conversation

nholland94
Copy link
Member

Closes #9451.

src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
Copy link
Member

@georgeee georgeee left a comment

Choose a reason for hiding this comment

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

Also, what do you think of cross-testing both implementations on random samples of data?

Like, adding a new RPC for testing purposes to query libp2p helper for blocks generated for the data provided.

And then checking that representation by both versions of the code is same.

src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
src/lib/mina_net2/bitswap_block.ml Outdated Show resolved Hide resolved
@nholland94 nholland94 added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Mar 27, 2022
src/app/libp2p_helper/src/libp2p_helper/bitswap_msg.go Outdated Show resolved Hide resolved
src/app/libp2p_helper/src/libp2p_helper/bitswap_msg.go Outdated Show resolved Hide resolved
in
loop ls >>| List.concat
loop ls (return []) >>| List.rev >>| List.concat
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we rewrite above with fold instead of manual recursive loop ?

Copy link
Member Author

@nholland94 nholland94 Mar 31, 2022

Choose a reason for hiding this comment

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

Because a fold won't terminate if acc returns error (will continue to iterate regardless). I could use a fold_until instead, but the implementation would be much uglier I fear.

let size = Bigstring.length chunk in
last_leaf_block_data_size :=
if !last_leaf_block_data_size = 0 then size
else min !last_leaf_block_data_size size
Copy link
Member

Choose a reason for hiding this comment

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

I understand the logic here, like leaf is the smallest block and for it queue is empty. But use of min strikes my sight, could we get rid of it?

Copy link
Member

Choose a reason for hiding this comment

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

Although it's just a test and isn't so important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the min was just the easy way to make this work, since the "last leaf" isn't the last block you traverse here.

src/lib/mina_net2/libp2p_helper.ml Outdated Show resolved Hide resolved
@nholland94 nholland94 force-pushed the feature/bitswap-block-format-ocaml branch from aed20a9 to e5daaa6 Compare April 27, 2022 16:38
@nholland94 nholland94 merged commit c791bcf into compatible Apr 28, 2022
@nholland94 nholland94 deleted the feature/bitswap-block-format-ocaml branch April 28, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch proj-network-stability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants