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(ipld/plugin): verify that data written to namespaceHasher has legit size #964

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

Wondertan
Copy link
Member

My understanding of the bug right now is that the bitswap msg that was rcvd from the network had the wrong size of inner nodes hashes, and instead of erroring out, our IPLD plugin panics. It's not clear what could cause the wrong msg size under normal conditions, but it's clear that we have to fix it by checking if the sizes of msgs are correct.

Closes #952 and closes celestiaorg/celestia-core#780

@Wondertan Wondertan added the kind:fix Attached to bug-fixing PRs label Jul 28, 2022
@Wondertan Wondertan requested a review from liamsi as a code owner July 28, 2022 16:27
@Wondertan Wondertan self-assigned this Jul 28, 2022
@Wondertan Wondertan requested a review from renaynay as a code owner July 28, 2022 16:27
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Can you make a quick unit test for this?

ipld/plugin/nmt.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

@renaynay, I can make a test that would just test the hasher itself, but FWIW it does not bring any value. To actually test this, we need somehow simulate that over bitswap we rcv a wrong size msg, which is a big PITA

@Wondertan
Copy link
Member Author

Tests were failing due to a typo. Fixed now

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Can you file an issue to add such a test so we don't forget at least?

ipld/plugin/nmt.go Outdated Show resolved Hide resolved
ipld/plugin/nmt.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

An issue to test this out: #981
If we think this is super-duper important, I can spend a day or two writing it. The day or two because it's not straightforward and it may require forking bitswap.

ipld/plugin/nmt.go Outdated Show resolved Hide resolved
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Last nit but is optional

ipld/plugin/nmt.go Outdated Show resolved Hide resolved
@Wondertan Wondertan merged commit bf54f4e into main Aug 2, 2022
@Wondertan Wondertan deleted the hlib/fix-hasher branch August 2, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:fix Attached to bug-fixing PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Crash during syncing full storage node
4 participants