Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Web socket subscription for active difficulty #2091

Merged
merged 12 commits into from
Jun 27, 2019
Merged

Web socket subscription for active difficulty #2091

merged 12 commits into from
Jun 27, 2019

Conversation

chrislinegar
Copy link
Contributor

This implements #2080 - monitors changes in difficulty and notifies via web socket if the active difficulty changes

@chrislinegar chrislinegar force-pushed the active_difficulty-web-socket branch 2 times, most recently from d2df373 to b73bf89 Compare June 16, 2019 20:03
chrislinegar
chrislinegar commented Jun 16, 2019
View reviewed changes
nano/node/active_transactions.cpp
@@ -639,7 +639,12 @@ void nano::active_transactions::update_active_difficulty (std::unique_lock<std::
auto sum (std::accumulate (multipliers_cb.begin (), multipliers_cb.end (), double(0)));
auto difficulty = nano::difficulty::from_multiplier (sum / multipliers_cb.size (), node.network_params.network.publish_threshold);
assert (difficulty >= node.network_params.network.publish_threshold);

bool notify_change = trended_active_difficulty != difficulty;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently implemented to only notify if the active difficulty changes - is this correct or should it notify on every iteration even if active difficulty remains the same?

Copy link
Contributor

@cryptocode cryptocode Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only when it changes I think. Thoughts @zhyatt ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the call is sufficiently lightweight, I could see value in triggering it every cycle, specifically to ensure consistent data points for any tracking/visualizations, and also as a mechanism that allows watching for that consistency. If gaps are seen, this allows polling for difficulty separately through RPC as fallback or alerting of a possible issue (if these notifications aren't seen for a time period). Any additional thoughts @cryptocode or @guilhermelawless ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea, it's very light data especially considering the ~20 second period.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me if it simplifies clients and is low frequency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request_loop runs faster in testing - will publishing on every iteration have an impact on other tests if running faster?:

request_interval_ms = is_test_network () ? (is_sanitizer_build ? 100 : 20) : 16000;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem I believe, there is much higher frequency data being passed around. Tests should pass.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agree with @guilhermelawless, sending it every cycle should be fine.

@chrislinegar chrislinegar mentioned this pull request Jun 16, 2019
Closed
@cryptocode
Copy link
Contributor

Thanks @chrislinegar. Before reviewing this, could you fix the formatting issues reported by CI? There's a clang-format-all script under the ci directory.

@chrislinegar chrislinegar force-pushed the active_difficulty-web-socket branch from b73bf89 to ce5d11f Compare June 17, 2019 07:32
@chrislinegar chrislinegar force-pushed the active_difficulty-web-socket branch from 9fec66b to 9ffc0ea Compare June 17, 2019 16:27
@chrislinegar chrislinegar force-pushed the active_difficulty-web-socket branch from 9ffc0ea to 30892ad Compare June 17, 2019 16:28
@chrislinegar
Copy link
Contributor Author

Thanks @chrislinegar. Before reviewing this, could you fix the formatting issues reported by CI? There's a clang-format-all script under the ci directory.

thanks, done :)

@chrislinegar

This comment has been minimized.

Sign in to view
guilhermelawless
guilhermelawless reviewed Jun 20, 2019
View reviewed changes
cryptocode
cryptocode reviewed Jun 20, 2019
View reviewed changes
nano/node/active_transactions.cpp
@@ -639,7 +639,12 @@ void nano::active_transactions::update_active_difficulty (std::unique_lock<std::
auto sum (std::accumulate (multipliers_cb.begin (), multipliers_cb.end (), double(0)));
auto difficulty = nano::difficulty::from_multiplier (sum / multipliers_cb.size (), node.network_params.network.publish_threshold);
assert (difficulty >= node.network_params.network.publish_threshold);

bool notify_change = trended_active_difficulty != difficulty;
Copy link
Contributor

@cryptocode cryptocode Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only when it changes I think. Thoughts @zhyatt ?

@guilhermelawless
Copy link
Contributor

Current failure you might be seeing is outlined in #2101

cryptocode
cryptocode approved these changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@cryptocode cryptocode left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending formatting issue on CI/2101

@guilhermelawless
Copy link
Contributor

Thank you @chrislinegar . Only needs another pass of clang-format-all

@chrislinegar
Copy link
Contributor Author

Current failure you might be seeing is outlined in #2101

Just to confirm - is there any action needed on this PR for this issue?

@guilhermelawless
Copy link
Contributor

Just to confirm - is there any action needed on this PR for this issue?

No, it's resolved.

@zhyatt
Copy link
Collaborator

zhyatt commented Jun 26, 2019

@chrislinegar Are you considering this PR wrapped up on your end (all comments resolved)?

@zhyatt zhyatt added the documentation This item indicates the need for or supplies updated or expanded documentation label Jun 26, 2019
@zhyatt zhyatt added this to the V19.0 milestone Jun 26, 2019
@cryptocode cryptocode merged commit 311f8cb into nanocurrency:master Jun 27, 2019
@cryptocode cryptocode mentioned this pull request Jun 27, 2019
Merged
argakiig pushed a commit that referenced this pull request Jun 27, 2019
@chrislinegar
Web socket subscription for active difficulty (#2091)
1daeac0 
* Add active_difficulty topic

* Topic to string for active_difficulty

* Add difficulty_changed message builder

* Add difficulty observer

* Notify observer if difficulty changes

* Add test for subscribe_active_difficulty

* Update comment

* Run clang format all

* Use boost ptree double get instead of std::stod

* Rename test and for std::launch::async policy

* Remove difficulty_observer and call node.observers.difficulty directly

* Fix formatting issue
@zhyatt zhyatt added enhancement and removed documentation This item indicates the need for or supplies updated or expanded documentation labels Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhyatt zhyatt zhyatt left review comments

@guilhermelawless guilhermelawless guilhermelawless left review comments

@cryptocode cryptocode cryptocode approved these changes

Assignees
No one assigned
Labels
enhancement websockets
Projects
None yet
Milestone
V19.0
Development

Successfully merging this pull request may close these issues.
4 participants
@chrislinegar @cryptocode @guilhermelawless @zhyatt