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

Configurable message id function #248

Merged
merged 2 commits into from
Dec 16, 2019
Merged

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Dec 16, 2019

See #247: this PR is fully backwards compatible, no public API changes, except for the addition of:

  • configuration method ChangeMsgIdFn SetMsgIdFn to MessageCache. The cache doesn't really need a single message id function from the start, so changing it with a method seems fine. And this way it doesn't break any existing code. Note that users of caches like this can update it once they get hold of the exact msg id function to use. E.g. gossipsub updates it when PubSub attaches the router. So it also avoids a painful early design choice of PubSub getting all options, and gossipsub not being able to adapt based on these Options, before initializing PubSub.
  • introduction of MsgIdFunction, which has the same signature as the previous private msgID helper, but now it's configurable! The old msgID is now DefaultMsgIdFn.
  • a PubSub Option created with WithMessageIdFn to apply your own message ID function to PubSub :)

PS: I thought this was an easy and important issue to just fix it myself, and although I have some previous experience hacking on this repo (I looked deep into its concurrency and simulation for eth2 tooling for aggregation modeling), I can definitely appreciate a review since I don't normally contribute code upstream here.

Closes #247

cc @AgeManning

@protolambda
Copy link
Contributor Author

Edit: force pushed over previous commit, for some reason Go decided I wanted the old multi addr dependency version in here. Removed that from the diff.

@vyzo vyzo self-requested a review December 16, 2019 10:01
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

LGTM modulo a small nit regarding the naming of ChangeMsgIdFn.

mcache.go Outdated Show resolved Hide resolved
@vyzo vyzo changed the title fixes #247: implement msg id function as pubsub option Configurable message id function Dec 16, 2019
pubsub.go Show resolved Hide resolved
@vyzo
Copy link
Collaborator

vyzo commented Dec 16, 2019

We have a small issue with the ordering of options regarding the message id function, as seen by the tracer.
I think we might have to modify the tracer inline when present in the option.

pubsub.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

thank you!

@vyzo vyzo merged commit 1d5191c into libp2p:master Dec 16, 2019
@raulk
Copy link
Member

raulk commented Dec 16, 2019

May want to make a release for @protolambda and @prestonvanloon to be able to test this change against rust.

@vyzo
Copy link
Collaborator

vyzo commented Dec 16, 2019

Yeah, release forthcoming.

@Nashatyrev
Copy link

Is it the case that Go gossip implementation lacks client validation functionality?
@vyzo may be you can comment here ethereum/consensus-specs#1528 (comment) ?

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.

Optional Content addressed messages
4 participants