Add committee change events

[shargon]

Close #2763

[erikzhang]

We called GetCommitteeAddress twice but no one pay for that. I think it's easy to call GetCommitteeAddress off-chain, so maybe we don't need this notification.

[ixje]

The point of the notification is to have the important events retrievable via the ApplicationLogs. For MainNet this calculation happens once every 21 blocks, so the computation cost seems negligible to me.

An alternative could be

            if (ShouldRefreshCommittee(engine.PersistingBlock.Index, engine.ProtocolSettings.CommitteeMembersCount))
            {
                var old_committee = GetCommitteeFromCache(engine.Snapshot).Select(tuple => tuple.PublicKey).ToArray();

                StorageItem storageItem = engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Committee));
                var cachedCommittee = storageItem.GetInteroperable<CachedCommittee>();
                cachedCommittee.Clear();
                cachedCommittee.AddRange(ComputeCommitteeMembers(engine.Snapshot, engine.ProtocolSettings));

                var new_committee = cachedCommittee.Select(tuple => tuple.PublicKey).ToArray();
                if (!new_committee.SequenceEqual(old_committee))
                {
                    // notify list of new public keys
                }

The advantage with notifying the public keys list is that whomever consumes the notification can identify who in the committee changed (without having to make another RPC request to get the new committee list via getCommittee.

[roman-khimov]

The committee address itself is something used often enough (UpdateExtensibleWitnessWhiteList() for example) to be always calculated here (if ShouldRefreshCommittee()) and then cached somewhere to be reused until it changes (and it can only change here again).

Still for this event I'd rather use a list of keys, this gives more possibilities to the consumer of it. Maybe accompanied by validators/committee addresses.

[ixje]

Still for this event I'd rather use a list of keys, this gives more possibilities to the consumer of it. Maybe accompanied by validators/committee addresses.

List of keys sounds good to me. Addresses could be a plus, but if compute is a concern I'd be happy to leave that off and let the consumer do the transformation.

[shargon]

It won't be slower to check if it was changed?

[roman-khimov]

The list is sorted.

[ixje]

How about always sending the list of keys if ShouldRefreshCommittee() is True and leaving the comparison between old and new committee to the consumer? We can name the event "CommitteeRefreshed" with a list of public keys as parameters. Something like this

if (ShouldRefreshCommittee(engine.PersistingBlock.Index, engine.ProtocolSettings.CommitteeMembersCount))
            {
                ...

                cachedCommittee.Clear();
                cachedCommittee.AddRange(ComputeCommitteeMembers(engine.Snapshot, engine.ProtocolSettings));

                engine.SendNotification(Hash, "CommitteeRefreshed",
                    new VM.Types.Array(engine.ReferenceCounter) { cachedCommittee.Select(tuple => tuple.PublicKey).ToArray() });

This is the least compute you can probably get

[shargon]

How about always sending the list of keys if ShouldRefreshCommittee() is True and leaving the comparison between old and new committee to the consumer? We can name the event "CommitteeRefreshed" with a list of public keys as parameters. Something like this

It' good for performance, but then why we want the keys? Just call GetCommitteeAddress if you want

[ixje]

It' good for performance, but then why we want the keys? Just call GetCommitteeAddress if you want

GetCommitteeAddress is not a public contract method...but let's assume it is made public; a committee address cannot (within reasonable effort) be reverted to figure out who is actually part of the committee (a.ka. the public keys). So what useful information does that address then give us? In my opinion nothing more then that the committee changed to something else and then we might as well drop the address if that's all it tells us.

In my opinion the most ideal solution is a "CommitteeChanged" event which says exactly which public key was added and which public key was removed. With that a consumer (i.e. a statistics site) can start tracking which address was part of the committee for how long, its earnings during that period and what not. Given that this undoubtedly costs too many nanoseconds to pass the compute scrutiny here I proposed to just dump the public key list and let the consumer do the computations. That solution is also questioned...so what is your real concern? Is it applicationlog storage size? Because if that's the concern shouldn't we first be talking about the inefficient format of the applicationlog? I can easily reduce i.e. the "Transfer" event format size by 50% while simultaneously making it more digestible for the consumer without using compression to store it. And that's an event that is literally happening millions of times vs something that is happening once every 21 blocks...

[shargon]

We called GetCommitteeAddress twice but no one pay for that.

I'm agree with @ixje and I assume that is part of the persist logic, I will work on this after merge (#2942)

[roman-khimov]

I'd rather emit a list of keys, it's more data and then committee/validator address can be derived from it easily.

[superboyiii]

@shargon @erikzhang Let's move on?

[Jim8y]

Close as inactive

[shargon]

I would like to do this, let's wait

[vncoelho]

This is a very simple change. I think you can update and let's proceed @shargon

[shargon]

@vncoelho Wait for #2942 because of conflicts

[roman-khimov]

Still, why not a key list? It greatly expands possible uses of this data and we have the list anyway. Can be combined with address or can be not (eliminating the question of address calculation cost).

[shargon]

Still, why not a key list? It greatly expands possible uses of this data and we have the list anyway. Can be combined with address or can be not (eliminating the question of address calculation cost).

We can emit the key list and expose the address #3154

[shargon]

Closed in favor of #3158