Staking and mining code cleanup, improvements #301

tomasbrod · 2017-05-14T11:08:44Z

Read the commit messages.
Experimental until declared ready!
Interest and Research subsidy visible in getmininginfo.
Check out -debug=true -printtoconsole;
Block PoS hash and target visible in log;
Refactoring StakeMiner miner.cpp;

denravonska · 2017-05-14T15:51:38Z

Hey, cool. It looks like this one still tries to sign the same block over and over again, is that correct?

denravonska · 2017-05-14T12:42:56Z

src/miner.cpp

@@ -19,7 +19,7 @@ using namespace std;
 // BitcoinMiner
 //

-unsigned int nMinerSleep;
+volatile unsigned int nMinerSleep;


Is this for thread safety? IMO we should change all the current volatiles to std::atomic. At least that's what I think the intention was.

This variable is not changed after stake miner thread is started. Without the volatile modifier optimizer prevents me from changing it with debugger at run-time. It is just for debugger.

denravonska · 2017-05-14T14:09:02Z

src/miner.cpp

        nLastBlockSubmitted = 0;
+        return error("CheckStake: Nonce too low\r\n");
        return false;


Stray return.

denravonska · 2017-05-14T14:10:19Z

src/miner.cpp

-            MilliSleep(nMinerSleep);
+        }
+        SkipStake:
+        MilliSleep(nMinerSleep);


I think we can remove the minersleep flag altogether and just sleep 500ms.

Again. For use with debugger.

denravonska · 2017-05-14T14:10:52Z

src/miner.cpp

        }


        //Verify we are still on the main chain

-        if (IsLockTimeWithinMinutes(nLastBlockSolved,5)) 
+        if (false && IsLockTimeWithinMinutes(nLastBlockSolved,4))


Do we not do this check anymore?

denravonska · 2017-05-14T14:13:38Z

src/rpcmining.cpp

@@ -51,6 +51,10 @@ Value getmininginfo(const Array& params, bool fHelp)
    obj.push_back(Pair("blocks",        (int)nBestHeight));
    obj.push_back(Pair("currentblocksize",(uint64_t)nLastBlockSize));
    obj.push_back(Pair("currentblocktx",(uint64_t)nLastBlockTx));
+    try{
+        std::string superblock_num = mvApplicationCache["superblock;block_number"];


This could be changed to ReadCache("superblock", "block_number");. That implementation also injects a new object if one is missing, but I intend to fix it.

I have no intention to improve this api. Shoud I remove it then?

denravonska · 2017-05-14T15:01:50Z

src/kernel.cpp

@@ -614,42 +614,30 @@ static bool CheckStakeKernelHashV3(CBlockIndex* pindexPrev, unsigned int nBits,
    double coin_age = std::abs((double)nTimeTx-(double)txPrev.nTime);
    double BitsAge = PORDiff * 144;     //For every 100 Diff in Bits, two hours of coin age for researchers
    if (BitsAge > 86400) BitsAge=86400; //Limit to 24 hours (possibility of astronomical diff)
-    // Halford : Explain to the Researcher why they are not staking:
-    if (checking_local && LessVerbose(100))


Did this always evaluate to false?

No. The code is refactored just few lines below.

Need to stop reviewing that early. Read that code 3 times and realised twice that it was refactored :/

denravonska · 2017-05-14T15:05:49Z

src/miner.cpp

@@ -925,14 +918,16 @@ void StakeMiner(CWallet *pwallet)
            else
            {
                msMiningErrors = "Stake block rejected";
-                if (fDebug) printf("Stake block rejected \r\n");
+                printf("Stake block rejected \r\n");
                MilliSleep(1000);
            }
            SetThreadPriority(THREAD_PRIORITY_LOWEST);


I think we should remove these thread priorities. A medium priority across the line should be fine since we're not doing any heavy work.

ok
But i doubt it does anything since only root can ever raise priority of thread on linux.

denravonska · 2017-05-14T15:07:07Z

src/rpcblockchain.cpp

        //12-20-2015 Support for Binary Superblocks
        std::string superblock=UnpackBinarySuperblock(bb.superblock);
        std::string neural_hash = GetQuorumHash(superblock);
        result.push_back(Pair("SuperblockHash", neural_hash));
-        result.push_back(Pair("SuperblockLength", (double)bb.superblock.length()));
+        result.push_back(Pair("SuperblockLength", (long)superblock.length()));


This changes the meaning of "SuperblockLength". Will this cause any compatibility issues?

i'll change that

tomasbrod · 2017-05-14T16:27:11Z

@denravonska performed the changes, please verify

It looks like this one still tries to sign the same block over and over again, is that correct?

Yes that unfortunately still true. Bigger refactoring required to close #295.

denravonska · 2017-05-14T19:43:11Z

src/init.cpp

@@ -558,7 +558,7 @@ bool AppInit2()

    fUseFastIndex = GetBoolArg("-fastindex", false);

-    nMinerSleep = GetArg("-minersleep", 500);
+    nMinerSleep = GetArg("-minersleep", 8000);


I guess this will affect the shutdown time, right? Might be a non-issue though.

Actually, thinking about this again... There's a very specific second where staking is allowed, given the staking mask, right? With the previous 500ms sleep you were pretty much guaranteed to hit that window but with the new sleep there's a risk that you always sleep when the staking window is open. Or did I misinterpret the purpose of the mask?

Not a single second. There is specific period of 16 seconds where staking is allowed. The (bit inverse of) stake mask is applied to timestamp effectively eliminating the difference when of staking anywhere during that 16 seconds. It was introduced to save stakers from having to check every second.

Aaaaah, gotcha!

denravonska · 2017-05-14T19:43:36Z

Excellent, I will try it out first thing tomorrow morning.

denravonska · 2017-05-15T03:15:07Z

Trying this on testnet now and I noticed that it started receiving blocks from the network while still loading the blocks from disk. I'm also seeing a lot of these in the log, but I guess that is to be expected with 0 mag and a semi-broken testnet:

ERROR: CheckStakeKernelHashV3: Magnitude too low for POR reward: 1494818032.000

tomasbrod · 2017-05-15T08:11:34Z

src/kernel.cpp

+        if (payment_age <= BitsAge)
+            narr="Payment age < BitsAge: " + RoundToString(payment_age,3) + "; " + RoundToString(BitsAge,3);
+        if (boincblock.Magnitude <= 1)
+            narr="Magnitude too low for POR reward: " + RoundToString(payment_age,3);


Spam of this message in testnet. Remove warning? Print only when debug enabled? But it is important warning.

I think we can keep it if the user is intended to get rewards. I think testnet doesn't have any projects whitelisted right now.

tomasbrod · 2017-05-16T19:11:47Z

I would like to point you, @Ravon, to this wiki page. I am confused about what outputs is which flag meant to enable.

tomasbrod · 2017-05-17T11:53:04Z

src/kernel.cpp

+        if (coin_age <= RSA_WEIGHT)
+            narr=" Coin Age < RSA_Weight: " + RoundToString(coin_age,0) + "; " + RoundToString(RSA_WEIGHT,0);
+        if (coin_age <= 4*60*60)
+            narr=" Coin Age Immature: " + RoundToString(coin_age,0) + "; 14400";


hehe i lost this condition
that's why my stakes are not accepted on testnet sometimes

tomasbrod · 2017-05-18T09:22:04Z

I will close this request now. This one is only half done and I feel that more refactoring is needed.

denravonska · 2017-05-18T09:23:33Z

Are you sure? It's still an improvement over the current implementation.

tomasbrod · 2017-05-20T12:43:40Z

I misunderstood what a Close really means so I open this again with a push :)
The diff of src/miner.cpp is crazyconfusing. I suggest reading the after file, not the diff.
Remember: this code is experimental and not complete.

tomasbrod · 2017-05-31T21:36:34Z

New status of this PR: @denravonska
New stake code is pretty stable and stakes blocks just right. Less complex code and unnoticeably lower cpu usage.
Some info is not displayed in status. Other info added.
#317 is still an issue: watch out for 1GRC research reward and report there.
Sightly more debug output when enabled.
Only one set of coins (UTXO) is ever added to stake transaction, thus sightly slower interest.
All stake coins and reward is placed in single output: no more 1 cent outputs but may not split outputs as expected.
And due to the PoW bug/feature (#332) minersleep option does affect staking frequency.
I would appreciate some testing.

denravonska · 2017-06-03T20:15:52Z

We should try to include this in the next staging merge if possible. Regarding the minersleep option I'd prefer if we removed it. When debugging it's trivial to alter the code.

I can test it on testnet if you want to.

denravonska · 2017-06-03T20:30:47Z

Running this in testnet now. What's missing from the status?

Getting -ERROR: CheckStakeKernelHashV3: Coin Age < RSA_Weight: 49104; 106280 :(

iFoggz · 2017-06-05T23:58:04Z

Those indents thou in debug print for RSA_WEIGHT ;o) looks nice thou to have that added.

tomasbrod mentioned this pull request May 14, 2017

Add research subsidy to overview page. #302

Closed

denravonska reviewed May 14, 2017

View reviewed changes

tomasbrod force-pushed the stakeclean branch from c3c2489 to c9b0cb1 Compare May 14, 2017 15:56

tomasbrod force-pushed the stakeclean branch 2 times, most recently from 84b6deb to ef0b0ef Compare May 14, 2017 17:38

denravonska reviewed May 14, 2017

View reviewed changes

tomasbrod commented May 15, 2017

View reviewed changes

tomasbrod commented May 17, 2017

View reviewed changes

tomasbrod closed this May 18, 2017

tomasbrod reopened this May 20, 2017

tomasbrod force-pushed the stakeclean branch from 487485b to eb5cef8 Compare May 20, 2017 12:44

tomasbrod closed this May 31, 2017

tomasbrod reopened this May 31, 2017

tomasbrod force-pushed the stakeclean branch from 777154e to 3948628 Compare June 1, 2017 08:55

tomasbrod mentioned this pull request Jun 3, 2017

Spontaneous high CPU usage #349

Closed

tomasbrod mentioned this pull request Jun 3, 2017

RSAWeight and Magnitude not checked in staked blocks #345

Closed

tomasbrod added 2 commits June 9, 2017 15:11

Print beacons as they are loaded and deleted.

61de943

RPC command getblock show unpacked superblock content.

02c8284

tomasbrod added 4 commits June 9, 2017 15:11

Print blocks as they are loaded

a49dad5

Refactor Kernel v3.

6b0b345

Refactor of Stake Miner.

552d08d

main.cpp crlf lf

12240f1

tomasbrod force-pushed the stakeclean branch from b3b644b to d49dcda Compare June 9, 2017 13:34

Merge branch development into stakeclean

bc6bdf7

tomasbrod force-pushed the stakeclean branch from d49dcda to bc6bdf7 Compare June 9, 2017 13:43

denravonska merged commit 5307b1c into gridcoin-community:development Jun 9, 2017

This was referenced Jun 9, 2017

Stake V8, fix PoW and RSA exploits. #364

Merged

Staking creates one cent output #311

Closed

Repetitive calls to SerializeBoincBlock in Miner #295

Closed

denravonska mentioned this pull request Jun 10, 2017

High CPU Usage #365

Closed

This was referenced Jun 14, 2017

ResearchMiner:Net averages not yet loaded error slowing down sync time? #371

Closed

Odds and ends #383

Closed

This was referenced Jul 2, 2017

Fix for the staking up-arrow being gray even when staking. #411

Merged

write it so our grandmas also understand it (what means: "Payment < Diff" in the UI ?) #426

Closed

tomasbrod deleted the stakeclean branch November 10, 2017 17:40

Staking and mining code cleanup, improvements #301

Staking and mining code cleanup, improvements #301

Conversation

tomasbrod commented May 14, 2017 • edited Loading

denravonska commented May 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasbrod May 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasbrod commented May 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denravonska commented May 14, 2017

denravonska commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasbrod commented May 16, 2017

Choose a reason for hiding this comment

tomasbrod commented May 18, 2017 • edited Loading

denravonska commented May 18, 2017

tomasbrod commented May 20, 2017 • edited Loading

tomasbrod commented May 31, 2017 • edited Loading

denravonska commented Jun 3, 2017

denravonska commented Jun 3, 2017

iFoggz commented Jun 5, 2017

tomasbrod commented May 14, 2017 •

edited

Loading

tomasbrod May 14, 2017 •

edited

Loading

tomasbrod commented May 14, 2017 •

edited

Loading

tomasbrod commented May 18, 2017 •

edited

Loading

tomasbrod commented May 20, 2017 •

edited

Loading

tomasbrod commented May 31, 2017 •

edited

Loading