Verify serialization of all events

[sondreb]

The SignalR implementation is not able to properly serialize all internal events in Blockcore. I'm fixing the Transaction ones, but this is another one that might fail:

    /// <summary>
    /// A peer message has been received and parsed
    /// </summary>
    /// <seealso cref="EventBase" />
    public class PeerMessageReceived : PeerEventBase
    {
        public Message Message { get; }

        public int MessageSize { get; }

        public PeerMessageReceived(IPEndPoint peerEndPoint, Message message, int messageSize) : base(peerEndPoint)
        {
            this.Message = message;
            this.MessageSize = messageSize;
        }
    }

This should be further investigated and verified.

[dangershony]

I am not sure this is a good idea, this will slow down the sync for sure when the wallet has many trx.
every time the that event is fired we deserialize the trx to hex (and again to get the trx id.

[dangershony]

Whats the problem with serialization? why is i not working?

[sondreb]

The problem is that Transaction does not serialize to JSON during the Web Socket send event, and that type of issue does not trigger exceptions, so it's hard to discover. You simply don't get events out from SignalR. I have not tested all events yet, so we'll keep this issue open until all events has been verified to work properly.

The recent commit improves performance by lazy-getting the transaction ID, and the hex we won't return in the events. That is something that the consumer can request in separate call.

[dangershony]

ah right right in that case I think maybe we don't propagate the eventbus types to signalR or just what you did is fine.

[MithrilMan]

@sondreb it's not strictly an improvement because if multiple part listen to that event and ask for the lazy loaded implementation, your implementation load it twice, that's not the proper way to lazy load.

Also a couple of observations:

• I don't think we should put too much work into events themselves, they are supposed to be lightweight and if more work is required by the receiver it should be done on its own task to not impact normal node operations.

• I think that if the problem is even message serialization, then we have to implement serializator for event messages and not explicitly set some property to get out some property that may be useful in specific scenarios

P.S.
why commit directly without PR? I find it's harder to review and even risky because can be committed something that should require a review

[sondreb]

We're utilizing the Clients.Clients(consumersToInform) method which takes a list of consumers, not sure if that will perform multiple serializations of the object or not.

I still went ahead and optimized it further with a lazy loaded property. in the commit above.

There is zero usage of this Web Socket event API at the moment (at least for the events in question here), so it's hard to know how it would be best to extend or improve the entities. Need more user feedback to consider additional improvements.

One important optimization would be to configure the interval for different events, so you don't get e.g. staking info too often. But not worth spending time on that until we actually have users and issues with too many messages.

This was a quick bug fix so didn't consider doing it as PR, but it's probably on border-line change that it could have been a PR.

[MithrilMan]

ok that's a proper lazyload
I'll try to get some time to view signalr feature implementation to see the impact and how can be refactored to have the least impact possible.

Anyway my consideration about having lightweight event types is that they are public events that can be used by 3rd party and you cannot foresee their usage.
It's important to document properly how eventbus works and how to handle these events (e.g. queuing them in a consumer system.io.pipeline and consume then in its own task) because the event publishing is synchronous and every handler takes its time to handle the event, so if it has to do very little job is fine to handle it synchronously, otherwise go for "publish/subscriber
(pipeline) on its own task" implementation

One important optimization would be to configure the interval for different events, so you don't get e.g. staking info too often. But not worth spending time on that until we actually have users and issues with too many messages.

ok that's an implementation detail of the signalr feature, may be a valid point, I don't have seen its implementation yet
you may even want to have a kind of "aggregator" that aggregates multiple events and produce a single event with all aggregate details