Detach wallet from miner #3762

str4d · 2019-01-04T00:02:31Z

Cherry-picked from upstream PR bitcoin/bitcoin#5994.

Part of #2074.

str4d · 2019-01-04T00:06:08Z

Force-pushed because the no-op bracket change made by @LarryRuane in #3647 (which merged today) conflicted with this PR, and I hadn't updated my local master yet 😅

Eirik0

Looks good overall - just a couple of comments. One thing that I wondered while going through this was if it would have been easier to first revert 8e8b6d7 then do the cherry-picking and then reapply the things we needed from that commit separately - not sure about this, just a thought I had.

Eirik0 · 2019-01-09T20:55:17Z

src/init.cpp

This is not part of the commit this is cherry-picked from, but it appears to be in code that is unique to us. Do you mind briefly explaining what this is doing?

This is leveraging the fact that boost::signals2 executes connected handlers in-order. Further up, the wallet is connected to this signal if the wallet is enabled. The wallet handler was modified from upstream so that it does nothing if -mineraddress is set, and GetScriptForMinerAddress() does nothing if -mineraddress is not set (or set to an invalid address). The upshot is:

If the wallet is enabled and -mineraddress is not set, the passed-in CScript is set to a wallet address.

If the wallet is enabled and -mineraddress is set, the passed-in CScript is set to -mineraddress.

If the wallet is disabled and -mineraddress is set, the passed-in CScript is set to -mineraddress.

If the wallet is disabled and -mineraddress is not set, the passed-in CScript is not modified; in practice this means it is empty, and GenerateBitcoins() returns an error.

Eirik0 · 2019-01-09T21:08:34Z

src/miner.h

The commit this is cherry-picked from does not remove class CWallet. I guess that this would cause a compilation error if it were an actual problem.

Eirik0 · 2019-01-09T21:34:02Z

src/miner.cpp

This could probably use a comment.

Eirik0 · 2019-01-09T21:46:05Z

qa/rpc-tests/test_framework/util.py

I think it is a little confusing to have to pass in the expected_utxos to get the correct coinbase address. How did this work before when we just called getnewaddress() would it give us the either the oldest or the most recent address. Would it be possible to default to that if expected_utxos is not provided or do that instead.

In a pairing, @Eirik0 and I decided that we would remove the expected_utxos argument, and just return the address that contains the most UTXOs. For all the RPC tests that use this function, this is sufficient (because either they call once targeting a large-UTXO address, or they call every time they need a single UTXO).

I instead made it optional, because one of the RPC tests does rely on selecting specific coinbase addresses.

str4d · 2019-01-28T18:47:32Z

Addressed @Eirik0's comments.

Eirik0

utACK

Eirik0 · 2019-01-30T03:26:59Z

@zkbot try

zkbot · 2019-01-30T03:27:07Z

⌛ Trying commit bcb3d6ce39454c0e42ee9d60dc0121e4105df538 with merge e8d8897c620ad473bf85fc9ed1ad8e81dfee1a57...

zkbot · 2019-01-30T05:50:44Z

☀️ Test successful - pr-try
State: approved= try=True

mdr0id · 2019-02-20T19:34:02Z

@str4d I am seeing this fail to build with NO_WALLET=1. Can you please confirm what is used to build this?

mdr0id

Seeing the zcash-gtest binary fail using CONFIGURE_FLAGS="--enable-wallet=no" ' in build.sh`

- use one CReserveScript per mining thread

After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after startup does not return the address being used by the miner.

str4d · 2019-03-05T20:29:33Z

Rebased on master to fix the merge conflict. Addressing the zcash-gtest compilation issue now...

This ensures it is accessible by the test suite when the wallet is disabled.

The code was already compiled as part of the wallet, but the tests were not, meaning that the tests would fail to compile when the wallet was disabled.

str4d · 2019-03-05T21:05:23Z

Fixed the compilation issues by moving code into the correct compilation units.

mdr0id · 2019-03-06T05:15:25Z

Tested ACK per last commit to resolve binary issue with zcash-gtest

Eirik0

reACK

Eirik0 · 2019-03-12T15:39:55Z

@zkbot r+

zkbot · 2019-03-12T15:40:14Z

📌 Commit 1fee150 has been approved by Eirik0

zkbot · 2019-03-12T15:40:40Z

⌛ Testing commit 1fee150 with merge 224635bba7e380948f9a6d7f952eb6118c8e8dad...

zkbot · 2019-03-12T19:58:37Z

💔 Test failed - pr-merge

Eirik0 · 2019-03-12T20:01:23Z

The failures do not seem to be related to the PR, for example, CI got stuck waiting forever of a macOS build which was not happening.

Eirik0 · 2019-03-12T20:01:29Z

@zkbot retry

zkbot · 2019-03-12T20:01:35Z

⌛ Testing commit 1fee150 with merge 1cbe507...

Detach wallet from miner Cherry-picked from upstream PR bitcoin/bitcoin#5994. Part of #2074.

zkbot · 2019-03-12T21:46:41Z

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing 1cbe507 to master...

str4d added A-wallet Area: Wallet C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. A-pow Area: Proof-of-Work and mining labels Jan 4, 2019

str4d force-pushed the 2074-detach-wallet-from-miner branch from 884a21b to f1fa2a4 Compare January 4, 2019 00:04

str4d mentioned this pull request Jan 4, 2019

Bitcoin Core refactoring and cleanups 1 #2912

Merged

Eirik0 reviewed Jan 9, 2019

View reviewed changes

mms710 requested review from daira and mdr0id January 29, 2019 18:09

Eirik0 approved these changes Jan 30, 2019

View reviewed changes

mdr0id suggested changes Mar 4, 2019

View reviewed changes

jonasschnelli and others added 7 commits March 6, 2019 09:02

detach wallet from miner

b2993bc

fix GetScriptForMining() CReserveKey::keepKey() issue

38da0d1

add CReserveScript to allow modular script keeping/returning

f4055fe

- use one CReserveScript per mining thread

miner: rename UpdateRequestCount signal to ResetRequestCount

648d6be

test: Fetch coinbase address from coinbase UTXOs

b0f1d64

After upstream PR bitcoin/bitcoin#5994, the first call to getnewaddress after startup does not return the address being used by the miner.

test: Make expected_utxos optional in get_coinbase_address()

3cb20f2

Add comments

c233f6f

str4d force-pushed the 2074-detach-wallet-from-miner branch from bcb3d6c to c233f6f Compare March 5, 2019 20:29

str4d added 2 commits March 6, 2019 09:41

Move utiltest.cpp from wallet to common

c7fc789

This ensures it is accessible by the test suite when the wallet is disabled.

Move payment disclosure code and tests into wallet

1fee150

The code was already compiled as part of the wallet, but the tests were not, meaning that the tests would fail to compile when the wallet was disabled.

mdr0id self-requested a review March 6, 2019 05:14

mdr0id requested a review from Eirik0 March 6, 2019 05:14

mdr0id approved these changes Mar 6, 2019

View reviewed changes

Eirik0 approved these changes Mar 12, 2019

View reviewed changes

zkbot added a commit that referenced this pull request Mar 12, 2019

Auto merge of #3762 - str4d:2074-detach-wallet-from-miner, r=Eirik0

1cbe507

Detach wallet from miner Cherry-picked from upstream PR bitcoin/bitcoin#5994. Part of #2074.

zkbot merged commit 1fee150 into zcash:master Mar 12, 2019

zkbot mentioned this pull request Mar 12, 2019

3553 listnotes rpc test #3569

Draft

str4d mentioned this pull request Mar 12, 2019

Bitcoin Core 0.12.0 #2074

Open

str4d deleted the 2074-detach-wallet-from-miner branch March 12, 2019 22:32

str4d mentioned this pull request Mar 14, 2019

Shield immediately in coinbase #2488

Closed

str4d added this to the v2.0.4 milestone Mar 19, 2019

This was referenced May 19, 2019

Detach wallet from miner RunOnFlux/fluxd#38

Merged

Upstream zcash v2.0.4 RunOnFlux/fluxd#39

Merged

V2.0.4 Upstream and merge RunOnFlux/fluxd#40

Merged

V2.0.4 upstream and merge RunOnFlux/fluxd#41

Merged

str4d mentioned this pull request Jul 22, 2019

add addressindex RPC for bitcore block explorer #3926

Merged

Detach wallet from miner #3762

Detach wallet from miner #3762

Uh oh!

Conversation

str4d commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

str4d commented Jan 4, 2019

Uh oh!

Eirik0 left a comment

Choose a reason for hiding this comment

Uh oh!

Eirik0 Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

str4d Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

Eirik0 Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

Eirik0 Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

Eirik0 Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

str4d Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

str4d Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

str4d commented Jan 28, 2019

Uh oh!

Eirik0 left a comment

Choose a reason for hiding this comment

Uh oh!

Eirik0 commented Jan 30, 2019

Uh oh!

zkbot commented Jan 30, 2019

Uh oh!

zkbot commented Jan 30, 2019

Uh oh!

mdr0id commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdr0id left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d commented Mar 5, 2019

Uh oh!

str4d commented Mar 5, 2019

Uh oh!

mdr0id commented Mar 6, 2019

Uh oh!

Eirik0 left a comment

Choose a reason for hiding this comment

Uh oh!

Eirik0 commented Mar 12, 2019

Uh oh!

zkbot commented Mar 12, 2019

Uh oh!

zkbot commented Mar 12, 2019

Uh oh!

zkbot commented Mar 12, 2019

Uh oh!

Eirik0 commented Mar 12, 2019

Uh oh!

Eirik0 commented Mar 12, 2019

Uh oh!

zkbot commented Mar 12, 2019

Uh oh!

zkbot commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

str4d commented Jan 4, 2019 •

edited

Loading

mdr0id commented Feb 20, 2019 •

edited

Loading

mdr0id left a comment •

edited

Loading