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

Fix vote republishing rules #663

Closed
wants to merge 1 commit into from
Closed

Fix vote republishing rules #663

wants to merge 1 commit into from

Conversation

PlasmaPower
Copy link
Contributor

@PlasmaPower PlasmaPower commented Feb 23, 2018 •
edited
Loading

The primary change in this PR is to make the cooldown per block per representative, as opposed to the previous per block cooldown.

Once widely deployed, this should really help with the slow receive times since it'll be easy to hit the quorum threshold.

Edit: this now also fixes duplicate sequence number detection.

@PlasmaPower PlasmaPower mentioned this pull request Feb 23, 2018
Closed
@PlasmaPower
Copy link
Contributor Author

I'm trying to decide on a good value for the min republish vote weight. Right now I'm using 10k XRB. Previously the limit was 256 XRB, which was way too small in my opinion.

@PlasmaPower
Copy link
Contributor Author

Weight to cooldown distribution rewritten in terms of supply, and changed to be more restrictive (min weight percent for rebroadcast is now 0.1% of supply, currently including 31 reps).

androm3da
androm3da reviewed Feb 23, 2018
View reviewed changes
rai/node/node.hpp Outdated
@@ -57,7 +57,7 @@ class election : public std::enable_shared_from_this<rai::election>
rai::uint128_t minimum_threshold (MDB_txn *, rai::ledger &);
rai::votes votes;
rai::node & node;
std::chrono::steady_clock::time_point last_vote;
std::map<rai::account, std::chrono::steady_clock::time_point> last_votes;
Copy link
Contributor

Choose a reason for hiding this comment

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

probably could/should be an unordered_map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

androm3da
androm3da reviewed Feb 23, 2018
View reviewed changes
rai/node/node.cpp Outdated
rai::transaction transaction (node.store.environment, nullptr, true);
auto supply (node.ledger.supply (transaction));
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 it's ideal to put the units here. These are both raw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Units aren't really important here though, as everything is in proportions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition, the rest of the codebase uses and assumes raw.

@cryptocode
Copy link
Contributor

cryptocode commented Feb 23, 2018 •
edited
Loading

Some stats before and after PR 663 and 664.

Before:

calls                  : 7000
skip due to time       : 5733
skip due to weight     : 84

After:

calls                  : 7000
skip due to time       : 0
skip due to weight     : 6227

By skip, I mean republish_vote -> confirm_send not getting called.

@PlasmaPower
Copy link
Contributor Author

Based on @cryptocode's numbers, this PR seems to decrease the number of votes rebroadcasted. In my opinion that's a good sign, as we're properly prioritizing votes by weight.

@PlasmaPower
Copy link
Contributor Author

Despite the decrease in rebroadcasted votes from one node, this PR will still probably increase bandwidth. Currently, the network isn't sending nodes votes with high weight, so very few votes are being rebroadcasted by this PR. After this PR is widely deployed, nodes will be getting votes with high weight, which will likely all be rebroadcasted.

@PlasmaPower PlasmaPower force-pushed the fix/vote-republish-rules branch 3 times, most recently from 7df2ff7 to e5e363e Compare February 26, 2018 03:08
@PlasmaPower
Copy link
Contributor Author

Undid cache_vote->vote_max rename as I came full circle on the design of the function.

@PlasmaPower PlasmaPower force-pushed the fix/vote-republish-rules branch 4 times, most recently from 2aa3fcd to 6ff9620 Compare February 26, 2018 13:22
@PlasmaPower
Copy link
Contributor Author

Now testing the cooldown: https://github.com/nanocurrency/raiblocks/pull/663/files#diff-045a181bb917e4bc71081327d83285da

@PlasmaPower
Copy link
Contributor Author

It's important to note that vote rebroadcasting rules and vote processing rules have been merged, so if a vote won't be rebroadcasted (e.g. cooldown not up, vote weight too low) it won't be processed.

I'm not entirely sure if that's the right idea, but I think it makes some theoretical attacks harder.

@PlasmaPower PlasmaPower force-pushed the fix/vote-republish-rules branch 2 times, most recently from 7bface6 to c68a105 Compare February 26, 2018 17:59
clemahieu
clemahieu reviewed Mar 15, 2018
View reviewed changes
rai/node/node.cpp Outdated
// This tries to assist rep nodes that have lost track of their highest sequence number by replaying our highest known vote back to them
// Only do this if the sequence number is significantly different to account for network reordering
// Amplify attack considerations: We're sending out a confirm_ack in response to a confirm_ack for no net traffic increase
if (vote.vote->sequence - message_a.vote->sequence > 10000)
if (vote.vote->sequence + 10000 > message_a.vote->sequence)
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 this needs to be vote.vote->sequence > message_a.vote->sequence + 10000.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

@PlasmaPower
Copy link
Contributor Author

This has been merged with some modifications: 63e15c2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clemahieu clemahieu clemahieu left review comments

@androm3da androm3da androm3da left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PlasmaPower @cryptocode @clemahieu @androm3da