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 ValidatorData field to Message #231

Merged
merged 1 commit into from
Nov 15, 2019
Merged

add ValidatorData field to Message #231

merged 1 commit into from
Nov 15, 2019

Conversation

vyzo
Copy link
Collaborator

@vyzo vyzo commented Nov 15, 2019

Useful for passing data from the validation to the application.

Useful for passing data from the validation to the application.
@whyrusleeping
Copy link
Contributor

Thanks @vyzo !

@@ -151,6 +151,7 @@ type PubSubRouter interface {
type Message struct {
*pb.Message
ReceivedFrom peer.ID
VaidatorData interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

@vyzo where is this field set, did you forget to add a file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is this just a convenience thing since the data is already available by emitting data from the validators?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is for validators to set, and for subscribers to consume. Basically, I'd like to avoid having to unmarshal things twice

Copy link
Collaborator Author

@vyzo vyzo Nov 15, 2019

Choose a reason for hiding this comment

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

it is initialized to nil when we construct the message; it's for use from validators, they can set it to pass data to the application (eg. the unmarshalled payload to avoid unmarshalling twice).
This is a filecoin ask.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vyzo Ok, no problem with convenience functions.

Although, I'd like to point out that in less than 30 minutes you've just passed a change that causes problems if there are multiple validators for a single message since they may both clobber the ValidateData field. There is also no documentation for this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

This completely breaks the abstraction of pubsub as a shared service. There's no reason we can't have validators return additional data (along with an error) and have the validation subsystem collect all the extra validation data.

Copy link
Contributor

@fxfactorial fxfactorial May 14, 2020

Choose a reason for hiding this comment

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

So what's the proper usage of this field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It allows the validation to pass data to the application; one common usage (eg in lotus) is to pass the decoded message to avoid parsing and decoding twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that and that’s my intent as well , just seems like there was an extended convo about it and wondering if any issues or assumptions need to be held when using it this way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue in the convo was about multiple validators competing for the coveted field, but it's a very rare occurrence to have a message that validates in multiple topics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants