protocols, retrieval: swap-enabled messages implement Price

[holisticode]

This PR changes the way a message type is marked as accounted (enabled for swap accounting).

So far, the protocol would be queried with the message type, and then the protocol was responsible to return if a message was enabled for swap accounting or not.

This change simplifies this by just requiring accounted message types to implement the protocols.Price interface. The accounting module in p2p/protocols then just tries a type case to Price; if it succeeds, it will continue by accounting the price, if it fails it returns assuming the message needs no accounting.

This removes the need for reflection on the protocol's current Price implementation and catches bugs where an accounted message type would fail if it is a value type and not a pointer, as happened in the first retrieval iteration.

We should force to use pointers as message types on every protocol

[janos]

I like the approach and I did not think that it would require so few changes. But, tests are failing mostly because NewAccounting constructor needs to be updated.

[holisticode]

Yesssss so sorry I only had tested the swap package....
I am updating the p2p/protocols package with all tests and simulations now :)

[janos]

Nice. :) This approach is better than using reflections and maps with types, for me.

[mortelli]

i know this is still a draft, but i was curious enough to take a look and frankly, i think this is a great change.

i just have one question: how do you think this will fare against the idea of eventually getting message prices through an oracle?

[holisticode]

@mortelli getting the price through oracles is not affected in any way different than the current code.

It was accessed via a constant in swap/prices.go, and it is now the same way.
The redesign of accessing message prices via oracle needs to take care of this.

I can imagine a service on swap which contains all prices, and the Price method of each message type could be querying that service for the price, just as it is asking the constant now.

[zelig]

great change. looking at the code maybe best time to add constructors to price:

SenderPays(1)
ReceiverPays(1).PerByte()

[Eknir]

Good refactor!

I made one comment which will make your code to make use of go's typing system and effectively transform a run-time error into a compile-time error.

Apart from that, it would be good to make a comment for both the Chunkdelivery struct, as well as the RetrieveRequest struct implement the PricedMessage interface.

[Eknir]

As this function can only succeed by passing a PricedMessage, we can make it a compile-time error if no priced-message is passed instead of a run-time error when we replace msg interface{} with msg PricedMessage.

I just tried this locally and got an error

network/retrieval/retrieve.go:115:15: cannot use protocols.NewAccounting(balance) (type *protocols.Accounting) as type protocols.Hook in assignment:
	*protocols.Accounting does not implement protocols.Hook (wrong type for Send method)
		have Send(*protocols.Peer, uint32, protocols.PricedMessage) error
		want Send(*protocols.Peer, uint32, interface {}) error

which is resolvable by updating the Hook interface (p2p/protocols/protocol.go).

Additionally, this will make the body of the function also more concise.

Same holds for the Receive implementation in p2p/protocols.go/accounting.go:139 => change the function to accept only a PricedMessage type.

[mortelli]

keep in mind that both of the Send and Receive functions return error variables.

when you pass a message which does not implement the PricedMessage interface, the code returns nil. this means success—there is no price, therefore no accounting to be done, therefore no error at this level.

in other words, we should go ahead and process the protocol message if it's not a priced one.

what's more: by only accepting priced messages, we would be blocking every other message from that protocol from being processed. if the protocol has an accounting hook set, it will call Send and Receive for all its messages, and all should be able to be processed whether they are priced or not.

i would like to know more about this error you are getting, can you give more information?

[zelig]

@Eknir, if you do this, you need to implement Price for every message of the protocol.
Plus Send and Receive are generic protocol level hooks with generic type signature, accounting is just one such hook (since no other exists, this argument is somewhat weak, but nontheless in line with the design.)

[Eknir]

Thanks @mortelli and @victor . I was somewhat misled because I had the idea that both of these functions were not intended to be called by a message which does not need accounting. If that is not the case, my argument obviously does not hold.

I dived a bit more in the code and I see that indeed, the accounting.Send() function is called with many different message types (p2p/protocol/protocol.go) and that is because of this is needed that the function can be called also by messages for which no price is defined.

@mortelli , the error I am getting is because I changed the type of msg in the Send function, but I did not change it in the protocols.Hook (p2p/protocols/protocols.go:117) definition.
This makes Accounting (p2p/protocols/accounting.go) to not anymore implement the Hook interface, leading to the error.

[Eknir]

Please consider my comment with regards to letting the Send and the Receive functions accept a PricedMessage type instead of the empty interface type.

[Eknir]

LGTM!

[holisticode]

@zelig #1785