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

[Refactoring] Budget, round 2: review locks #1844

Merged
merged 18 commits into from Sep 29, 2020

Conversation

random-zebra
Copy link

Keep going with the masternode-budget classes refactoring, mainly addressing the thread synchronization aspect.

There is currently a potential deadlock due to wrong lock order between cs_main and cs_budgets, which will be solved in a follow-up PR as it needs a more invasive refactoring (decoupling the checks in UpdateValid/IsCollateralTransactionValid, doing those that don't depend on the chainstate just once, and not requiring cs_main held for the others).

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Obvious concept ACK with a good beer 🍺 .
Left few comments up until a21500d (non inclusive), will go steady checking everything :).

src/masternode-budget.cpp Outdated Show resolved Hide resolved
src/masternode-budget.cpp Show resolved Hide resolved
src/masternode-budget.cpp Outdated Show resolved Hide resolved
src/masternode-budget.cpp Show resolved Hide resolved
@random-zebra random-zebra force-pushed the 202009_budget_locks branch 2 times, most recently from 2ad9def to 6006a05 Compare September 16, 2020 15:33
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review up until 11dc169 (non inclusive).
Left few more comments, overall the code is looking much better.

src/masternode-budget.cpp Show resolved Hide resolved
src/masternode-budget.cpp Outdated Show resolved Hide resolved
src/masternode-budget.cpp Outdated Show resolved Hide resolved
src/masternode-budget.cpp Outdated Show resolved Hide resolved
@random-zebra
Copy link
Author

Rebased.

furszy
furszy previously approved these changes Sep 23, 2020
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

awesome round 2 :D. You are killing it!
ACK b89c5a7.

left a tiny nit, not needed for this PR.

src/masternode-budget.cpp Show resolved Hide resolved
mapSeenMasternodeBudgetProposals --> mapSeenProposals
mapSeenMasternodeBudgetVotes --> mapSeenProposalVotes
mapOrphanMasternodeBudgetVotes --> mapOrphanProposalVotes
vecImmatureBudgetProposals --> vecImmatureProposals
Finalized budgets need access to proposals for getting their names
(GetProposals)
Access with GetProposalsStr, which replaces GetProposals.
Set it before adding the finalized budget to the map (with
SetBudgetProposalsStr) thus without requiring locks.
GetProposal/GetFinalizedBudget that copy the objects from the map
@random-zebra
Copy link
Author

Rebased.

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.

ACK 3b48041

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

utACK 3b48041 after rebase

@furszy furszy merged commit 7196a35 into PIVX-Project:master Sep 29, 2020
random-zebra added a commit that referenced this pull request Oct 6, 2020
288953d [Refactor] Cleaner GetBlockValue implementation (random-zebra)
0d2fc0a [Bug] Align GetBlockValue calls to CreateCoinStake: current height + 1 (random-zebra)

Pull request description:

  This is a known old bug, which was never truly fixed, due to it being harmless (and discovered after the last block-value reduction).

  The function `GetBlockValue(int nHeight)` returns the block-reward value for a block at a given height (based on a "halving" schedule hardcoded within the function).
  The issue is that it was being called passing the height of the *previous* block.
  At some point, one particular call (in `CreateCoinStake`) was changed to use the (correct) current block height, while leaving the wrong one in all other places (including masternode payments/budgets  and validation code).

  This made no real difference with PIVX, as the change was introduced before the last milestone (which increased the block value from 4.5 to 5, rather than reducing it), and, after that, the block value never changed anymore (so it has always been `GetBlockValue(N) == GetBlockValue(N-1)`).
  But it was an issue for all PIVX forks that had different schedules (or that introduced drastic changes to the masternode/staker-rewards proportions).

  In  #967 @CaveSpectre11 gives a complete explanation of the problem (already discussed on a wrong fix attempt in #814), and proposes to change only the call in `CreateCoinStake`, aligning it to the (wrong) height (mostly for the sake of consistency and for the benefit of future PIVX forks, as it has no effect on PIVX chain, at his current height).

  This PR, instead, proposes the opposite solution. Align all other calls, so that the correct height is passed (`pindex->nHeight` during validation, and `pindexPrev->nHeight + 1` when creating a new block).
  This makes the code consistent with the actual block values on chain.
  For example, the first-6-masternodes-premine was **not** in the genesis block, but at block 1.
  Therefore we should have had `60001 * COIN` returned by  `GetBlockValue(1)`,  not by `GetBlockValue(0)`.
  - main-net <https://explorer.pivx.link/block/000005504fa4a6766e854b2a2c3f21cd276fd7305b84f416241fd4431acbd12d>
  - testnet <https://testnet.pivx.link/block/0000081796e8bf7f8047cc84681ceecb64da781e11961da844d0f8b7c13690a5>

  Thus, to align it with the values on chain  (and prevent issues during initial sync) the schedule defined inside `GetBlockValue` is "shifted ahead" by one block.

  This also refactors `GetBlockValue` to make it more readable.

  Tested Resyncing from scratch on both testnet and mainnet.

  Based on top of
  - [x] #1844

  just to avoid merge conflicts with the refactoring of `CBudgetManager::FillBlockPayee`.
  Only the last two commits are new here.

ACKs for top commit:
  furszy:
    utACK 288953d after rebase.
  Fuzzbawls:
    ACK 288953d

Tree-SHA512: 684bb9aee09067e459abd2bbcb338e1a93d499e76732043e85a89b607d70fc22a778253426ae14763a26be6b93e60a65cb165368ea8b7dbf77d87c9021b993b8
random-zebra added a commit that referenced this pull request Oct 19, 2020
…for proposals / finalized budgets

4a49010 [BUG] Check budget payment history against active chain (random-zebra)
67c4343 [BUG] Fix finalized budget threshold if there's less than 10 active mns (random-zebra)
b65e862 [Bug] Fix masternode threshold 10% != 2 * 5% (random-zebra)
1d19725 [Cleanup] Remove redundant check in CBudgetProposal::CheckStartEnd (random-zebra)
351c088 [Trivial] fix linter issue with string in multiple lines (random-zebra)
741fa3a [BUG] Fix FillBlockPayee when there's no budget with enough votes (random-zebra)
c149407 [BUG] Immediate expiration of finalized budgets (random-zebra)
8866284 [RPC][BUG] Fix 'mnfinalbudget show' returning a single budget (random-zebra)
ddac6ce [Trivial] Serialize strProposals to disk (random-zebra)
25f749a [Refactor] Check confirmations once - remove nBlockFeeTx / nTime (random-zebra)
6b4c503 [Trivial] Log current/feetx height in CheckCollateralConfs (random-zebra)
6ad29bf [Refactor] Don't add immature/unconfirmed proposals - remove vecImmature (random-zebra)
090d708 [Trivial] Rename IsBudgetCollateralValid and don't export it (random-zebra)
ad1e9a2 [Refactoring] Cache block height of proposals collateral (random-zebra)
d4bedbd [Refactor] Perform CFinalizedBudget static checks in AddFinalizedBudget (random-zebra)
5c1208f [Refactor] Decouple CFinalizedBudget::UpdateValid checks (random-zebra)
0f399e6 [Cleanup] Move extra "Budget + name + (proposals)" out of strInvalid (random-zebra)
676139c [Refactor] Perform CBudgetProposal static checks in AddProposal (random-zebra)
a3d4651 [Refactor] Decouple static checks from IsBudgetCollateralValid (random-zebra)
73cdd20 [Cleanup] Remove extra "Proposal + name" from strInvalid (random-zebra)
be2ecf1 [Budget] Additional checks for proposal recipient address (random-zebra)
12d411f [Refactor] Decouple CBudgetProposal::UpdateValid checks (random-zebra)

Pull request description:

  Based on top of:
  - [x] #1843
  - [x] #1844

  Current PR starts with `[Refactor] Decouple CBudgetProposal::UpdateValid checks` (6c6a97e).

  This focuses on extracting context-independent checks and doing them only once in `AddProposal`/`AddFinalizedBudget`.
  As for the confirmation/expiration check (which is the only thing left for `UpdateValid`) , it is done without locking `cs_main` (while holding `cs_budgets`/`cs_proposals`) by caching the height of the block with the collateral transaction and relying on it (todo: it needs to be addressed the case of it being reorganized from the chain).

  This new field, called `nBlockFeeTx` is added directly to the serialization of CBudgetProposal and CFinalizedBudget, so the old budget.dat will be invalidated and recreated new at startup.

ACKs for top commit:
  furszy:
    Code review ACK 4a49010. Going to try it in regtest now.
  furszy:
    Tested, great ACK 4a49010 .
  Fuzzbawls:
    ACK 4a49010

Tree-SHA512: 3248259a76470da1270731b6b3ba1371db05d280cbe70624f9083f76a130f98f0752615205bfda63fb4aa9ae80ff61c343d77ab70db61c7e72bbc3e5eae7e024
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.

None yet

3 participants