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

Confirmation solicitor revamp #2472

Merged
merged 11 commits into from
Jan 15, 2020
Merged

Confirmation solicitor revamp #2472

merged 11 commits into from
Jan 15, 2020

Conversation

guilhermelawless
Copy link
Contributor

@guilhermelawless guilhermelawless commented Jan 10, 2020 •
edited
Loading

Being a revamp this is best reviewed in split mode. The changes are:

  • Fix for getting stuck in the loop
  • Requests ordering is now respected to ensure prioritization
  • Batch count was global and not per-channel
  • confirmation_request_count no longer used to alternate between elections, instead now doing it time-based. Using a counter + modulo was not great.
  • Deleted all uses of single confirm_req since there is more than enough weight on TCP now. This can however be reverted.
  • Added a flag to disable active_transactions to allow for testing the solicitor
  • Added a basic test for the solicitor

Eventually, it seemed more efficient to get rid of the request class and do most of the logic in ::add() and have ::flush() only batch and dispatch the messages. Overall I think the solicitor has still simplified a lot of the logic, but I could not escape making it more complicated while still making sure the behavior is the same as before.

Rate-limiting logic is left in active_transactions

@guilhermelawless guilhermelawless added bug functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality labels Jan 10, 2020
@guilhermelawless guilhermelawless added this to the V21.0 milestone Jan 10, 2020
@guilhermelawless guilhermelawless self-assigned this Jan 10, 2020
wezrule
wezrule reviewed Jan 10, 2020
View reviewed changes
nano/node/confirmation_solicitor.cpp Outdated Show resolved Hide resolved
nano/node/confirmation_solicitor.cpp Outdated Show resolved Hide resolved
nano/node/election.hpp Outdated Show resolved Hide resolved
nano/node/confirmation_solicitor.cpp Outdated Show resolved Hide resolved
nano/node/confirmation_solicitor.cpp Show resolved Hide resolved
SergiySW
SergiySW reviewed Jan 12, 2020
View reviewed changes
nano/node/nodeconfig.hpp Outdated Show resolved Hide resolved
@guilhermelawless
Split solicitor add into broadcast+add and bring back the logic to ac…
ec0efd2 
…tive_transactions

This brings back rate-limitting logic and modifying election variables to active_transactions only. Timings are also slightly adjusted:

- Only 2 requests required before starting to flood blocks
- Timings for test network
@guilhermelawless
Rename flag
d934780
@guilhermelawless
Only broadcast OR request confirmation in the same loop for the same …
701410a 
…election
@guilhermelawless
Copy link
Contributor Author

Last commits bring the rate-limiting logic back to active_transactions and elections are now given as const ref to the solicitor.

@guilhermelawless guilhermelawless added the blocker Some future items cannot be completed until this is merged. label Jan 13, 2020
wezrule
wezrule reviewed Jan 15, 2020
View reviewed changes
nano/node/confirmation_solicitor.cpp
Comment on lines +6 to +8
nano::confirmation_solicitor::confirmation_solicitor (nano::network & network_a, nano::network_constants const & params_a) :
max_confirm_req_batches (params_a.is_test_network () ? 1 : 20),
max_block_broadcasts (params_a.is_test_network () ? 4 : 30),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could remove network_constants dependency and pass in bool is_test_network instead, but I don't think it's that important. Haven't tested it, but otherwise LGTM

@guilhermelawless guilhermelawless merged commit 0bc041e into nanocurrency:develop Jan 15, 2020
@guilhermelawless guilhermelawless deleted the confirmation-solicitor-fixes branch January 15, 2020 15:05
wezrule pushed a commit to wezrule/raiblocks that referenced this pull request Jan 22, 2020
@guilhermelawless @wezrule
Confirmation solicitor revamp (nanocurrency#2472)
6fe4ec9 
* Optional minimum version when querying representatives from crawler

* Revamping confirmation_solicitor to mimick previous active_transactions behavior

* Use a time-based approach to throttle confirmation requests and block flooding

* Addressing Wesley review

* Remove unusued node.hpp include (thanks wes)

* Simplify logic by using unordered_map::operator[] which calls the default constructor if not found

* Split solicitor add into broadcast+add and bring back the logic to  active_transactions

This brings back rate-limitting logic and modifying election variables to active_transactions only. Timings are also slightly adjusted:

- Only 2 requests required before starting to flood blocks
- Timings for test network

* Rename flag

* Only broadcast OR request confirmation in the same loop for the same election

* Enclose lambda in clang-format off
wezrule pushed a commit to wezrule/raiblocks that referenced this pull request Jan 22, 2020
@guilhermelawless @wezrule
Confirmation solicitor revamp (nanocurrency#2472)
8a41e49 
* Optional minimum version when querying representatives from crawler

* Revamping confirmation_solicitor to mimick previous active_transactions behavior

* Use a time-based approach to throttle confirmation requests and block flooding

* Addressing Wesley review

* Remove unusued node.hpp include (thanks wes)

* Simplify logic by using unordered_map::operator[] which calls the default constructor if not found

* Split solicitor add into broadcast+add and bring back the logic to  active_transactions

This brings back rate-limitting logic and modifying election variables to active_transactions only. Timings are also slightly adjusted:

- Only 2 requests required before starting to flood blocks
- Timings for test network

* Rename flag

* Only broadcast OR request confirmation in the same loop for the same election

* Enclose lambda in clang-format off
wezrule added a commit that referenced this pull request Jan 24, 2020
@wezrule @clemahieu
@guilhermelawless @argakiig @SergiySW @cryptocode
Node telemetry (#2446)
1edb8c6 
* Node telemetry

* Add genesis block

* Don't checkif at the end of the stream with UDP to support compatibility with different message versions

* Add single request and optional blocking

* Formatting

* Use nano locks

* Fix clang build

* Out of date comments

* Update some names of classes/functions/variables

* Fix for disconnected peer channels & some cleanup

* Remove unnecessary lock requirement in active_transactions::lock. (#2475)

* Remove unnecessarry lock requirement in active_transactions::lock.
This also removes some code-smell around passing in a bool to determine if the lock should actually be acquired.

* Using a timeout counter in line with other test idioms.

* Confirmation solicitor revamp (#2472)

* Optional minimum version when querying representatives from crawler

* Revamping confirmation_solicitor to mimick previous active_transactions behavior

* Use a time-based approach to throttle confirmation requests and block flooding

* Addressing Wesley review

* Remove unusued node.hpp include (thanks wes)

* Simplify logic by using unordered_map::operator[] which calls the default constructor if not found

* Split solicitor add into broadcast+add and bring back the logic to  active_transactions

This brings back rate-limitting logic and modifying election variables to active_transactions only. Timings are also slightly adjusted:

- Only 2 requests required before starting to flood blocks
- Timings for test network

* Rename flag

* Only broadcast OR request confirmation in the same loop for the same election

* Enclose lambda in clang-format off

* Beta network reset #2 (#2476)

* Delete rep_weights_beta.bin

* New genesis

* Change header_magic_number to one never used before

* use v1.1 for actions-aws-cli (#2486)

* The start of CLI tests (#2403)

* The start of CLI tests

* Add needed header file after merge

* Serg review comments

* Websocket bootstrap subscription (#2471)

* Bootstrap attempt ID
* Websocket bootstrap subscription

* Change processed blocks factor for requeued pulls (#2437)

From `processed blocks / 10000` to `processed blocks / 4096` for better processing of largest chains in case of failures

* Upgrade confirmation height table to include cemented frontier (#2481)

* Upgrade confirmation height table to include cemented frontier

* Stein review comments

* Add //clang-format block around lambda

* Update comment (thanks Gui)

* Upgrade to v17 instead

* Update comments

* Request aggregator (#2485)

* Request aggregator

Adds a class that runs in a new thread to pool confirmation requests by endpoint.
This allows a reduction of bandwidth, vote generation, and moves some vote generation out of the I/O threads.

* Use a constant for confirm_ack_hashes_max

* Small code simplification

* Use const transaction

* Disable clang-format for lambdas

* Add missing deadlines and update deadline before poll block

* Use a scoped lock_guard pattern and initialize start in-class

* Misc. fixes and documentation

* use clang-format-8 (#2490)

There is some funkiness with clang-format-9 and lambdas that are not
honored

* Formatting fix so clangformat 8 applies cleanly (#2491)

* Support multiple work peers in the same host (#2477)

* Correct check for peers when creating work

* Support multiple work peers in the same address

* Send cancels despite errors to account for non-conforming implementations

* Add tests using a fake work peer, acting as good, malicious or slow

* Comment

* Formatting

* Fix multiple response error in RPC

* Formatting

* Add extra error handling when specifying port and not address with the RPC (and vice versa)

* Formatting

* Serg comments

* Formatting

* Mask not needed

* Missed file check in for mask removal

* Fix test failures

* Gui suggestions

* Formatting

* Fix assert with test on Windows and some cleanup

* Lower number of nodes in node_telemetry.many_nodes when using sanitizers

Co-authored-by: clemahieu <clemahieu@gmail.com>
Co-authored-by: Guilherme Lawless <guilherme.lawless@gmail.com>
Co-authored-by: Russel Waters <vaelstrom@gmail.com>
Co-authored-by: Sergey Kroshnin <sergiysw@gmail.com>
Co-authored-by: cryptocode <stein@nano.org>
@zhyatt zhyatt removed the blocker Some future items cannot be completed until this is merged. label Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clemahieu clemahieu clemahieu approved these changes

@wezrule wezrule wezrule approved these changes

@SergiySW SergiySW Awaiting requested review from SergiySW

Assignees

@guilhermelawless guilhermelawless

Labels
bug functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality
Projects
None yet
Milestone
V21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@guilhermelawless @clemahieu @wezrule @SergiySW @zhyatt