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.

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

Fix vote check #1233

Merged
merged 8 commits into from Sep 27, 2018

Conversation

Projects
None yet
3 participants
@rkeene
Copy link
Contributor

commented Sep 25, 2018

Fix supplied in #964 was incorrect, updated

@rkeene rkeene added the bug label Sep 25, 2018

@rkeene rkeene added this to the V17.0 milestone Sep 25, 2018

@rkeene rkeene self-assigned this Sep 25, 2018

@rkeene rkeene requested a review from SergiySW Sep 25, 2018

@SergiySW
Copy link
Collaborator

left a comment

Now comparision is correct. Except possible situation if node is receiving last vote several times, but this should be rare

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

I think we should get rid of that part entirely. Why's it necessary?

@rkeene

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I think we need to update the vote_max() interface to return a vote_code so we know if it came from the DB/cache or the current vote was inserted (vote) or not (replay)

@rkeene rkeene added the incomplete label Sep 26, 2018

@rkeene rkeene force-pushed the rkeene:fix-vote-check branch from 55aaa76 to a31f1dd Sep 26, 2018

@rkeene rkeene requested a review from SergiySW Sep 26, 2018

Show resolved Hide resolved rai/node/lmdb.cpp Outdated

@rkeene rkeene removed the incomplete label Sep 27, 2018

@rkeene

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

After looking into node.active.vote() (rai::active_transactions::vote()) more, I think just removing the consideration altogether is good. It will return true if a replay OR the vote is being ignored because it is being too spammy (from existing->election->vote(), aka rai::election::vote()).

@rkeene

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

The reason this code was correct before #964 was because it would always evaluate the second par of the if (... || ...) expression as false and thus irrelevant

@rkeene rkeene merged commit cb7f3f0 into nanocurrency:master Sep 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

argakiig added a commit to argakiig/raiblocks that referenced this pull request Oct 12, 2018

Merge in master (#7)
* Fixed config.json file permissions (nanocurrency#1191)

* Bootstrap only from nodes that could be possibly up-to-date given vote-by-hash (nanocurrency#1184)

This will disable bootstrapping from nodes that are very likely out of date since they can only be updated during periodic bootstrapping, which is too slow.

* Added support for PENDING_HASH_AMOUNT_AND_ADDRESS to bulk_pull_account (nanocurrency#1201)

* Swallow exceptions regarding changing filesystem permissions if the user knows better than us (nanocurrency#1203)

* Service UDP receive socket more frequently (nanocurrency#1210)

* Resize stack to 8MB to match Linux (nanocurrency#1213)

* Abstracted setting file permissions into platform-specific code (nanocurrency#1215)

* Ensure sum of all votes is greater than the online minimum (nanocurrency#1157)

Ensure sum of all votes is greater than the online minimum before attempting to get confirmation on a block

* Clean-up of Abstraction of setting permissions on Windows (nanocurrency#1216)

* Expand the beta network acceptable range of protocols (nanocurrency#1217)

* Allow watch-only accounts json import (nanocurrency#1211)

* Improve node.quick_confirm test (nanocurrency#1222)

* Retry failed core test a few times for 90 days until we can fix inconsistent tests (nanocurrency#1218)

* Start elections for processed blocks after transaction commit (nanocurrency#1219)

* Better `--snapshot` CLI error messages (nanocurrency#1224)

* Release assert on DB failures (nanocurrency#1226)

* Add a test for bulk_pull_account (nanocurrency#1227)

* Increase the base backoff for our test workaround

* Ensure the async_write lambda is a closure around the buffer (nanocurrency#1231)

* Fix max vote sequence comparison (nanocurrency#964)

* Node breakup: port mapper (nanocurrency#1038)

* Additional statistics about protocol messages (nanocurrency#1234)

* In broadcast_confirm_req case, limit to 32 peers (nanocurrency#1230)

In the basic broadcast_confirm_req case, limit to 32 representatives
or peers.  This will limit the outgoing bandwidth.

This changes the semantics of broadcast_confirm_req to require the
caller to call it again if the first call didn't achieve the desired
results.  In practice, this is already done because it may not reach
any peers (indeed there may be none connected).

* Republish unconfirmed blocks with delay (nanocurrency#1232)

* Basic confirm_req scheduling of batch improvement (nanocurrency#1229)

* Copy processed_active before dequeuing it (nanocurrency#1239)

This fixes nanocurrency#1235

* Fix vote check (nanocurrency#1233)

This was broken in nanocurrency#964

*  Escalation for long unconfirmed elections (nanocurrency#1237)

* Change default desktop config.json to reduce load on slow PCs (nanocurrency#1247)

* Updated script to generate source tarball and changelog (nanocurrency#1249)

* Better build prep scripts (nanocurrency#1248)

* Revert " Escalation for long unconfirmed elections" (nanocurrency#1253)

Reverts pull request nanocurrency#1237

* Fix deadlock for elections escalation (nanocurrency#1254)

* Check epoch_link before checking balance of state blocks (nanocurrency#1255)

* Move blockstore visitors to remove lmdb dependency (nanocurrency#1186)

* Add a max time to batch processes in "process_receive_many" (nanocurrency#1256)

*  Remove generate_hash_votes_at from config (nanocurrency#1170)

* Remove generate_hash_votes time from config

* Clear blocks_bundle

* Default 0 for generate_hash_votes_at v14

* republish_block should only republish block without voting

* Put new version

* Remove unused condition

* Remove unnessesary blocks_bundle.clear ()

* Remove unused transaction

* Remove transaction from network.republish_block

* Fixes

* Returning correct files permissions

* Correct permission for common.sh

* Name threads by role (nanocurrency#1258)

Threads !

* Add vote status stats even when vote logging is off (nanocurrency#1261)

*  Remove processing blocks from confirm_req (nanocurrency#1246)

* Extended bulk.offline_send (nanocurrency#1259)

Keepalive removed. Nodes should be unknown for each other until bootstrap (offline)
More intermediate checks to find issue parts

Currently should fail

* Remove process_active from confirm_req (nanocurrency#1265)

* Factor out logging (nanocurrency#1262)

* Periodic pending search (nanocurrency#1268)

* Fix node.password_fanout test with running beta node (nanocurrency#1269)

* Log common exceptions before asserting (nanocurrency#1278)

* Separate config option for number of network threads (nanocurrency#1276)

* Optimize "*_pending" RPC calls when include_active used (nanocurrency#1277)

* Move port/address parsing to common, where it's declared (nanocurrency#1263)

* Force macOS wallet to use aqua light mode whilst QT adds support for Mojave Dark Mode (nanocurrency#1279)

* Build prep enhancements (nanocurrency#1286)

* Better changelog generation (nanocurrency#1287)

* Fix escalation source block check (nanocurrency#1282)

* Improve pending & unchecked search performance (nanocurrency#1288)

Up to 25-45% performance increase for large wallets search

1 request in for loop instead of 2 requests (if there are no pending/unchecked for given key - most likely scenario).

* converting to boost threads and setting 8MB thread stacks (nanocurrency#1289)

converting to boost threads and setting 8MB thread stack size universally

* Better error messages (nanocurrency#1225)

* Revert stack size linking for windows (nanocurrency#1290)

* Add mutex around peers.get<T>() (nanocurrency#1291)

* Factor out node_config (nanocurrency#1296)

* Make account_history RPC call correctly obey "count" (nanocurrency#1293)

* Include protocol version in "version" RPC call (nanocurrency#1194)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.