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

[RPC] Add generatetoaddress rpc to mine to an address #7671

Merged
merged 2 commits into from Mar 23, 2016

Conversation

Projects
None yet
6 participants
@achow101
Member

achow101 commented Mar 12, 2016

Add a new optional parameter to the generate call to mine to a specific address even if it doesn't exist in the wallet.

@jonasschnelli

View changes

Show outdated Hide outdated src/rpc/mining.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 12, 2016

Member

Nice!
utACK.

Member

jonasschnelli commented Mar 12, 2016

Nice!
utACK.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 12, 2016

Member

I like the concept, but perhaps we can separate that out to a different RPC (which then can function without wallet support)?

Also, the change in arguments to generate conflicts with #7663.

Member

sipa commented Mar 12, 2016

I like the concept, but perhaps we can separate that out to a different RPC (which then can function without wallet support)?

Also, the change in arguments to generate conflicts with #7663.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 12, 2016

Member

@sipa How about moving it to a new rpc generatetoaddress?

Done, but there is code duplication since most of it is copied from the generate rpc. I left it duplicated so that it doesn't conflict with #7663.

Member

achow101 commented Mar 12, 2016

@sipa How about moving it to a new rpc generatetoaddress?

Done, but there is code duplication since most of it is copied from the generate rpc. I left it duplicated so that it doesn't conflict with #7663.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 12, 2016

Member

This seems unnecessary. Why not just move GBT->block code from getblocktemplate_proposals.py to blocktools?

Member

luke-jr commented Mar 12, 2016

This seems unnecessary. Why not just move GBT->block code from getblocktemplate_proposals.py to blocktools?

@achow101 achow101 changed the title from [RPC] Allow mining to a specific address to [RPC] Add generatetoaddress rpc to mine to an address Mar 12, 2016

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 12, 2016

Member

This seems unnecessary.

Why?

Why not just move GBT->block code from getblocktemplate_proposals.py to blocktools?

Not sure what you mean by this.

Member

achow101 commented Mar 12, 2016

This seems unnecessary.

Why?

Why not just move GBT->block code from getblocktemplate_proposals.py to blocktools?

Not sure what you mean by this.

@achow101 achow101 referenced this pull request Mar 13, 2016

Closed

Add a regression test mode #22

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 14, 2016

Member

Concept ACK
Needs rebase after #7507,#7663

I like the concept, but perhaps we can separate that out to a different RPC (which then can function without wallet support)?

Sounds like a good idea to me. Mining without wallet support would be awesome for some tests.

Member

laanwj commented Mar 14, 2016

Concept ACK
Needs rebase after #7507,#7663

I like the concept, but perhaps we can separate that out to a different RPC (which then can function without wallet support)?

Sounds like a good idea to me. Mining without wallet support would be awesome for some tests.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 14, 2016

Member

Rebased

Member

achow101 commented Mar 14, 2016

Rebased

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 14, 2016

Member

I'd prefer to have rpc tests as part of this pull.

Member

MarcoFalke commented Mar 14, 2016

I'd prefer to have rpc tests as part of this pull.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 14, 2016

Member

@MarcoFalke Where would be the best place to put such tests?

Member

achow101 commented Mar 14, 2016

@MarcoFalke Where would be the best place to put such tests?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 14, 2016

Member

@achow101 You can either start from scratch (c.f. https://github.com/bitcoin/bitcoin/blob/48f39058315c908c360acb596923cbc090119480/qa/rpc-tests/disablewallet.py) or extend an existing python script in this folder.

Member

MarcoFalke commented Mar 14, 2016

@achow101 You can either start from scratch (c.f. https://github.com/bitcoin/bitcoin/blob/48f39058315c908c360acb596923cbc090119480/qa/rpc-tests/disablewallet.py) or extend an existing python script in this folder.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 14, 2016

Member

One of the primary use cases for this, I think, would be to have RPC tests where you get an address from one node, and let another node mine directly to it, without the need to generate 100 blocks in between before you can transfer.

Member

sipa commented Mar 14, 2016

One of the primary use cases for this, I think, would be to have RPC tests where you get an address from one node, and let another node mine directly to it, without the need to generate 100 blocks in between before you can transfer.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 14, 2016

Member

I added rpc tests, both with and without wallets.

The wallet RPC Test mines Bitcoin to another node's address and checks that that node has received the Bitcoin.

The RPC test without the wallet mines Bitcoin to an arbitrary address and checks that it works. It then mines to an arbitrary invalid address and checks that that fails.

Member

achow101 commented Mar 14, 2016

I added rpc tests, both with and without wallets.

The wallet RPC Test mines Bitcoin to another node's address and checks that that node has received the Bitcoin.

The RPC test without the wallet mines Bitcoin to an arbitrary address and checks that it works. It then mines to an arbitrary invalid address and checks that that fails.

@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/wallet.py
@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/disablewallet.py
@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 14, 2016

Member

addressed nits

Member

achow101 commented Mar 14, 2016

addressed nits

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 15, 2016

Member

Random Q: Should this also be able to take an arbitrary scriptPubKey?

One of the primary use cases for this, I think, would be to have RPC tests where you get an address from one node, and let another node mine directly to it,

Also eventually we want to have at least part of the RPC tests usable without wallet.

Member

laanwj commented Mar 15, 2016

Random Q: Should this also be able to take an arbitrary scriptPubKey?

One of the primary use cases for this, I think, would be to have RPC tests where you get an address from one node, and let another node mine directly to it,

Also eventually we want to have at least part of the RPC tests usable without wallet.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 15, 2016

Member

Random Q: Should this also be able to take an arbitrary scriptPubKey?

Do you think that it should?

Member

achow101 commented Mar 15, 2016

Random Q: Should this also be able to take an arbitrary scriptPubKey?

Do you think that it should?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 16, 2016

Member

Do you think that it should?

I don't deeply care myself. Just a question that came up with me, usually the discussion of custom scripts comes up immediately after allowing arbitrary addresses, as addresses map to a only a subset of the possible output scripts.

So to be clear, I'm ok with just leaving it like this.

Member

laanwj commented Mar 16, 2016

Do you think that it should?

I don't deeply care myself. Just a question that came up with me, usually the discussion of custom scripts comes up immediately after allowing arbitrary addresses, as addresses map to a only a subset of the possible output scripts.

So to be clear, I'm ok with just leaving it like this.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 17, 2016

Member

OK, I'll just leave it as is.

Member

achow101 commented Mar 17, 2016

OK, I'll just leave it as is.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 17, 2016

Member
Member

sipa commented Mar 17, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 21, 2016

Member

Needs rebase.

Member

laanwj commented Mar 21, 2016

Needs rebase.

Create generatetoaddress rpc
Creates the generatetoaddress rpc which is virtually identical to the generate rpc except that it takes an argument for the address to mine to. It does not rely on wallet functionality.

The mining code shared by generate and generatetoaddress has been moved to another method to reduce duplication.
@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Mar 21, 2016

Member

rebased

Member

achow101 commented Mar 21, 2016

rebased

@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/disablewallet.py
RPC tests for generatetoaddress
Adds two RPC tests for the generatetoaddress RPC, one in the wallet, and one when the wallet is disabled.

The wallet RPC Test mines Bitcoin to another node's address and checks that that node has received the Bitcoin.

The RPC test without the wallet mines Bitcoin to an arbitrary address and checks that it works. It then mines to an arbitrary invalid address and checks that that fails.

@laanwj laanwj merged commit d5c5c71 into bitcoin:master Mar 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 23, 2016

Merge #7671: [RPC] Add generatetoaddress rpc to mine to an address
d5c5c71 RPC tests for generatetoaddress (Andrew C)
fe00ca7 Create generatetoaddress rpc (Andrew C)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj
Member

laanwj commented Mar 23, 2016

ACK fe00ca7

@laanwj laanwj referenced this pull request Mar 23, 2016

Closed

TODO for release notes 0.13.0 #7678

14 of 16 tasks complete

@achow101 achow101 deleted the achow101:generate-to-addr branch Oct 29, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7671: [RPC] Add generatetoaddress rpc to mine to an address
d5c5c71 RPC tests for generatetoaddress (Andrew C)
fe00ca7 Create generatetoaddress rpc (Andrew C)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7671: [RPC] Add generatetoaddress rpc to mine to an address
d5c5c71 RPC tests for generatetoaddress (Andrew C)
fe00ca7 Create generatetoaddress rpc (Andrew C)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7671: [RPC] Add generatetoaddress rpc to mine to an address
d5c5c71 RPC tests for generatetoaddress (Andrew C)
fe00ca7 Create generatetoaddress rpc (Andrew C)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge #7671: [RPC] Add generatetoaddress rpc to mine to an address
d5c5c71 RPC tests for generatetoaddress (Andrew C)
fe00ca7 Create generatetoaddress rpc (Andrew C)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment