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

[budget] partially fixing a race condition that could cause a good peer to be banned #2395

Merged

Conversation

furszy
Copy link

@furszy furszy commented May 18, 2021

Moved TRY_LOCK to LOCK to perform the budget maps check every 14 blocks without exception. If peers doesn't perform the validation and update proposals, votes and budget finalization maps at a similar network time, there could be a peer sending votes that aren't longer valid and end up being banned.

This is just an initial small mitigation, have seen peers banning other peers for this race condition. This area needs a further rework later on.
Budget data is being validated on every new arriving block while the masternodes status is being checked and updated every 5 seconds (which is way too fast and not needed but that is another topic). The budget data update should be done right after the masternode removal/invalidation, so the peer updates the budget maps and does not send the invalid data to the network (important note: better to do this rather than force a budget data check and update on every peer sync request).
Plus will probably work in another PR in decoupling the called "masternodeds thread" into its proper file and update budget data from it as well (solving some cyclic dependencies in the way).

-- This is something to have in mind for DMN and the future PoSe too. The budget maps check and update should be done right after the masternode removal/invalidation and not at different times --

… being banned.

Moved TRY_LOCK to LOCK to perform the budget maps check every 14 blocks without exception. If peers doesn't perform the validation and update proposals, votes and budget finalization maps at the same network time, then there could be a peer sending votes that aren't longer valid and end up being banned.
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Great catch! utACK 7faa5e9

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

utACK 7faa5e9

@furszy furszy merged commit b8ee4be into PIVX-Project:master May 20, 2021
furszy referenced this pull request in furszy/bitcoin-core Jun 18, 2021
… being banned.

Moved TRY_LOCK to LOCK to perform the budget maps check every 14 blocks without exception. If peers doesn't perform the validation and update proposals, votes and budget finalization maps at the same network time, then there could be a peer sending votes that aren't longer valid and end up being banned.

Github-Pull: bitcoin#2395
Rebased-From: 7faa5e9
@random-zebra random-zebra modified the milestones: 6.0.0, 5.2.0 Jun 18, 2021
furszy added a commit that referenced this pull request Jun 19, 2021
3344fb3 [CI] Remove Ubuntu 16.04 from GA workflow, has been deprecated by GA and will be removed on September 20, 2021. (furszy)
b5a8638 [GUI] Cleanup compiler warnings in qtutils.h/cpp (furszy)
73b56e9 [GUI] Fix invisible text due an invalid transparent selection color. (furszy)
df5d820 budget: fixing a possible race condition that could cause a good peer being banned. (furszy)
a5fd073 Fix minimize and close bugs (furszy)
b516e36 depends: update Qt 5.9 source url (Kittywhiskers Van Gogh)
18f4b4a BugFix: fix not updating GUI balance race condition. (furszy)
2c923ed scripted-diff: Replace 'NULL' with 'nullptr' in guiutil.cpp (random-zebra)
7b8b23a Fix memory leaks in qt/guiutil.cpp (Dan Raviv)
cbd5c78 Add search option to My Addresses list in receive widget (Volodia)
0dcbea2 [build] depends macOS: point --sysroot to SDK (Sjors Provoost)
3b542fc [Doc] remove old gitian keys. (furszy)
0f099e8 [GUI] Generate FAQ answer content programmatically (Fuzzbawls)
8483861 qt:Show the entire Window when double clicking on taskbar (ken2812221)
998c7e8 [GUI] fix QT 5.15 `currentIndexChanged(QString)` deprecated method call. (furszy)

Pull request description:

  List of straightforward PRs back ported from v6.0 into the v5.2 branch (ordered by merge date).

  * #2259.
  * #2260.
  * #2348
  * #2350
  * #2353
  * #2305
  * #2374
  * #2379
  * #2384
  * #2377
  * #2395
  * #2401
  * #2413

ACKs for top commit:
  random-zebra:
    utACK 3344fb3
  Fuzzbawls:
    utACK 3344fb3

Tree-SHA512: 4317e83d4c1228b8ae20dc1bc5c8e43ac87598d4d9d9244fdd032f2a0c5eccd1a7ed27bc29094c7411dd653b187c728ab31d732ea686abda15228920a390e4e1
@furszy furszy deleted the 2021_budget_fix_possible_banning_issue branch November 29, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants