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

Detach wallet from miner #3762

Merged
merged 9 commits into from Mar 12, 2019

Conversation

@str4d
Copy link
Contributor

commented Jan 4, 2019

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

Part of #2074.

@str4d str4d added this to Needs Prioritization in Arborist Team via automation Jan 4, 2019

@str4d str4d force-pushed the str4d:2074-detach-wallet-from-miner branch from 884a21b to f1fa2a4 Jan 4, 2019

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

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 😅

@str4d str4d moved this from Needs Prioritization to PRs That Need Review + Their Associated Issues in Arborist Team Jan 4, 2019

@Eirik0
Copy link
Contributor

left a comment

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.

@@ -1750,6 +1746,8 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
return InitError(_("-mineraddress is not in the local wallet. Either use a local address, or set -minetolocalwallet=0"));
}
#endif // ENABLE_WALLET

GetMainSignals().ScriptForMining.connect(GetScriptForMinerAddress);

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Jan 9, 2019

Contributor

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 comment has been minimized.

Copy link
@str4d

str4d Jan 24, 2019

Author Contributor

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.
class CScript;
#ifdef ENABLE_WALLET
class CReserveKey;
class CWallet;

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Jan 9, 2019

Contributor

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.

void GetScriptForMinerAddress(CScript &script)
class MinerAddressScript : public CReserveScript
{
void KeepScript() {}

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Jan 9, 2019

Contributor

This could probably use a comment.


# Find a coinbase address on the node, filtering by the number of UTXOs it has.
# The default cached chain has one address per coinbase output.
def get_coinbase_address(node, expected_utxos):

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Jan 9, 2019

Contributor

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.

This comment has been minimized.

Copy link
@str4d

str4d Jan 24, 2019

Author Contributor

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).

This comment has been minimized.

Copy link
@str4d

str4d Jan 28, 2019

Author Contributor

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

@Eirik0 Eirik0 moved this from PRs That Need Review + Their Associated Issues to In Review in Arborist Team Jan 9, 2019

@mms710 mms710 moved this from In Review to Review Backlog in Arborist Team Jan 17, 2019

@mms710 mms710 moved this from Review Backlog to Current Sprint in Arborist Team Jan 17, 2019

@mms710 mms710 moved this from Current Sprint to In Progress in Arborist Team Jan 17, 2019

@mms710 mms710 moved this from In Progress to Current Sprint in Arborist Team Jan 17, 2019

@mms710 mms710 moved this from Current Sprint to Sapling Priorites in Arborist Team Jan 17, 2019

@mms710 mms710 moved this from Sapling Priorites to Sprint Backlog in Arborist Team Jan 17, 2019

@mms710 mms710 moved this from Sprint Backlog to Blocked in Arborist Team Jan 17, 2019

@bitcartel bitcartel moved this from Blocked to In Progress in Arborist Team Jan 24, 2019

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

Addressed @Eirik0's comments.

@str4d str4d moved this from In Progress to In Review in Arborist Team Jan 28, 2019

@mms710 mms710 requested review from daira and mdr0id Jan 29, 2019

@Eirik0

Eirik0 approved these changes Jan 30, 2019

Copy link
Contributor

left a comment

utACK

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

⌛️ Trying commit bcb3d6c with merge e8d8897...

zkbot added a commit that referenced this pull request Jan 30, 2019

Auto merge of #3762 - str4d:2074-detach-wallet-from-miner, r=<try>
Detach wallet from miner

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

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

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

@mdr0id

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

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

@mdr0id
Copy link
Contributor

left a comment

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

str4d added some commits Mar 5, 2019

Move utiltest.cpp from wallet to common
This ensures it is accessible by the test suite when the wallet is
disabled.
Move payment disclosure code and tests into wallet
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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

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

@mdr0id mdr0id requested review from mdr0id and Eirik0 Mar 6, 2019

@mdr0id

mdr0id approved these changes Mar 6, 2019

@mdr0id

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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

@Eirik0

Eirik0 approved these changes Mar 12, 2019

Copy link
Contributor

left a comment

reACK

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

📌 Commit 1fee150 has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

⌛️ Testing commit 1fee150 with merge 224635b...

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

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

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

Part of #2074.

@mms710 mms710 moved this from In Review to Merge Queue in Arborist Team Mar 12, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

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

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

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

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

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

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

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

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Mar 12, 2019

@zkbot zkbot referenced this pull request Mar 12, 2019

Open

3553 listnotes rpc test #3569

@str4d str4d referenced this pull request Mar 12, 2019

Open

Bitcoin Core 0.12.0 #2074

196 of 452 tasks complete

@str4d str4d deleted the str4d:2074-detach-wallet-from-miner branch Mar 12, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.