Message interfaces #122

sergefdrv · 2019-10-15T13:44:54Z

This pull request introduces an abstract interface between a particular representation of protocol messages and their usage by the core protocol logic.

This decouples the core protocol code from a particular way the protocol messages are represented, e.g. from automatically generated code of Protocol Buffers. It is advantageous to make this separation before introducing more complex message types as those of the view change operation.

nhoriguchi · 2019-10-23T06:19:54Z

core/internal/messagelog/mocks/mock.go

@@ -34,23 +34,27 @@ func (m *MockMessageLog) EXPECT() *MockMessageLogMockRecorder {
 }

 // Append mocks base method
-func (m *MockMessageLog) Append(arg0 *messages.Message) {
+func (m *MockMessageLog) Append(arg0 *protobuf.Message) {
+	m.ctrl.T.Helper()


Is this .Helper() line relevant to current change? I know this and other changes on mock files are automatically generated by make generate, but if so why not included all changes generated by make generate? If you have specific reason, please leave some comment on it. Or committing all make generate changes is OK, you can do it first at separate patch, then update message interface.

I think I just never looked at changes to auto-generated files. But this commit touches too many files, so the changes should be better minimal. I will update the old generated mock files in a separate commit.

nhoriguchi · 2019-10-23T06:30:10Z

messages/api.go

+//
+//	type IsSealed struct{}
+//
+//	func (IsSealed) isSealed() {}


"Sealed interface" concept is new to me, but it sounds to strictly control visibility of the type, which should be good for code robustness. Anyway this patch only provides interfaces and I can't say anything about how each interface has each method for now.

This concept is admittedly tricky, but seems to be the most reliable way to enforce explicit implementation of interfaces in Go (see golang/go#34996).

I was thinking maybe combine those type IsFoo struct{} and their methods at the end of the file.

Anyway this patch only provides interfaces and I can't say anything about how each interface has each method for now.

True, I just wanted to share the interfaces before I proceed with implementation. Now I have some implementation (not yet used), so I can share that, too.

This concept is admittedly tricky, but seems to be the most reliable way to enforce explicit implementation of interfaces in Go (see golang/go#34996).

You're trying to improve Golang itself, wow 😲

To have some fun at night 😆

nhoriguchi · 2019-10-23T06:33:27Z

messages/api.go

+
+type Message interface {
+	encoding.BinaryMarshaler
+}


This works for decoupling logic and representation, so I'm looking forward to see the concrete type.

ynamiki

Looks good to me.

sergefdrv · 2019-10-24T21:07:14Z

Summary of changes in #122 (commits):

Regenerated old mock files in separate commit
Added message implementation

sergefdrv · 2019-10-24T21:08:49Z

Sorry, I'll fix the DCO issue with the next update...

nhoriguchi · 2019-10-25T08:02:02Z

Added message implementation

Thanks for the update. Reply message still unimplemented, is that intentional?

sergefdrv · 2019-10-26T19:54:10Z

Reply message still unimplemented

I seem to have finished implementing messages half-way 🙂 My plan was to get your feedback on this while working on some other patches for view change (that will also fix #110). Those changes might affect how the message interfaces will be used in the core protocol logic.

nhoriguchi · 2019-10-28T07:14:51Z

messages/protobuf/impl.go

@@ -34,14 +34,20 @@ func (*impl) NewFromBinary(data []byte) (messages.Message, error) {
 		return nil, fmt.Errorf("failed to unmarshal message wrapper: %s", err)
 	}

-	switch msg.Type.(type) {
+	switch t := msg.Type.(type) {
+	case *Message_Request:


This function switches based on message type extracted from raw message data, but it's seems that the sealed interface message.IsRequest is not used here.

https://github.com/sergefdrv/minbft/blob/8245dae27eba1a6bdb4592aac367daec48ba92b8/messages/protobuf/request.go#L22-L25

protobuf.request embeds messages.IsRequest, so I guess that you realize the new thing on this type. So if you explain how it's used, the concept will get clearer.

That's what I try to convey in the "explicit interfaces" proposal: The sealed interfaces is a tricky solution to the problem.

Here, msg is a Message structure which is defined by auto-generated code. Protobuf's code generator protoc defines a oneof field Message.Type as an unexported interface isMessage_Type with a single unexported method isMessage_Type(). The whole purpose of isMessage_Type interface is to choose which concrete types can be assigned to the field. This is close to the sealed interfaces construction, but within the same Go package.

Thus, this type switch determines the unmarshaled message type relying on protobuf's definitions. This should work as expected, because the switch cases match concrete types (structures, not interfaces) and protoc guarantees the expected semantic of such type switches/assertions.

However, the returned value of this function is an abstract interface messages.Message, defined in messages/api.go. That value's type should embed one of IsXxx structures defined in messages/api.go. The value returned by newRequest() in the following line is of request type, which embeds messages.IsRequest. This embedding ensures that request implements the unexported methods of messages.Request (the only purpose of messages.IsRequest). The remaining methods of messages.Request are implemented by request itself so that it fulfils messages.Request interface.

nhoriguchi · 2019-10-28T07:19:13Z

messages/protobuf/request_test.go

+	"github.com/hyperledger-labs/minbft/messages"
+)
+
+func TestRequest(t *testing.T) {


I guess that some test code using isRequest() to check the type elegantly will be added later?

I think there is no need to test isRequest() because impl.NewRequest() simply cannot return anything not implementing messages.Request interface, which requires isRequest() method. It is checked by the compiler. Moreover, isRequest() is unexported in messages package, so it cannot be invoked here.

OK, so this method is only used to make some type to fulfill the interface. How useful it is will be
clearer with view-change patch.

sergefdrv · 2019-10-28T20:02:41Z

Summary of changes:

Fixed DCO issue
Added implementation of Reply message

nhoriguchi

The change set looks good to me, so I think it's ready to merge.

Summary of changes:

This makes me review easier (nice practice), thank you.

sergefdrv · 2019-10-29T08:59:20Z

The change set looks good to me, so I think it's ready to merge.

I initially planned to merge it after final changes for switching the core to use the message interfaces. That's why I opened this pull request ad "draft".

This makes me review easier (nice practice), thank you.

In fact, the link is just a copy of the one attached to the "force-pushed" text (which doesn't look like a link):

nhoriguchi · 2019-10-30T01:40:40Z

The change set looks good to me, so I think it's ready to merge.

I initially planned to merge it after final changes for switching the core to use the message interfaces. That's why I opened this pull request ad "draft".

"Draft" status was removed when I clicked "ready for review" button to update my review status. I'm not sure that this was the right review process, so sorry if it surprised you. I leave this PR open for you to add the final changes, or to merge current PR branch.

sergefdrv · 2019-11-05T21:00:29Z

It looks like the official recommendation for resolving ambiguities among similar interfaces, as opposed to using sealed interfaces, is to augment interfaces with so-called qualifying methods. This would look like follows.

type ReplicaMessage interface {
	Message
	ReplicaID() uint32

	ImplementsReplicaMessage()
}

// ...

type Reply interface {
	ReplicaMessage
	// ...

	ImplementsReply()
}

type reply struct {
	// ...
}

func (m *reply) MarshalBinary() ([]byte, error) { /* ... */ }

func (m *reply) ReplicaID() uint32 { /* ... */ }

// ...

func (reply) ImplementsReply() {}
func (reply) ImplementsReplicaMessage() {}

What do you think of this alternative?

ynamiki · 2019-11-06T01:10:36Z

Thanks for the info. I'd rather like the official way even both are not perfect. It needs less codes, is easier to read for me, and is in the official FAQ.

nhoriguchi · 2019-11-06T02:03:43Z

Implements* methods in your example are global so visible to external packages. It seems not what you originally wanted, but if you think that's acceptable, that's fine and I'll go with it.

sergefdrv · 2019-11-06T09:06:19Z

I'd rather like the official way even both are not perfect. It needs less codes, is easier to read for me, and is in the official FAQ.

I agree that following an official recommendation should make that easier for others to understand it. Though, I'm not sure if it needs less code: Note that reply struct needs to implement both ImplementsReply and ImplementsReplicaMessage. Maybe in our case it's not a big overhead.

Implements* methods in your example are global so visible to external packages. It seems not what you originally wanted, but if you think that's acceptable, that's fine and I'll go with it.

What was desired was to avoid reply struct satisfying messages.ReplicaMessage interface just because it happens to implement ReplicaID method. The "qualifying methods" approach also achieves that goal. The "sealed interfaces" approach is a bit more strict in the sense that the only way to implement messages.Reply is to embed messages.IsReply provided by messages package. So it resembles nominative typing rather than regular Go's structural typing.

Basically the actual problem I tried to solve is perfectly captured on Wikipedia:

A pitfall of structural typing versus nominative typing is that two separately defined types intended for different purposes, but accidentally holding the same properties (...), could be considered the same type by the type system, simply because they happen to have identical structure.

So far, I don't have a strong opinion on which approach to follow. Maybe I adjust the code to follow the "qualifying methods" approach, and we'll see how it looks like. Sleep on it, then decide 🙂

ynamiki · 2019-11-07T01:04:23Z

I'm not sure if it needs less code

It is my understanding that the sealed interface needs an additional type definition (IsReplicaMessage, etc.), which is not in the official recommended way.

I don't have a strong option and either is fine.

sergefdrv · 2019-11-10T21:12:15Z

I tried to adjust the code for qualifying methods approach. Please have a look to see which approach you prefer.

ynamiki · 2019-11-12T06:50:37Z

Thanks, I prefer the qualifying methods approach.

nhoriguchi · 2019-11-13T05:28:01Z

Basically the actual problem I tried to solve is perfectly captured on Wikipedia:

A pitfall of structural typing versus nominative typing is that two separately defined types intended for different purposes, but accidentally holding the same properties (...), could be considered the same type by the type system, simply because they happen to have identical structure.

So far, I don't have a strong opinion on which approach to follow. Maybe I adjust the code to follow the "qualifying methods" approach, and we'll see how it looks like. Sleep on it, then decide

OK, so let's go with the "qualifying methods" approach. It's nice to keep the commit about "sealed interface" because the project history has the record and that allows us to revisit easier in the future.

I'm OK to merge your branch, but the latest commit has "RFC" in the subject, so you may make the final change.

sergefdrv · 2019-11-13T09:27:52Z

@ynamiki @Naoya-Horiguchi Thanks for the feedback! The recommended approach indeed doesn't seem too bad, let's stick to it.

I'm OK to merge your branch, but the latest commit has "RFC" in the subject, so you may make the final change.

I would still prefer to merge it when I make the interfaces actually used by the core... Please let me know if you need it sooner.

sergefdrv · 2019-12-09T20:37:42Z

Summary of changes:

Rebased
Updated commit message of "messages: Use qualifying methods instead of sealed interfaces"

(More commits will come soon.)

nhoriguchi · 2019-12-12T10:50:51Z

A few comments:

I think the final commit ("client,core: Use message interfaces") is the most interesting one since last review, and it looks good to me as long as I checked (it's a little too large to check line-by-line.)
One question: there are many conversions like *messages.Request -> messages.Request especially in definitions of function parameters, is that necessary change for your current purpose? Maybe my point is unclear, but I'm a little worried that that could introduce unexpected behaviors.
Patch 12/13 ("core: Restructure TestMakeReplicaMessageProcessor ") has some broken indentations, although that will be fixed by patch 13/13.

sergefdrv · 2019-12-12T12:23:01Z

@Naoya-Horiguchi Thanks for reviewing this.

I think the final commit ("client,core: Use message interfaces") is the most interesting one since last review, and it looks good to me as long as I checked (it's a little too large to check line-by-line.)

Sorry for making the commit so large, but I couldn't find a better way to do the switch without breaking building/testing.

One question: there are many conversions like *messages.Request -> messages.Request especially in definitions of function parameters, is that necessary change for your current purpose? Maybe my point is unclear, but I'm a little worried that that could introduce unexpected behaviors.

Before the switch, Request was a structure (from messages/protobuf package), after the switch it is an interface (from messages package), thus the declarations change. This must be a pretty safe change, because the compiler detects this kind of type mismatch.

Patch 12/13 ("core: Restructure TestMakeReplicaMessageProcessor ") has some broken indentations, although that will be fixed by patch 13/13.

Thanks, I think I missed it when resolving rebase conflicts. That's easy to fix 🙂

nhoriguchi · 2019-12-13T01:40:14Z

Before the switch, Request was a structure (from messages/protobuf package), after the switch it is an interface (from messages package), thus the declarations change. This must be a pretty safe change, because the compiler detects this kind of type mismatch.

Make sense, thank you very much. So I'm totally fine about proposed patches ~~with the indent fix~~(you already did that). 👍

ynamiki

I had a question but it is solved.

One (slightly off-topic) question: do we have a nice tool to support refactoring that has been done in the last commit? I guess changing a struct to an interface is a common refactoring method and this cause tedious and extensive code modification. It is hard to do that manually.

core/message-handling.go

nhoriguchi · 2019-12-15T13:30:04Z

One (slightly off-topic) question: do we have a nice tool to support refactoring that has been done in the last commit? I guess changing a struct to an interface is a common refactoring method and this cause tedious and extensive code modification. It is hard to do that manually.

I have no idea about specific tools, but maybe some modern editor (VSCode in my mind) helps for the purpose. For example, if you change some code around struct definition, VSCode automatically (in real time manner) checks whole repository and marks all type mismatches as warnings. So you can adjust all affected places with confidence.

nhoriguchi · 2019-12-16T02:32:34Z

I tried to resolve conflict, but I found that I had to change core/request_test.go (not listed in the conflicting files) to pass the unit tests, so I think we can't simply use Web UI and we might have to push origin/master via CLI. Should I do this? The resolved branch passes the CI test.

sergefdrv · 2019-12-16T16:17:43Z

do we have a nice tool to support refactoring that has been done in the last commit? I guess changing a struct to an interface is a common refactoring method and this cause tedious and extensive code modification. It is hard to do that manually.

@ynamiki I think gofmt could help with this job, but this time I actually followed compiler errors and fixed most of them with regex substitution...

sergefdrv · 2019-12-16T16:22:03Z

I tried to resolve conflict, but I found that I had to change core/request_test.go (not listed in the conflicting files) to pass the unit tests,

Maybe we better first finalize and merge #142, then rebase this PR.

so I think we can't simply use Web UI and we might have to push origin/master via CLI.

I am not sure how good the Web UI is for this kind of job 🙂

Should I do this? The resolved branch passes the CI test.

No worries, I'll take care of this PR.

ynamiki · 2019-12-17T01:48:37Z

do we have a nice tool to support refactoring that has been done in the last commit?

Thanks to both of you for reply. I understand there are not a one-shot method and we need to update code manually referencing compiler errors.

Signed-off-by: Sergey Fedorov <[email protected]>

Protocol Buffers is only one of possible mechanisms to serialize messages. To prepare for decoupling the core code from a specific implementation of message serialization, move the code into a corresponding sub-package. Signed-off-by: Sergey Fedorov <[email protected]>

The protocol logic should not depend on particular implementation of message representation, e.g. on automatically generated code. Define abstract interface for message types to decouple the usage of messages from their particular representation. Signed-off-by: Sergey Fedorov <[email protected]>

Signed-off-by: Sergey Fedorov <[email protected]>

The officially recommended way [1] to require types to explicitly declare that they implement an interface is to add a special method with a descriptive name to the interface's method set. Then the type must have an (empty) implementation of that method, in order to implement the interface. This technique is also known as "qualifying methods". [1]: https://golang.org/doc/faq#guarantee_satisfies_interface Use qualifying methods to ensure the desired semantics of type switches/assertions for message interfaces. Signed-off-by: Sergey Fedorov <[email protected]>

Signed-off-by: Sergey Fedorov <[email protected]>

View change will introduce more protocol message types that have to be processed according to the current view number. Generalize viewMessageProcessor to allow for this extension. Signed-off-by: Sergey Fedorov <[email protected]>

Message interfaces will not require each implementation to provide a method for getting embedded messages. Message interfaces will be accompanied with a helper function to get embedded messages. Restructure the test to prepare for this change. Signed-off-by: Sergey Fedorov <[email protected]>

Now message interfaces are defined and implemented, switch the core and client code to use those interfaces as opposed to directly manipulating Protocol Buffers auto-generated code. Signed-off-by: Sergey Fedorov <[email protected]>

sergefdrv · 2019-12-18T13:39:45Z

Summary of changes:

Rebase with conflict resolution

…repare-timer Resolve conflict with changes from "Message interfaces hyperledger-labs#122". Signed-off-by: Naoya Horiguchi <[email protected]>

sergefdrv requested review from nhoriguchi and ynamiki October 15, 2019 13:44

nhoriguchi reviewed Oct 23, 2019

View reviewed changes

ynamiki approved these changes Oct 24, 2019

View reviewed changes

sergefdrv force-pushed the message-interfaces branch from d23e28a to 8245dae Compare October 24, 2019 21:02

nhoriguchi reviewed Oct 28, 2019

View reviewed changes

sergefdrv force-pushed the message-interfaces branch from 8245dae to 762ae1a Compare October 28, 2019 20:00

nhoriguchi marked this pull request as ready for review October 29, 2019 00:29

nhoriguchi approved these changes Oct 29, 2019

View reviewed changes

sergefdrv force-pushed the message-interfaces branch from feb001e to e94a066 Compare December 9, 2019 20:34

sergefdrv requested review from nhoriguchi and ynamiki December 10, 2019 22:17

sergefdrv force-pushed the message-interfaces branch from 14337be to f11add8 Compare December 12, 2019 12:26

ynamiki approved these changes Dec 14, 2019

View reviewed changes

core/message-handling.go Show resolved Hide resolved

Sergey Fedorov added 13 commits December 18, 2019 11:39

Regenerate gomock files

08d8780

Signed-off-by: Sergey Fedorov <[email protected]>

messages/protobuf: Add implementation stub

b6ad989

Signed-off-by: Sergey Fedorov <[email protected]>

messages/protobuf: Implement Request message

d818250

Signed-off-by: Sergey Fedorov <[email protected]>

messages/protobuf: Implement Prepare message

bd3381e

Signed-off-by: Sergey Fedorov <[email protected]>

messages/protobuf: Implement Commit message

869d846

Signed-off-by: Sergey Fedorov <[email protected]>

messages/protobuf: Implement Reply message

6f7e131

Signed-off-by: Sergey Fedorov <[email protected]>

messages: Add mocks for testing

f5e54ba

Signed-off-by: Sergey Fedorov <[email protected]>

client,core: Use message interfaces

8984c84

Now message interfaces are defined and implemented, switch the core and client code to use those interfaces as opposed to directly manipulating Protocol Buffers auto-generated code. Signed-off-by: Sergey Fedorov <[email protected]>

sergefdrv force-pushed the message-interfaces branch from f11add8 to 8984c84 Compare December 18, 2019 13:37

nhoriguchi merged commit 7ec9ad4 into hyperledger-labs:master Dec 19, 2019

This was referenced Dec 20, 2019

Fix starting prepare timer #144

Merged

Include message type to signed payload #6

Closed

sergefdrv deleted the message-interfaces branch January 20, 2020 14:30

Message interfaces #122

Message interfaces #122

Uh oh!

Conversation

sergefdrv commented Oct 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ynamiki left a comment

Choose a reason for hiding this comment

Uh oh!

sergefdrv commented Oct 24, 2019

Uh oh!

sergefdrv commented Oct 24, 2019

Uh oh!

nhoriguchi commented Oct 25, 2019

Uh oh!

sergefdrv commented Oct 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergefdrv commented Oct 28, 2019

Uh oh!

nhoriguchi left a comment

Choose a reason for hiding this comment

Uh oh!

sergefdrv commented Oct 29, 2019

Uh oh!

nhoriguchi commented Oct 30, 2019

Uh oh!

sergefdrv commented Nov 5, 2019

Uh oh!

ynamiki commented Nov 6, 2019

Uh oh!

nhoriguchi commented Nov 6, 2019

Uh oh!

sergefdrv commented Nov 6, 2019

Uh oh!

ynamiki commented Nov 7, 2019

Uh oh!

sergefdrv commented Nov 10, 2019

Uh oh!

ynamiki commented Nov 12, 2019

Uh oh!

nhoriguchi commented Nov 13, 2019

Uh oh!

sergefdrv commented Nov 13, 2019

Uh oh!

sergefdrv commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nhoriguchi commented Dec 12, 2019

Uh oh!

sergefdrv commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nhoriguchi commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sergefdrv commented Dec 9, 2019 •

edited

Loading

sergefdrv commented Dec 12, 2019 •

edited

Loading

nhoriguchi commented Dec 13, 2019 •

edited

Loading