Feature/es voting stat plugin #1983

Dimfred · 2019-09-07T10:02:18Z

Sry for the last PR (#1971), here is now the clean one.

Like said, still not fully mature.

added also switch cases in later commis replaced case label with static constexpr method from abstract object

pmconrad · 2019-09-08T08:27:43Z

still not fully mature.

Note that you can create "draft" pull requests.

pmconrad

How does the new plugin affect replay time

when turned off?
when turned on (without ES-objects plugin)?

libraries/chain/include/graphene/chain/voting_statistics_object.hpp

libraries/chain/db_maint.cpp

libraries/chain/include/graphene/chain/voteable_statistics_object.hpp

pmconrad · 2019-09-08T08:39:47Z

libraries/chain/include/graphene/chain/database.hpp

+         /**
+          * Emitted after the beginning of the maintenance interval
+          */
+         fc::signal<void(uint32_t)> on_maintenance_begin;


Nice, this resolves #211.

pmconrad · 2019-09-08T08:40:41Z

libraries/chain/include/graphene/chain/database.hpp

+         /**
+          * Emitted after the calculation of the voting_stake (occurs in the maintenance interval)
+          */
+         fc::signal<void(const account_object&, const account_object&, const uint64_t)> on_voting_stake_calculated;


Hm, unsure if we want to add such a specific signal. @abitmore @jmjatlanta?

At a glance I don't like it. Is there any way to avoid this?

I need all the data to properly feed the plugin, so AFAIK there is no way to avoid this. Everything is performed inside the vote_tally_helper and nothing is given outside.

What exactly do you don't like? Is the signal signature too specific?

To make it more generic I could switch the emission signature to emit an object with the data. This would make it extensible. It would be up to the signal receiver to use the data or not.

If I understood correctly, void(const account_object&, const account_object&, const uint64_t) means we need to emit the signal once per account, which IMHO is too many. I think it's easier if we add the voting_stake data directly into account_statistics_object as a member variable, although this would need a bit more memory.

Yes you are right. The signal is emitted once per account.

Inserting data in account_statistics_object would mean double tracking of the data. Also it would then insert always (or make a check whether the plugin is active, but this seems weird IMHO). But the plugin kinda looses its purpose then.

What would be if I gathered the data first and then emit the signal with all the data? This would call the signal only once and wouldn't require more memory to be used. Though still would require a check whether the plugin is active or not.

libraries/plugins/es_objects/es_objects.cpp

libraries/plugins/voting_stat/include/graphene/voting_stat/voting_stat_plugin.hpp

libraries/protocol/include/graphene/protocol/types.hpp

pmconrad · 2019-09-08T09:02:21Z

tests/CMakeLists.txt

@@ -52,4 +52,8 @@ target_link_libraries( es_test
                       graphene_es_objects graphene_egenesis_none graphene_api_helper_indexes
                       fc ${PLATFORM_SPECIFIC_LIBS} )

+file( GLOB VOTING_STAT_SOURCES "voting_stat/*.cpp" )
+add_executable( voting_stat_test ${COMMON_SOURCES} ${VOTING_STAT_SOURCES} )


Please move general tests into normal test suite and ES-specific tests into ES test suite. A separate executable will most likely not be run by developers.

Okay will do that.

pmconrad · 2019-09-11T05:45:59Z

libraries/plugins/voting_stat/voting_stat_plugin.cpp

+      _maint_block = block_num;
+      _create_voteable = true;
+   }
+   ++_maint_counter;


This will not work properly. If there's a fork around the maintenance interval, the counter may be incremented multiple times for a single maintenance interval, and not consistently across all nodes.

Hm now this is a bit of a problem. But if I would just remove that (ignore the fact that we then can't control the number of objects we track) wouldn't there be the same problem? That different objects will be pushed to es from different nodes?

Is there some form of signal which could be applied on_fork? That would decrement the counter?

No, but you can detect the switch to a fork in applied_block - block numbers should increase strictly monotonically, i. e. if the new block number is the same or lower than the previous you know that the chain is switching to a fork.

Ah okay. Understood. Will rebuild that.

I have still some problems with that. E.g. on_maintenance_begin is called and the plugin routine is performed. Now a fork appears, which leads to another maintenance interval.
AFAIU the plugin routine should now be applied to the fork maintenance.
So when something like that happens I need to revert the changes from the previous interval and apply the plugin routine to the fork interval. Please correct me if I am wrong.

Actually knowing that the chain is switching to a fork is not enough, because in extreme cases a fork can span more than one maintenance interval.
A catch-all solution would be to store the counter in a database field instead of using an instance variable. Database entries are rolled back automatically when switching to a different fork.

Okay I see. I'll try to figure that out. But in such cases there should also be problems with es_objects plugin I think. For example I have added a forbid_delete_flag which forbids the es_objects delete operation for vote*_objects (so it's possible to delete them in the plugin but still store them in es). Meaning a fork appears, rerolls the objects, which should normally delete them (probably). But this doesn't happen.

But in general I think I understood your solution. I will create a db object which keeps track of the counter.

Meaning a fork appears, rerolls the objects, which should normally delete them (probably). But this doesn't happen.

Correct. IIRC we decided that it doesn't matter because objects will eventually be overwritten by new objects with the same ID.

Ah indeed, you are right. Okay then I think I got no further question for now. Thanks for the help.

Dimfred added 8 commits September 7, 2019 11:57

added new signals to database, calling signals from maintenance

19cc469

added voting_statistics_object

f1329e6

added voteable statistics object

b690a33

added voting_stat_plugin

7c5c124

added vot*_objs to es_objects

4336e00

added also switch cases in later commis replaced case label with static constexpr method from abstract object

added test

07d0a81

add here?

43ef521

static space_type for abstract_object (optional)

47861c9

Dimfred force-pushed the feature/es-voting-stat-plugin-clean branch from c214135 to 47861c9 Compare September 7, 2019 10:05

abitmore added this to In development in Protocol Upgrade Release (5.0.0) via automation Sep 7, 2019

abitmore added this to the 4.1.0 - Feature Release milestone Sep 7, 2019

pmconrad suggested changes Sep 8, 2019

View reviewed changes

Dimfred added 8 commits September 10, 2019 14:23

added voting stat to plugin README

2fe34b1

moved voting stat obj to plugin dir, changed ids to match plugin space

3fa5581

db maint removed unnecessary inlude

e00daae

db notify removed question

c0f3def

moved voteable obj to plugin dir and changed ids to match plugin space

8ff6922

chnaged the plugin space removed objs from types

3736c90

es objs

c53bf9a

(temp) moved test to standard bench and removed es tests

b53b254

Dimfred force-pushed the feature/es-voting-stat-plugin-clean branch from 0f0ad85 to b53b254 Compare September 10, 2019 12:28

pmconrad reviewed Sep 11, 2019

View reviewed changes

changed the plugin space

72587d3

Dimfred force-pushed the feature/es-voting-stat-plugin-clean branch 3 times, most recently from 5ca7486 to c4a679d Compare October 21, 2019 13:24

maint counter object added, changed plugin accordingly

f47e5ba

Dimfred force-pushed the feature/es-voting-stat-plugin-clean branch from c4a679d to f47e5ba Compare October 21, 2019 13:30

abitmore added this to In development in Feature Release (5.1.0) via automation Sep 13, 2020

abitmore removed this from In development in Protocol Upgrade Release (5.0.0) Sep 13, 2020

abitmore modified the milestones: 5.0.0 - Protocol Upgrade Release, 5.1.0 - Feature Release Sep 13, 2020

abitmore linked an issue Sep 13, 2020 that may be closed by this pull request

[Feature Request] Allow plugins to add tasks to chain maintenance #211

Open

abitmore modified the milestones: 5.1.0 - Feature Release, 5.2.0 - Feature Release Dec 31, 2020

abitmore removed this from In development in Feature Release (5.1.0) Jan 6, 2021

abitmore added this to In development in Feature Release (5.2.0) via automation Jan 6, 2021

abitmore removed this from In development in Feature Release (5.2.0) Mar 14, 2021

abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Mar 14, 2021

abitmore modified the milestones: 5.2.0 - Feature Release, Future Feature Release Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/es voting stat plugin #1983

Feature/es voting stat plugin #1983

Dimfred commented Sep 7, 2019 •

edited by abitmore

pmconrad commented Sep 8, 2019

pmconrad left a comment

pmconrad Sep 8, 2019

pmconrad Sep 8, 2019

abitmore Sep 9, 2019

Dimfred Sep 10, 2019 •

edited

abitmore Sep 10, 2019

Dimfred Sep 10, 2019

pmconrad Sep 8, 2019

Dimfred Sep 10, 2019

pmconrad Sep 11, 2019

Dimfred Sep 11, 2019 •

edited

pmconrad Sep 11, 2019

Dimfred Sep 11, 2019

Dimfred Sep 30, 2019

pmconrad Sep 30, 2019

Dimfred Oct 1, 2019

pmconrad Oct 1, 2019

Dimfred Oct 1, 2019

Feature/es voting stat plugin #1983

Are you sure you want to change the base?

Feature/es voting stat plugin #1983

Conversation

Dimfred commented Sep 7, 2019 • edited by abitmore

pmconrad commented Sep 8, 2019

pmconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimfred Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimfred Sep 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimfred commented Sep 7, 2019 •

edited by abitmore

Dimfred Sep 10, 2019 •

edited

Dimfred Sep 11, 2019 •

edited