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

Switch miner to P2PKH, add -mineraddress option #1965

Merged
merged 6 commits into from Feb 9, 2017

Conversation

Projects
None yet
5 participants
@str4d
Copy link
Contributor

commented Dec 23, 2016

Closes #945 and #1955.

@str4d str4d added this to the 1.0.5 milestone Dec 23, 2016

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2016

This PR is branched from #1863; merge that first.


CScript scriptPubKey = CScript() << ToByteVector(pubkey) << OP_CHECKSIG;
CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(keyID) << OP_EQUALVERIFY << OP_CHECKSIG;

This comment has been minimized.

Copy link
@str4d

str4d Jan 4, 2017

Author Contributor

Reviewers, please double-check that I have this P2PKH script correct.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Jan 18, 2017

Contributor

It's correct.

@bitcartel bitcartel requested review from bitcartel, daira and ebfull Jan 16, 2017

@ebfull ebfull self-assigned this Jan 18, 2017

@bitcartel
Copy link
Contributor

left a comment

There is no error if the mineraddress is invalid, whether empty whitespace, garbage or an address with a typo.

Instead, mining proceeds successfully with the coinbase reward going to address tm9iMLAuYMzJ6jtFLcA7rzUmfreGuKvr7Ma.

This is the address when keyID is null.

./zcash-cli validateaddress tm9iMLAuYMzJ6jtFLcA7rzUmfreGuKvr7Ma
{
"isvalid" : true,
"address" : "tm9iMLAuYMzJ6jtFLcA7rzUmfreGuKvr7Ma",
"scriptPubKey" : "76a914000000000000000000000000000000000000000088ac",
"ismine" : false
}

The above should be fixed and automated tests written to check input validity and spending the coinbase.

Also consider adding an option -mineraddressismine (default true) where the user must explicitly set this to false to allow mining to an address not in the wallet.

Reporting problems with the miner address, whether invalid or not mine, should be at launch so that the user is notified immediately.

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

@str4d str4d force-pushed the str4d:1955-single-address-mining branch from b6eb162 to aa0081f Jan 26, 2017

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2017

@str4d Getting compile errors:

libbitcoin_server.a(libbitcoin_server_a-rpcmining.o): In function `getblocktemplate(std::vector<json_spirit::Value_impl<json_spirit::Config_vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<json_spirit::Value_impl<json_spirit::Config_vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > const&, bool)':
/home/xxxxxxxxx/projects/zcash/src/rpcmining.cpp:621: undefined reference to `CreateNewBlockWithKey(CReserveKey&)'
collect2: error: ld returned 1 exit status
Makefile:3186: recipe for target 'zcashd' failed

@str4d str4d force-pushed the str4d:1955-single-address-mining branch from aa0081f to a4c34ea Jan 27, 2017

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2017

@bitcartel Strange, I don't get those errors...

Re: the current merge conflict, I'll rebase this on master once #1863 is merged.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2017

Ah, the error @bitcartel has shows up with --disable-mining. Fixing.

@str4d str4d force-pushed the str4d:1955-single-address-mining branch from a4c34ea to c3d2145 Jan 29, 2017

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2017

@bitcartel I like your idea of a -minetolocalwallet flag (I'm using that because -mineraddressismine is a bit hard to read), but should the default be false when the zcashd is compiled without a wallet?

@str4d str4d force-pushed the str4d:1955-single-address-mining branch 2 times, most recently from ecdad51 to ae8dae9 Jan 30, 2017

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2017

Rebased on #1863 again (which fixed the merge conflict, since #1863 has been rebased on 1.0.5), and fixed some things.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2017

@str4d

-minetolocalwallet flag... should the default be false when the zcashd is compiled without a wallet?

Sounds good. If zcashd doesn't have a wallet, then the flag should be disregarded.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

@str4d compile error

rpcmining.cpp: In function ‘json_spirit::Value generate(const Array&, bool)’:
rpcmining.cpp:271:1: error: a function-definition is not allowed here before ‘{’ token
 {
 ^
rpcmining.cpp:905:1: error: expected ‘}’ at end of input
 }
 ^
Makefile:4250: recipe for target 'libbitcoin_server_a-rpcmining.o' failed
make[2]: *** [libbitcoin_server_a-rpcmining.o] Error 1

@str4d str4d force-pushed the str4d:1955-single-address-mining branch from 2ed1f99 to a7d6500 Feb 8, 2017

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

@str4d Did you change the help text? I still see two cases of ...set -minetolocalwallet=false... in init.cpp.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

@bitcartel whoops, I changed the help text but not the error messages. Fixing...

@str4d str4d force-pushed the str4d:1955-single-address-mining branch from a7d6500 to f49bcdf Feb 8, 2017

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

ACK

@daira
Copy link
Contributor

left a comment

utACK, but I'm going to smoke-test it with all four combinations of {ENABLE_WALLET, ENABLE_MINER} as well, because there's a lot of conditional code here.

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

If I disable the wallet (by editing build.sh to pass --enable-wallet=no to configure) but do not disable the miner and do not use -mineraddress, I expect to get a failure on startup. However, I actually still see "You are mining with the tromp solver on 4 threads." on the metrics screen, even though no mining takes place.

Better error reporting for the !ENABLE_WALLET && ENABLE_MINING case.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

ACK everything except my last patch.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

ACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

📌 Commit 9bba9b3 has been approved by bitcartel

zkbot added a commit that referenced this pull request Feb 9, 2017

Auto merge of #1965 - str4d:1955-single-address-mining, r=bitcartel
Switch miner to P2PKH, add -mineraddress option

Closes #945 and #1955.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

⌛️ Testing commit 9bba9b3 with merge 0c78782...

@str4d
Copy link
Contributor Author

left a comment

@daira @bitcartel I'm not going to stop this merging, but see my comment.

return InitError(_("Zcash was not built with wallet support. Set -minetolocalwallet=0 to use -mineraddress, or rebuild Zcash with wallet support."));
}
if (!mapArgs.count("-mineraddress")) {
return InitError(_("Zcash was not built with wallet support. Set -mineraddress, or rebuild Zcash with wallet support."));

This comment has been minimized.

Copy link
@str4d

str4d Feb 9, 2017

Author Contributor

As-is, this will require a build with !ENABLE_WALLET && ENABLE_MINING to always have -mineraddress set, even if -gen is not set. Is this what we want? It does make some sense, because zcash-cli setgenerate wouldn't work properly if -mineraddress wasn't set, and the user likely does want to mine in this setup, but I'm not sure we should assume that.

This comment has been minimized.

Copy link
@daira

daira Feb 9, 2017

Contributor

Ideally this would be allowed when -gen is not set, but setgenerate would fail.

This comment has been minimized.

Copy link
@daira

daira Feb 10, 2017

Contributor

Fixed in #2081

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 9bba9b3 into zcash:master Feb 9, 2017

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:1955-single-address-mining branch Feb 10, 2017

zkbot added a commit that referenced this pull request Feb 10, 2017

Auto merge of #2081 - daira:1955-single-address-mining, r=daira
Address @str4d's comment on #1965 about the case where -gen is not set

#1965 (comment)

Note that the case of calling the ``setgenerate true`` RPC with the wallet disabled was already handled correctly.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>

zkbot added a commit that referenced this pull request Feb 10, 2017

Auto merge of #2081 - daira:1955-single-address-mining, r=daira
Address @str4d's comment on #1965 about the case where -gen is not set

#1965 (comment)

Note that the case of calling the ``setgenerate true`` RPC with the wallet disabled was already handled correctly.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>

zkbot added a commit that referenced this pull request Feb 10, 2017

Auto merge of #2081 - daira:1955-single-address-mining, r=daira
Address @str4d's comment on #1965 about the case where -gen is not set

#1965 (comment)

Note that the case of calling the ``setgenerate true`` RPC with the wallet disabled was already handled correctly.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>

zkbot added a commit that referenced this pull request Feb 10, 2017

Auto merge of #2081 - daira:1955-single-address-mining, r=daira
Address @str4d's comment on #1965 about the case where -gen is not set

#1965 (comment)

Note that the case of calling the ``setgenerate true`` RPC with the wallet disabled was already handled correctly.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>

zkbot added a commit that referenced this pull request Feb 10, 2017

Auto merge of #2081 - daira:1955-single-address-mining, r=bitcartel
Address @str4d's comment on #1965 about the case where -gen is not set

#1965 (comment)

Note that the case of calling the ``setgenerate true`` RPC with the wallet disabled was already handled correctly.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
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.