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

consensus, accrual: Fix newbie accrual #2004

Merged

Conversation

jamescowens
Copy link
Member

@jamescowens jamescowens commented Jan 22, 2021

This PR implements CAmount GetNewbieSuperblockAccrualCorrection(Cpid cpid) and is the fix for the newbie accrual bug.

This PR closes #1999.

@jamescowens jamescowens self-assigned this Jan 22, 2021
@jamescowens jamescowens added this to the Hilda milestone Jan 22, 2021
@jamescowens jamescowens force-pushed the fix_newbie_accrual branch 5 times, most recently from 049d210 to 43caa1f Compare January 24, 2021 17:54
@jamescowens jamescowens force-pushed the fix_newbie_accrual branch 3 times, most recently from 9924706 to 1c4f5bb Compare January 24, 2021 22:30
@jamescowens
Copy link
Member Author

Ok. This is getting close I think. You have to test this against mainnet, because the only CPID's that fail are on mainnet right now.

If you run this with a test node against mainnet and delete the accrual directory beforehand to force rebuilding the accruals, then run the auditsnapshot accruals, you get the following:

accrual_mismatches_20210124_after_accrual_rebuild.log

and

TallySuperblockAccrual_debug_out_20210124.log

This is based on having the corrections actually replay at the first superblock higher than the original newbie fix height of 2104000.

@jamescowens
Copy link
Member Author

When you move the

inline int GetNewbieSnapshotFixHeight()
{
// This was the original hard fork point for the newbie accrual fix that didn't work.
// return fTestNet ? 1393000 : 2104000;
return fTestNet ? 1393000 : 2152000;
}

to a different height, in this case right before the latest superblock, the corrections grow of course, because they are being made later, but the final accruals on the current snapshot still match. See

accrual_mismatches_20210124_after_accrual_rebuild_3.log

and

TallySuperblockAccrual_debug_out_20210124_3.log

So I think this is ready for a initial review.

@jamescowens
Copy link
Member Author

This is the current state of mismatches when the mainnet fix height is set to max (i.e. beyond the current height) so that it is not activated...

accrual_mismatches_20210124_after_accrual_rebuild_fork_height_reset_to_max.log

There are 89 mismatches, of which 88 are last period only accruals (i.e. due to the failure to record an accrual account for a newly activated CPID).

@jamescowens
Copy link
Member Author

I am leaving the commented out code in there for right now for reference purposes. This will be removed post review. The height for the mainnet mandatory will most likely be set in a separate PR depending on the timing of testing and exchange and community notification.

@jamescowens jamescowens marked this pull request as ready for review January 24, 2021 23:24
src/rpc/mining.cpp Show resolved Hide resolved
@jamescowens jamescowens force-pushed the fix_newbie_accrual branch 5 times, most recently from 7c5bd7a to f583eaa Compare January 26, 2021 03:10
This commit implements the accrual correction function necessary to
fix the newbie accrual bug and also implements changes to the auditing
functions. Note that a historical beacon map that is effectively a
linked list is also implemented and the lookback scope of both the
audit and any accrual correction is limited to the chain of beacon
renewals going back to the beacon advertisement from the current active
beacon (if it is a renewal).

It also updates the testnet newbie fix fork point to 1480000 and
mainnet to std::numeric_limits<int>::max() for right now.
…Reward()

This fixes a fracture in consensus that results from the inconsistent state of the original
newbie fix global boolean. See the commentary in the referenced function.
@jamescowens
Copy link
Member Author

jamescowens commented Jan 26, 2021

I discovered almost simultaneously in sync from zero test with this PR (prior to the last push) and being notified on Slack, that some newbie CPID's had managed to get non-truncated accruals staked onto the blockchain after the original newbie height fix at 2104000. This has necessitated further changes to the PR: See the extensive comments in GetNewbieSuperblockAccrualCorrection, reposted here:

This function was moved from the anonymous namespace and private, to public and made static, because it has
to be called from ClaimValidator::CheckResearchReward() directly too. Why?

The broken original newbie fix was CONDITIONAL. The initialization in Tally::Initialize was broken and would
never activate, yet the alternate path through AcceptBlock AddToBlockIndex could actually set the original
global boolean if syncing and passing through the trigger block height (now memorialized in
GetOrigNewbieSnapshotFixHeight()). This led to the following fracture:

Nodes that synced from zero through the original newbie trigger block height would have the global boolean set to
activate the fix. On the other hand as time (height passes) after the trigger height, nodes would get restarted
and are already beyond that height, so their boolean would be set to zero. So if a node that hadn't been restarted
is a newbie and actually stakes with the fix enabled, it will have an accrual account on that node and report
the correct historical accrual to be paid, rather than the newbie truncated value prior to the fix. There are
actually some blocks that made it on the chain that way.

Rather than leave the original broken global boolean flag for the original newbie fix, which doesn't actually work
and would be confusing to code maintainers in the future, I decided to implement this correction function.

The function has two current uses:

  1. It is used in TallySuperblockAccrual() above to compute the "catch-up" correction on the acceptance of a staked
    superblock for any CPIDs that were subject to the newbie bug and have historical accruals that need to be
    included. This is activated at GetNewbieSnapshotFixHeight(), and will cause this function to be used to apply
    the corrections to any CPID's that do not have an accrual account at that height.

  2. It is used in ConnectBlock (the ClaimValidator::CheckResearchReward()) to conditionally apply the accrual
    correction if the claimed value by the block fails the original CheckReward. If the original computed reward
    plus the correction equals the claimed reward, then the block is passed with a warning. This enables any
    node to conditionally validate a block that was staked with the accrual correction active, even if the
    receiving node does not have the NEW correction active.

This function computes the accrual that should have been recorded in the periods between the first superblock that
posted that validated the original verified beacon in a chain of renewals and the current superblock. This uses a
calculation very similar to the calculation in the auditsnapshotaccrual RPC function.

The reason I go through the trouble to limit the lookback scope to the chain of beacon renewals (including the
original advertisement that was validated) is to limit the lookback for this function, and it is proper to limit
the scope of the lookback accrual correction to be done only over the time-frame that is covered by an unbroken
beacon chain.

This function should be very light after the new newbie fix is crossed by the network (after the application of
the accrual corrections for all the CPID's in the first superblock past that height), since the AccrualDelta with
no additional corrections will be necessary (i.e. this function will generate no periods).

@jamescowens
Copy link
Member Author

I am running sync from zero and some other final tests on this on both testnet and mainnet and then will merge into the development branch. Given the situation we need to move on this.

@jamescowens
Copy link
Member Author

The sync from zero on both mainnet and testnet passed. Temporarily resetting the GetNewbieSnapshotFixHeight() for mainnet to 2154000 (before the last superblock), deleting the accrual directory, and restarting then running auditsnapshotaccruals results in 0 mismatches. Merging to the dev branch.

@jamescowens jamescowens merged commit d089323 into gridcoin-community:development Jan 26, 2021
jamescowens added a commit that referenced this pull request Mar 1, 2021
 Added
 - gui: Add RAC column to wizard summary page projects table #1951 (@cyrossignol)
 - rpc: clean up the superblocks function and add magnitude to getmininginfo #1966 (@jamescowens)
 - rpc: Add transaction size to RPC output #1971 (@cyrossignol)
 - voting: Add user-facing support for poll response types #1976 (@cyrossignol)
 - gui: Port Bitcoin Intro class (implement the ability to choose a data directory via the GUI) #1978 (@jamescowens)
 - gui: Port Bitcoin MacOS app nap manager #1991 (@jamescowens)
 - mining, rpc: Implement staking efficiency measure and improve SelectCoinsForStaking and CreateCoinStake #1992 (@jamescowens)
 - accrual, rpc: Implement auditsnapshotaccruals #2001 (@jamescowens)
 - docs: add doxygen support #2000 (@div72)
 - beacon: Specialized beacon storage in leveldb #2009 (@jamescowens)
 - rpc: Add a call to dump contracts in binary form #2011 (@div72)
 - rpc: Add boolean option to report active beacons only in beaconreport #2013 (@jamescowens)
 - consensus: Set Hilda mainnet hardfork height to 2197000 #2022 (@jamescowens)

 Changed
 - refactor: [Memory optimization] Block index duplicate PoS state #1945 (@cyrossignol)
 - refactor: [Memory optimization] Block index superblock and contract flags #1950 (@cyrossignol)
 - refactor: [Memory optimization] Remove stake modifier checksums #1954 (@cyrossignol)
 - refactor: [Memory optimization] Block index allocation overhead #1957 (@cyrossignol)
 - refactor: [Memory optimization] Remove block index subsidy fields #1960 (@cyrossignol)
 - refactor: [Memory optimization] Separate chain trust from the block index #1961 (@cyrossignol)
 - refactor: [Memory optimization] Eliminate padding between block index fields #1962 (@cyrossignol)
 - beacon, gui: Add check for presence of beacon private key to updateBeacon() #1968 (@jamescowens)
 - util: Enhance ETTS calculation #1973 (@jamescowens)
 - refactor: Use new clamp in util.h #1975 (@jamescowens)
 - gui: Redo global status for overview #1983 (@jamescowens)
 - util: Improvements to MilliTimer class and use in the miner and init #1987 (@jamescowens)
 - rpc: Move rpc files to directory #1995 (@Pythonix)
 - rpc: Enhance consolidateunspent and fix fee calculation #1994 (@jamescowens)
 - contract: Double the lookback scope of contract replay #1998 (@jamescowens)
 - net: Don't rely on external IP resolvers #2002 (@Tetrix42)
 - beacon: Change beacon map to pointers #2008 (@jamescowens)
 - gui: Update bitcoin_sv.ts #2014 (@sweede-se)
 - util: Update snapshot URLs and add accrual directory #2019 (@jamescowens)
 - beacon: Tweak BeaconRegistry::Revert #2020 (@jamescowens)
 - rpc, qt: bump fees @2023 (@div72)

 Removed
 - researcher: Remove automatic legacy beacon key import #1963 (@cyrossignol)
 - util: Revert "Close LevelDB after loading the block index" #1969 (@cyrossignol)
 - ci: Fix python symlink issue & remove travis #1990 (@div72)
 - ci: remove python workaround #2005 (@div72)

 Fixed
 - gui: fix mandatory/leisure detection of upgrade check #1959 (@Pythonix)
 - voting: Fix title in "gettransaction" RPC for legacy poll contracts @1970 (@cyrossignol)
 - gui: Fix missing menu items on macOS #1972 (@scribblemaniac)
 - rpc: Fix answer offset in "votedetails" #1974 (@cyrossignol)
 - voting: Implement missing try-catch in VotingVoteDialog::vote #1980 (@jamescowens)
 - scraper: Add check for minimum housekeeping complete in scraper #1977 (@jamescowens)
 - voting: Fix nonsense vote weights for legacy polls #1988 (@cyrossignol)
 - voting: Fix incorrect field returned in ResolveMoneySupplyForPoll() #1989 (@cyrossignol)
 - consensus, accrual: Fix newbie accrual #2004 (@jamescowens)
 - log: grammar correction #2016 (@nathanielcwm)
 - wallet: Correct nMinFee fee calculation in CreateTransaction #2021 (@jamescowens)
 - rpc, miner: Correct GetLastStake #2026 (@jamescowens)
 - wallet: Fix bug in CreateTransaction causing insufficient fees #2029 (@jamescowens)
@jamescowens jamescowens deleted the fix_newbie_accrual branch March 10, 2021 23:50
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.

Pending rewards reset in the morning
3 participants