Skip to content

Conversation

@random-zebra
Copy link

@random-zebra random-zebra commented Jan 8, 2020

This PR cleans up the miner code and streamlines the logic used by the client (both in the RPC and in the GUI) to determine whether the wallet is staking or not.
Currently it relies on a collection of variables (mapHashedBlocks, nLastCoinStakeInterval, nSearchInterval, etc...) updated in kernel.
These are here replaced with a new class CStakerStatus, updated in CreateCoinStake (thus directly in wallet), and an instance of CStakerStatus is introduced as member of CWallet.
This class keeps two variables, timeLastStakeAttempt and tipLastStakeAttempt, containing respectively the time of last stake attempt and a pointer to the index of the block upon which last stake attempt was made.

The Staking Status is Active whenever timeLastStakeAttempt is less than
30 seconds in the past.

This PR also expands the output of getstakingstatus rpc call adding:

  • staking_enabled to tell whether or not staking has been disabled via conf file / startup flag
  • hashLastStakeAttempt and timeLastStakeAttempt values
  • heightLastStakeAttempt (the height of the block with hash hashLastStakeAttempt)
  • tiptime (the time of the chaintip block) to compare with timeLastStakeAttempt. This field replaces valid_time which is unnecessary now).

and fixes enoughcoins with proper staking balance.

@random-zebra random-zebra added Bug Wallet Block Generation Mining/Staking related issues labels Jan 8, 2020
@random-zebra random-zebra self-assigned this Jan 8, 2020
@random-zebra random-zebra changed the title [WIP][Core] Fix inconsistent staking status reported via rpc/cli and add info to getstakingstatus [WIP][Core] Fix inconsistent staking status reported via rpc/gui + add info to getstakingstatus Jan 8, 2020
@random-zebra random-zebra force-pushed the 2020_stakingstatus branch 2 times, most recently from e7ae048 to 54bb3f4 Compare January 8, 2020 17:52
Streamline the logic to update the wallet's staking status using two
single variables timeLastStakeAttempt and hashLastStakeAttempt.
The Staking Status is Active whenever timeLastStakeAttempt is less than
30 seconds in the past.

The logic is implemented with a new class CStakerStatus, member of the
wallet. Time and hash are now updated directly in the wallet
(CreateCoinStake) instead of in the kernel (Stake)
remove:
- nLastCoinStakeSearchTime (and nSearchTime/fStakeFound + if block)
- fLastLoopOrphan
@random-zebra random-zebra changed the title [WIP][Core] Fix inconsistent staking status reported via rpc/gui + add info to getstakingstatus [Core] Fix inconsistent staking status reported via rpc/gui + add info to getstakingstatus Jan 8, 2020
@random-zebra random-zebra added this to the 4.0.1 milestone Jan 8, 2020
Directly update the staking status icon when the wallet is manually
locked so the user doesn't have to wait for the polling timeout and gets
instant feedback.

Otherwise the wallet would have (even if just for 15-30 seconds)
simultaneously the staking icon ON and the lock icon LOCKED, which could
be quite confusing.
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.

Overall looking quite good, few minor things

@furszy
Copy link

furszy commented Jan 9, 2020

Bug found.
Screen Shot 2020-01-09 at 3 49 15 PM

Locked wallet, staking "active" when it's not (due the timer polling and the 30 secs "active" window).

The setStakingStatusActive(false) method call in the topbar lock action is causing it.
Change the updateStakingStatus to this:

void TopBar::updateStakingStatus(){
    setStakingStatusActive(walletModel && (walletModel->getEncryptionStatus() != WalletModel::Locked) && walletModel->isStakingStatusActive());
}

Or create a IsWalletLocked() method in the walletModel object with the same line of code and put it there.

- remove extra "Update" function and introduce "SetNull"
- guard initialization in CWallet::SetNull()
- GUI: fix staking icon update when wallet locked
- update CStakerStatus time *after* Stake()
- RPC: move staking_status to the top
@random-zebra
Copy link
Author

@furszy 's points tackled

The call is already wrapped in it, no need to check a second time.
Further, since the status is get from pwalletMain, throw if it is not
loaded.

Also move the staking status inside if(pwalletMain) in getinfo
@random-zebra random-zebra changed the title [Core] Fix inconsistent staking status reported via rpc/gui + add info to getstakingstatus [Core] Rework staking status Jan 10, 2020
@furszy
Copy link

furszy commented Jan 10, 2020

I'm not good with the latest commit, if the wallet was created (which must happen before start the miner thread and we must ensure it) the stakingStatus pointer will always have a value as SetNull is called in the constructor.

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.

Pretty nice cleanup 👌 , ACK 3893739

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 3893739

random-zebra added a commit that referenced this pull request Jan 11, 2020
3893739 [RPC] Fix getstakingstatus removing compile-time conditional (random-zebra)
9362e88 [Wallet][Cleanup][GUI] minor updates to staking status (random-zebra)
968d861 [Wallet] CStakerStatus: save a pointer to the tip instead of the hash (random-zebra)
bb2a987 [GUI] Refactor updateStakingStatus and set it to inactive after locking (random-zebra)
d2aebc5 [Trivial] Fix lastHashTime type in miner (random-zebra)
b8ed76f [RPC] Add CStakerStatus data to getstakingstatus (random-zebra)
d2d5f08 [PoS] Lock cs_main when getting chainActive data in miner (random-zebra)
dbc46d8 [Cleanup] Remove unused variables in miner (random-zebra)
01173e9 [Core][PoS] Replace mapHashedBlocks and nLastCoinStakeSearchInterval (random-zebra)
6b2b813 [Cleanup][Wallet] Remove unused nSearchInterval field in CreateCoinStake (random-zebra)

Pull request description:

  This PR cleans up the miner code and streamlines the logic used by the client (both in the RPC and in the GUI) to determine whether the wallet is staking or not.
  Currently it relies on a collection of variables (`mapHashedBlocks`, `nLastCoinStakeInterval`, `nSearchInterval`, etc...) updated in kernel.
  These are here replaced with a new class `CStakerStatus`, updated in `CreateCoinStake` (thus directly in wallet), and an instance of  `CStakerStatus` is introduced as member of CWallet.
  This class keeps two variables, `timeLastStakeAttempt ` and `tipLastStakeAttempt`, containing respectively the time of last stake attempt and a pointer to the index of the block upon which last stake attempt was made.

  The Staking Status is Active whenever timeLastStakeAttempt is less than
  30 seconds in the past.

  This PR also expands the output of `getstakingstatus` rpc call adding:

  - `staking_enabled` to tell whether or not staking has been disabled via conf file / startup flag
  - `hashLastStakeAttempt` and `timeLastStakeAttempt` values
  - `heightLastStakeAttempt` (the height of the block with hash `hashLastStakeAttempt`)
  - `tiptime` (the time of the chaintip block) to compare with `timeLastStakeAttempt`. This field replaces  `valid_time` which is unnecessary now).

  and fixes `enoughcoins` with proper staking balance.

ACKs for top commit:
  furszy:
    Pretty nice cleanup 👌 , ACK 3893739
  Fuzzbawls:
    ACK 3893739

Tree-SHA512: f145650f59d2799641e64359c2656f0fd47397ab88893f4c179fa47b57ff5f166929b2af35741124a8a937431b072392f36b4ce50cce564a55df52a78aa9af0e
@random-zebra random-zebra merged commit 3893739 into PIVX-Project:master Jan 11, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Streamline the logic to update the wallet's staking status using two
single variables timeLastStakeAttempt and hashLastStakeAttempt.
The Staking Status is Active whenever timeLastStakeAttempt is less than
30 seconds in the past.

The logic is implemented with a new class CStakerStatus, member of the
wallet. Time and hash are now updated directly in the wallet
(CreateCoinStake) instead of in the kernel (Stake)
Github-Pull: PIVX-Project#1245
Rebased-From: 01173e9
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
remove:
- nLastCoinStakeSearchTime (and nSearchTime/fStakeFound + if block)
- fLastLoopOrphan
Github-Pull: PIVX-Project#1245
Rebased-From: dbc46d8
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Directly update the staking status icon when the wallet is manually
locked so the user doesn't have to wait for the polling timeout and gets
instant feedback.

Otherwise the wallet would have (even if just for 15-30 seconds)
simultaneously the staking icon ON and the lock icon LOCKED, which could
be quite confusing.
Github-Pull: PIVX-Project#1245
Rebased-From: bb2a987
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
- remove extra "Update" function and introduce "SetNull"
- guard initialization in CWallet::SetNull()
- GUI: fix staking icon update when wallet locked
- update CStakerStatus time *after* Stake()
- RPC: move staking_status to the top
Github-Pull: PIVX-Project#1245
Rebased-From: 9362e88
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jan 11, 2020
The call is already wrapped in it, no need to check a second time.
Further, since the status is get from pwalletMain, throw if it is not
loaded.

Also move the staking status inside if(pwalletMain) in getinfo
Github-Pull: PIVX-Project#1245
Rebased-From: 3893739
Fuzzbawls added a commit that referenced this pull request Jan 12, 2020
d209897 [RPC] Notate all account stuff as deprecated (Fuzzbawls)
10bbf9e [Qt] Initialize isLoading to false for CS view (Fuzzbawls)
b7c63b8 [Trivial] Set log2_work decimals to 16 in the logs Github-Pull: #1252 Rebased-From: d83e67c (random-zebra)
a80e2e9 [RPC] Fix getstakingstatus removing compile-time conditional (random-zebra)
876d393 [Wallet][Cleanup][GUI] minor updates to staking status (random-zebra)
f9f437a [Wallet] CStakerStatus: save a pointer to the tip instead of the hash Github-Pull: #1245 Rebased-From: 968d861 (random-zebra)
f9022cf [GUI] Refactor updateStakingStatus and set it to inactive after locking (random-zebra)
cd6f6cb [Trivial] Fix lastHashTime type in miner Github-Pull: #1245 Rebased-From: d2aebc5 (random-zebra)
6845d18 [RPC] Add CStakerStatus data to getstakingstatus Github-Pull: #1245 Rebased-From: b8ed76f (random-zebra)
b63d963 [PoS] Lock cs_main when getting chainActive data in miner Github-Pull: #1245 Rebased-From: d2d5f08 (random-zebra)
6fac9b9 [Cleanup] Remove unused variables in miner (random-zebra)
7128945 [Core][PoS] Replace mapHashedBlocks and nLastCoinStakeSearchInterval (random-zebra)
c546ac1 [Cleanup][Wallet] Remove unused nSearchInterval field in CreateCoinStake Github-Pull: #1245 Rebased-From: 6b2b813 (random-zebra)
5dbda15 [GUI] Every masternode action checking for tier two network synced. (furszy)
499dac8 [Trivial][RPC] Fix example line in importprivkey help (missing arg) Github-Pull: #1242 Rebased-From: ce93872 (random-zebra)
4133401 [Tests] Add wallet_import_stakingaddress to test runner (random-zebra)
8ac5cbb [Tests] Add functional test for import staking address/key Github-Pull: #1242 Rebased-From: d1ebf2e (random-zebra)
bb9426a [RPC] Add coldstaking address support in importaddress Github-Pull: #1242 Rebased-From: 3bd5579 (random-zebra)
3084143 [RPC] Add coldstaking address support in importprivkey Github-Pull: #1242 Rebased-From: 144ec35 (random-zebra)
bb021af [Model][Wallet][Performance] TxRecord updateStatus not accepted stake status fix + performance improvements. (furszy)
1f0823f [GUI][Model] Start missing masternodes flow implemented. (furszy)
90639d1 [GUI] Masternodes start all flow implemented. (furszy)
14f3220 [Core] Don't log missing MNs during CBudgetProposal::CleanAndRemove (random-zebra)
7e1b908 [GUI] Translator class abstracted to be able to reuse the ProcessSendCoinsReturn method in the masternodeswizard class. (furszy)
fa73ae3 [GUI] Masternode creation wizard, creation fail event properly detailing the failure cause in the subsequent error dialog. (furszy)
447265f [Wallet] Graceful shutdown in the unlock corrupted wallet,  showing the proper error message in screen. (furszy)
81cbe1d [Cleanup][Tests] Remove precompute option in default framework node conf Github-Pull: #1237 Rebased-From: 51cbb00 (random-zebra)
ded2cc2 [Cleanup] remove fPrecompute option in SelectStakeCoins (random-zebra)
391c9e6 [Cleanup][Tests] Remove crazy useful unittest created by "Tom" Github-Pull: #1237 Rebased-From: 0187f0d (random-zebra)
67411b5 [Cleanup][Wallet] Remove zpiv spend cache from zpiv tracker Github-Pull: #1237 Rebased-From: 29a934c (random-zebra)
7084840 [Cleanup][DB] Remove DB functions for zerocoin precomputing Github-Pull: #1237 Rebased-From: bd3eebb (random-zebra)
1e93feb [Cleanup] Remove zPIV precomputing global variables. Github-Pull: #1237 Rebased-From: 837f4e2 (random-zebra)
4de0dec [Cleanup][Qt] remove COLUMN_PRECOMPUTE from zpivcontroldialog Github-Pull: #1237 Rebased-From: 520284e (random-zebra)
3394078 [Cleanup] Init: remove precompute-related helps in strUsage Github-Pull: #1237 Rebased-From: 2585532 (random-zebra)
7d03510 [Cleanup] Init: remove "precompute" debug category (not used anywhere) Github-Pull: #1237 Rebased-From: fbcf37b (random-zebra)
9ea6aa2 [Tests][BUG] Fix RPC_TRANSACTION_REJECTED in mining_pos_reorg Github-Pull: #1218 Rebased-From: 1fb9dd3 (random-zebra)
44c60dc [Tests] sort exported mints by denom in zc_spends and zc_wrapped_serials (random-zebra)
0538d25 [Tests] Speed up cache generation (random-zebra)
3abe8a8 [Travis][Tests] Add travis ping during create_cache Github-Pull: #1218 Rebased-From: 6a2a3b9 (random-zebra)
e6080f4 [RPC][BUG] Remove extra lock in spendrawzerocoin Github-Pull: #1218 Rebased-From: 35ea5bc (random-zebra)
88ddf09 [Tests][Trivial] remove stale zerocoin_publicSpend_reorg (random-zebra)
5c70582 [Tests] Fix and re-enable wrapping serials test Github-Pull: #1218 Rebased-From: 039c220 (random-zebra)
fc7069a [RPC] Enable v2 spending on regtest with spendrawzerocoin Github-Pull: #1218 Rebased-From: ddf18d9 (random-zebra)
5a3951a [Tests] Sort test_runner list by execution time (and comment it there) Github-Pull: #1218 Rebased-From: b3af5bd (random-zebra)
248adc5 [Tests] Fix edge case for double spends in zerocoin_spends.py (random-zebra)
66f3cb1 [Tests] Remove zpiv tests from fakestake (random-zebra)
11bc853 [Trivial] Rename base test class in p2p_time_offset functional test Github-Pull: #1218 Rebased-From: cb54381 (random-zebra)
8a25781 [Tests] Refactor POS reorg test (random-zebra)
ddc6b8a [Tests] Prefix "_pos_" tests with "_mining_" Github-Pull: #1218 Rebased-From: 384c636 (random-zebra)
97da652 [Tests] Fix wallet_reorg-stake functional test Github-Pull: #1218 Rebased-From: 8949e38 (random-zebra)
807b4ab [Consensus] Fix fake-stake spent serials detection on forked chains Github-Pull: #1218 Rebased-From: 0417f66 (random-zebra)
24bee71 [Tests] Unify and complete FakeStake test suite in a single testcase. Github-Pull: #1218 Rebased-From: d2c1ef2 (random-zebra)
0dc9e17 [Tests] Refactor PIVX tools in test_framework, blocktools and utils Github-Pull: #1218 Rebased-From: b3f0c59 (random-zebra)
07a7081 [RPC][Tests] createrawzerocoinpublicspend --> createrawzerocoinspend (random-zebra)
ccd5b80 [Tests] Refactor zerocoin_valid_public_spend into zerocoin_spends (random-zebra)
0b551a9 [Tests] Add zerocoins to PoS cache (random-zebra)
ae841ed [RPC] use json object as output of mintzerocoin (random-zebra)
5539b01 [Tests] Restore p2p_zpos_fakestake (random-zebra)
8f84377 [Tests] Lower zerocoin confirmations on regtest (random-zebra)
f36a035 [Tests] Fix zerocoin_valid_public_spend with new v3 activation height Github-Pull: #1218 Rebased-From: acf8fea (random-zebra)
d7804b7 [Tests] Set public spend activation for regtest to block 400 (from 350) (random-zebra)
e8b7ce3 [RPC] Refactor spendzerocoin/spendzcoinmints, fix createrawzerocoinstake (random-zebra)
9134f40 [Tests] Introduce PIVX specific blocktools (random-zebra)
56cfbdd [Tests] Cleanup rpc_spork functional test with new util functions Github-Pull: #1218 Rebased-From: 68ce4e9 (random-zebra)
e5e6f28 [Tests] Define spork util functions (random-zebra)
ac6f95e [Tests] Use PoS_cache in zerocoin_public_spend Github-Pull: #1218 Rebased-From: 383ab89 (random-zebra)
e093f7b [RPC] Add mint txid to spendrawzerocoin (and look for it if empty) Github-Pull: #1218 Rebased-From: 422f003 (random-zebra)
8ebd65a [Trivial] Detailed debug log for received spork messages Github-Pull: #1218 Rebased-From: afabe8c (random-zebra)
f669844 [Tests] Define a new 'PoS' cache with 330-blocks chain Github-Pull: #1218 Rebased-From: 80306e1 (random-zebra)
3c84efd [Tests] Fix CheckBlockHeader version after block 300 for regtest Github-Pull: #1218 Rebased-From: af05235 (random-zebra)
3b8de3a [Tests] Have combine_logs default to most recent test dir (random-zebra)
baa97af [Tests] rename base class to PivxTestFramework Github-Pull: #1218 Rebased-From: 8f668b6 (random-zebra)
ab553bf [Tests] Debug assert_raises_rpc_error Github-Pull: #1218 Rebased-From: 261bb6a (random-zebra)
76531e2 [Cleanup] Replace addr purpose strings with AddressBookPurpose constants Github-Pull: #1238 Rebased-From: 768694f (random-zebra)
09120ce [RPC] Fix Base58Type in ListaddressesForPurpose (random-zebra)
7e32b1e [RPC] Add optional bool arg to listdelegators to show blacklisted addrs Github-Pull: #1238 Rebased-From: 34e871c (random-zebra)
8f01bce [RPC] Add optional str argument to delegatoradd for address label Github-Pull: #1238 Rebased-From: 21f1b9d (random-zebra)
bb1c8f2 [Docs] Update build-unix.md (Fuzzbawls)
f035a4a [Cleanup] Remove precomputing (Fuzzbawls)
ee4aed3 [Trivial] Adding load txs on demand todo text. (furszy)
784094b [Startup] Decrease the amount of blocks checked for corruption in the startup. (furszy)
3b67bc6 [GUI][Performance] Optimizations to every list view load in the GUI. (furszy)
7da7eb7 [Model] Maximum amount of loaded record in ram, preventing any self-spamming. (furszy)
aeaf418 [GUI] Adding the capability to decrease the screen size for smaller screens support. (furszy)
2a5d14d [Wallet] IsEquivalentTo commented for now to not add an extra round of serialization + hash calculation in every read transaction in the wallet startup. (furszy)
f7a1420 Minor changes within 4.0 wallet FAQ Github-Pull: #1224 Rebased-From: 43b783f (NoobieDev12)
053d365 [Trivial] Remove spammy log in in StakeV1 Github-Pull: #1233 Rebased-From: f9857f7 (random-zebra)
16441e9 [GUI][Trivial] Minor edits to written content Github-Pull: #1184 Rebased-From: dae64f3 (random-zebra)
ffac2a7 [BUG][RPC] fix signature check (against old format) in mnbudgetrawvote Github-Pull: #1206 Rebased-From: ae063d0 (random-zebra)
e82e2f5 [GUI][Bug] Show locked balance in the available total amount and notify the user about its existence. (furszy)
47d2c40 [Backport][Performance] Cache best block hash for miner thread usage + refactor. (furszy)
fe12794 [Wallet] Remove un-necessary CheckTransaction call when loading wallet. (Fuzzbawls)
b8f2b1a [Build] Clean up warnings (Cave Spectre)
d4e6693 [Cleanup] Remove unnecessary QtCreator files (Fuzzbawls)
361757b [Build] Bump snapcraft nightlies to 4.0.99 (Fuzzbawls)

Pull request description:

  Updates the `4.0` branch with relevant merged PRs in preparation for tagging the `4.0.1` release.

  Included PRs:
  #1204
  #1205
  #1199
  #1222
  #1203
  #1223
  #1206
  #1184
  #1233
  #1224
  #1231
  #1228
  #1217
  #1234
  #1207
  #1238
  #1218
  #1237
  #1229
  #1211
  #1243
  #1221
  #1240
  #1242
  #1250
  #1245
  #1252
  #1253
  #1251

ACKs for top commit:
  random-zebra:
    utACK d209897

Tree-SHA512: 092349a93ea4bfe6d6284d5b996055d2900576fab971707458260a848fbdbb15926b13e6ee46469b283721f7dc51a455f5d209a62d9309caa03cecd3b66bd556
furszy added a commit that referenced this pull request Jan 21, 2020
7a8d755 [Tests] Remove '-staking' from extra-args (random-zebra)
72d5a31 [Tests] Fix/Update mining_pos_coldstaking test (random-zebra)
5ffa6b5 [Trivial] Rename mintablecoins --> stakeablecoins (random-zebra)
e781cd0 [Cleanup] zPIV Don't validate Accumulator Checkpoints anymore (random-zebra)
5b192e0 [Tests][Bug] Fix staking status in generate_pos() (random-zebra)

Pull request description:

  This is based on top of #1276 and #1277 which should be reviewed before.
  `generate_pos` framework function relies on the output of `getstakingstatus` RPC which was recently changed.
  Since #1245, "validtime" is not returned anymore (nor it was ever really needed here).
  Only things that we need in `generate_pos` are "walletunlocked" and "mintablecoins" (renamed "stakeablecoins").
  "enoughcoins" is going to be removed in #1277 , and "haveconnections" is not required on regtest.
  This also updates coldstaking test.

ACKs for top commit:
  Fuzzbawls:
    ACK 7a8d755
  furszy:
    ACK 7a8d755

Tree-SHA512: 0f45d8b3880d8d88258861fc85c04e0c62a834cede8991e1370afcaef05a2fb714a4441e201d0bdfd89251628e92aa51496182b8088917a8f3a9b740a21272f6
@random-zebra random-zebra deleted the 2020_stakingstatus branch September 24, 2020 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Block Generation Mining/Staking related issues Bug RPC Wallet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants