Pause mining during JoinSplit creation #1932

aniemerg · 2016-12-08T02:56:43Z

Closes #1522 .

ebfull · 2016-12-08T21:12:08Z

Awesome!

Is there no way to disable mining via the RPC? (To avoid the scenario where this code would re-enable mining that has been disabled in some other way.)

aniemerg · 2016-12-09T16:35:02Z

So, as best as I understand it, mining is enabled/disabled in the rpc via setgenerate(). If we were to use setgenerate(), we'd have to first check whether mining is enabled with getgenerate(), store that in a flag, disable mining, and then after the joinsplit, re-enable mining if appropriate. I can certainly take that approach (and submit a PR to that effect).

I avoided doing that, because it seemed slightly more complex, but also because this approach avoids changing mapArgs, as it would under the above rpc approach. Since mapArgs appears intended to reflect the parameter state set by the user, I wanted to avoid writing code that would change it. Suppose, for example, the user starts a z_sendmany, and then sends an rpc command to disable mining. Under the RPC approach I described above, it's possible that upon the completion of the joinsplit that mining is re-enabled against the wishes of the user.

str4d

Concept ACK, but at least one necessary change.

str4d · 2017-01-04T08:58:43Z

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -697,6 +702,9 @@ bool AsyncRPCOperation_sendmany::main_impl() {
    }

    sign_send_raw_transaction(obj);
+#ifdef ENABLE_WALLET    
+    GenerateBitcoins(GetBoolArg("-gen",false), pwalletMain, (int)GetArg("-genproclimit", -1));


Should be GetArg("-genproclimit", 1) to match the documented default for -genproclimit (see init.cpp). The defaults in rpcmining.cpp for setgenerate and getmininginfo are inconsistent, and also not actually documented there.

str4d · 2017-01-04T09:00:25Z

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -144,6 +145,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {

    assert(isfromtaddr_ != isfromzaddr_);

+#ifdef ENABLE_WALLET


If #1795 is merged before this PR, change this #ifdef and the one below to:

#if defined(ENABLE_WALLET) && defined(ENABLE_MINING)

str4d · 2017-01-04T09:05:51Z

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -144,6 +145,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {

    assert(isfromtaddr_ != isfromzaddr_);

+#ifdef ENABLE_WALLET
+    GenerateBitcoins(false, NULL, 0);
+#endif


If #1965 is merged before this PR, change this entire #ifdef block to:

#ifdef ENABLE_MINING #ifdef ENABLE_WALLET GenerateBitcoins(false, NULL, 0); #else GenerateBitcoins(false, 0); #endif #endif

and likewise below.

aniemerg · 2017-01-06T03:35:51Z

Should address desired changes. #1795 and #1965 should be merged first.

ebfull · 2017-01-18T00:16:24Z

Thank you @aniemerg! ACK

@zkbot r+

zkbot · 2017-01-18T00:16:24Z

📌 Commit 92e4b29 has been approved by ebfull

ebfull · 2017-01-18T00:17:05Z

@zkbot r-

ebfull · 2017-01-18T00:17:31Z

(Merging #1863 and #1965 first, will require additional review.)

bitcartel

NACK. Thanks for the PR @aniemerg. After review, I see an issue which blocks this PR and two that are concept NACKs (which we can defer)

Bug
Test
Concept: Addresses Pause mining while doing a JoinSplit #1522 but does it solve the problem?
Concept: Design Pattern

Let me expand upon these:

Bug

Mining is halted upon entering the operation's main_impl(). However, mining is only restored at the end of the function when a transaction has been created successfully. There are a bunch of errors that could occur between those two points. If there is an error, mining never restarts. One possible fix for this is to have the stopping and restarting in main(), surrounding the try catch block which calls main_impl().

Test

Perhaps modify one of the Python qa rpc tests to check that the miner threads are disabled when a JoinSplit is in progress?

Concept: Addresses Pause mining while doing a JoinSplit #1522 but does it solve the problem?

Ticket #1522 states "when a JoinSplit starts we should cancel all mining threads". This PR patches the async operation used by z_sendmany, but a user could make an rpc call to 'zcbenchmark createjoinsplit' which does not halt mining.

If there are parallel joinsplits, which we can do by re-enabling the option to have multiple async worker threads, there is a race condition where one joinsplit tx completes successfully and re-enables mining, while another joinsplit tx is still being processed.

Concept: Design Pattern

Why should an async operation care or even know about the miner? We should try to avoid unnecessary coupling between two subsystems. It would be better if the JSDescription class signaled to interested listeners, like the miner, that it was about to generate a JoinSplit and signal again when it has finished (say in JSDescription::Randomized or create a wrapper which we call).

To handle the race condition mentioned in (3), you could use an atomic counter which the halt signal handler will increment and the resume signal handler decrements. At launch state, the counter is 0 which the miner treats as a green light to mine. If there are parallel JoinSplits, the counter will increase but when all the JoinSplits have finished, the counter will be back at 0 and mining can resume.

So next steps...

Please fix (1) and if possible, add a test (2) to confirm that mining threads stop and restart when a JoinSplit is created.

Would be great if you could implement a solution like described in (4). Since that's a bigger chunk of work and other members of the team might have some ideas, that might be best broken off into a new ticket, or added to #1522, and have this PR be a temporary/partial solution to the problem i.e. we leave #1522 open.

@aniemerg Hope the above makes sense, let me know otherwise.

@ebfull Thoughts on above and next steps?

ebfull · 2017-01-19T16:12:19Z

Given the time requirements for 1.0.5 I will need to bump this to 1.0.6.

bitcartel · 2017-02-09T00:27:36Z

Waiting for #1863 and #1965 to be merged first.

bitcartel · 2017-02-09T19:20:00Z

We should have a test for this.

aniemerg · 2017-02-09T21:05:00Z

Regarding a test, I've been unsure how to accomplish that using the rpc testing framework. The way that this PR currently works is to turn off mining in a way that isn't revealed when using rpc commands such as getgenerate. So I am not sure how to write the most natural test -- that is, turn on mining, start a joinsplit, and check to see that mining is off.

daira · 2017-02-11T14:14:23Z

Re @bitcartel's point 3, I don't think it's a problem if zcbenchmark createjoinsplit doesn't pause mining. That RPC is intended to be used with a node (and machine, since it measures wall clock time) that is otherwise completely idle; just pausing mining on an active node would not be sufficient to get an accurate result.

arcalinea · 2017-02-28T02:51:05Z

@aniemerg if we can fix @bitcartel's first point, then we'll merge and open another ticket to address point 4.
If we can fix the first point by mid-week, we'll include this in 1.0.7.

aniemerg · 2017-02-28T03:32:36Z

@arcalinea commit 54c92a6 was intended to fix @bitcartel 's first point. I believe it still needs review.

daira

utACK, but please squash when merging.

arcalinea · 2017-03-02T00:57:54Z

Squashed the commits. Can we get one more review before merging though? (@str4d?)

str4d

utACK, assuming a new ticket is opened for point 4.

arcalinea · 2017-03-02T01:23:05Z

Thanks. Opened ticket #2145 for point 4, merging

@zkbot r+

zkbot · 2017-03-02T01:23:06Z

📌 Commit 49c2cec has been approved by arcalinea

zkbot · 2017-03-02T01:23:11Z

⌛ Testing commit 49c2cec with merge 312fbd9...

…alinea Pause mining during JoinSplit creation Closes #1522 .

zkbot · 2017-03-02T01:23:34Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

zkbot · 2017-03-02T01:23:34Z

📌 Commit 49c2cec has been approved by arcalinea

zkbot · 2017-03-02T02:17:54Z

☀️ Test successful - zcash

str4d added memory management I-performance Problems and improvements with respect to performance A-pow Area: Proof-of-Work and mining labels Dec 16, 2016

str4d changed the title ~~1522 pause mining for joinsplit~~ Pause mining during JoinSplit creation Dec 16, 2016

str4d added this to the 1.0.5 milestone Dec 16, 2016

str4d requested changes Jan 4, 2017

View reviewed changes

str4d approved these changes Jan 10, 2017

View reviewed changes

bitcartel requested review from bitcartel and daira January 16, 2017 22:13

bitcartel suggested changes Jan 18, 2017

View reviewed changes

ebfull modified the milestones: 1.0.6, 1.0.5 Jan 19, 2017

bitcartel added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2017

bitcartel modified the milestones: 1.0.7, 1.0.6 Feb 9, 2017

daira approved these changes Feb 28, 2017

View reviewed changes

Pause mining during joinsplit creation

49c2cec

arcalinea force-pushed the 1522-pause-mining-for-joinsplit branch from 54c92a6 to 49c2cec Compare March 2, 2017 00:56

str4d approved these changes Mar 2, 2017

View reviewed changes

arcalinea mentioned this pull request Mar 2, 2017

Pause mining during joinsplit creation w/out unnecessary subsystem coupling #2145

Closed

zkbot added a commit that referenced this pull request Mar 2, 2017

Auto merge of #1932 - aniemerg:1522-pause-mining-for-joinsplit, r=arc…

312fbd9

…alinea Pause mining during JoinSplit creation Closes #1522 .

zkbot merged commit 49c2cec into zcash:master Mar 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pause mining during JoinSplit creation #1932

Pause mining during JoinSplit creation #1932

aniemerg commented Dec 8, 2016 •

edited by str4d

ebfull commented Dec 8, 2016

aniemerg commented Dec 9, 2016

str4d left a comment

str4d Jan 4, 2017

str4d Jan 4, 2017

str4d Jan 4, 2017

aniemerg commented Jan 6, 2017

ebfull commented Jan 18, 2017

zkbot commented Jan 18, 2017

ebfull commented Jan 18, 2017

ebfull commented Jan 18, 2017 •

edited

bitcartel left a comment •

edited

ebfull commented Jan 19, 2017

bitcartel commented Feb 9, 2017

bitcartel commented Feb 9, 2017

aniemerg commented Feb 9, 2017

daira commented Feb 11, 2017

arcalinea commented Feb 28, 2017

aniemerg commented Feb 28, 2017

daira left a comment •

edited

arcalinea commented Mar 2, 2017 •

edited

str4d left a comment

arcalinea commented Mar 2, 2017 •

edited

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

		@@ -144,6 +145,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {

		assert(isfromtaddr_ != isfromzaddr_);

		#ifdef ENABLE_WALLET

Pause mining during JoinSplit creation #1932

Pause mining during JoinSplit creation #1932

Conversation

aniemerg commented Dec 8, 2016 • edited by str4d

ebfull commented Dec 8, 2016

aniemerg commented Dec 9, 2016

str4d left a comment

Choose a reason for hiding this comment

str4d Jan 4, 2017

Choose a reason for hiding this comment

str4d Jan 4, 2017

Choose a reason for hiding this comment

str4d Jan 4, 2017

Choose a reason for hiding this comment

aniemerg commented Jan 6, 2017

ebfull commented Jan 18, 2017

zkbot commented Jan 18, 2017

ebfull commented Jan 18, 2017

ebfull commented Jan 18, 2017 • edited

bitcartel left a comment • edited

Choose a reason for hiding this comment

ebfull commented Jan 19, 2017

bitcartel commented Feb 9, 2017

bitcartel commented Feb 9, 2017

aniemerg commented Feb 9, 2017

daira commented Feb 11, 2017

arcalinea commented Feb 28, 2017

aniemerg commented Feb 28, 2017

daira left a comment • edited

Choose a reason for hiding this comment

arcalinea commented Mar 2, 2017 • edited

str4d left a comment

Choose a reason for hiding this comment

arcalinea commented Mar 2, 2017 • edited

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

zkbot commented Mar 2, 2017

aniemerg commented Dec 8, 2016 •

edited by str4d

ebfull commented Jan 18, 2017 •

edited

bitcartel left a comment •

edited

daira left a comment •

edited

arcalinea commented Mar 2, 2017 •

edited

arcalinea commented Mar 2, 2017 •

edited