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

Implement watchonly for fundrawtransaction #5524

Closed
wants to merge 5 commits into from

Conversation

kanzure
Copy link
Contributor

@kanzure kanzure commented Dec 21, 2014

Support watchonly in fundrawtransaction, CreateTransaction, SelectCoins and coin selection in general.

Coin selection is exposed over RPC through fundrawtransaction, and watchonly can be enabled by passing includeWatching true (defaults to false).

fundrawtransaction will first attempt to fund a transaction without using watchonly, even when includeWatching is enabled. When this fails, an includeWatching attempt is made.

When an includeWatching attempt at coin selection is performed (in CreateTransaction), and all watchonlys are consumed leaving no funds available for a fee, then CreateTransaction will not include a fee in the transaction.

Also, includeWatching tests (for fundrawtransaction) added here are passing.

Reviewers! Here are some things I would especially appreciate eyelooking for: suggestions for more edge cases for the tests, other RPC commands that should be tested, other tests for other features that I should double check for whether or not this makes any impacts, locations or existence of any sort of documentation I should be updating, unintended side effects introduced in CreateTransaction like by multiple parameters that may eventually be used simultaneously in ways that I am failing to anticipate....

I received a few suggestions from others that SignSignature should be replaced with an estimator in CreateTransaction. However, this estimator apparently does not seem to be necessary for implementing watchonly-support here, so this suggestion may go into another ticket instead.

This requires #5503 to be merged (I wouldn't mind this getting merged into #5503).

@jonasschnelli
Copy link
Contributor

That was quick.
Nice!

Maybe it makes more sense to reopen this pull when #5503 is stable and got ACK and merged. I prefer to have a easy reviewable code-pice because reviewing and testing is the bottleneck.
Also it prevents you from rebasing all the time.

@jonasschnelli
Copy link
Contributor

Breaks travis.
Maybe you close and reopen this pull whenever #5503 makes it into master?

@kanzure kanzure force-pushed the fundrawtransaction-watchonly branch 2 times, most recently from 59d167d to cd51e3e Compare January 16, 2015 21:31
@kanzure
Copy link
Contributor Author

kanzure commented Jan 19, 2015

Breaks travis

Tests are passing now.

@kanzure kanzure force-pushed the fundrawtransaction-watchonly branch from cd51e3e to 3ce454f Compare January 31, 2015 21:12
@laanwj laanwj added the Wallet label Feb 27, 2015
@jonasschnelli
Copy link
Contributor

@kanzure could you rebase on top of #5503?

Conceptual ACK.
Could also go into 0.11 together with #5503 IMO.

Support watchonly in fundrawtransaction, CreateTransaction, SelectCoins
and coin selection in general.

Coin selection is exposed over RPC through fundrawtransaction, and
watchonly can be enabled by passing includeWatching true (defaults to
false).

fundrawtransaction will first attempt to fund a transaction without
using watchonly, even when includeWatching is enabled. When this fails,
an includeWatching attempt is made.

When an includeWatching attempt at coin selection is performed (in
CreateTransaction), and all watchonlys are consumed leaving no funds
available for a fee, then CreateTransaction will not include a fee in
the transaction.

Also, includeWatching tests (for fundrawtransaction).

See also: bitcoin#5503
@kanzure kanzure force-pushed the fundrawtransaction-watchonly branch from 3ce454f to a3af4c1 Compare March 15, 2015 17:36
@kanzure
Copy link
Contributor Author

kanzure commented Mar 15, 2015

Rebased on to the fundrawtransaction branch. Travis is reporting an error:

Tests successful
Running testscript rawtransactions.py...
Initializing test directory /tmp/testZdbica
JSONRPC error: Expected type int, got str
  File "/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/qa/rpc-tests/test_framework.py", line 117, in main
    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/qa/rpc-tests/rawtransactions.py", line 68, in run_test
    rawtxfund = self.nodes[2].fundrawtransaction(rawtx)
  File "/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/qa/rpc-tests/python-bitcoinrpc/bitcoinrpc/authproxy.py", line 126, in __call__
    raise JSONRPCException(response['error'])
Cleaning up

+ HelpExampleRpc("sendrawtransaction", "\"signedhex\"")
);

RPCTypeCheck(params, boost::assign::list_of(int_type)(int_type)(array_type));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be RPCTypeCheck(params, boost::assign::list_of(str_type)(bool_type));.

@TheBlueMatt
Copy link
Contributor

Why try to fund without watchonly first? If I call the RPC and say I want to include watchonly, it means I want to include watchonly (and get the best fee from the inputs I opted to select).

@jonasschnelli
Copy link
Contributor

I'd like to add this to #5503 but the nFeeRet=0 as well as the issue mentioned by @TheBlueMatt (calling CreateTransaction() two times) are unclear and needs fixing. RPC tests should also send a funded raw transaction and check the response to make sure it passes the mempool checks (i have a feeling that it won't because of the generated fee)

TheBlueMatt added a commit to TheBlueMatt/bitcoin that referenced this pull request Apr 24, 2015
Some code and test cases stolen from
Bryan Bishop <bryan@ledgerx.com> (pull bitcoin#5524).
TheBlueMatt added a commit to TheBlueMatt/bitcoin that referenced this pull request Apr 30, 2015
Some code and test cases stolen from
Bryan Bishop <bryan@ledgerx.com> (pull bitcoin#5524).
@TheBlueMatt TheBlueMatt mentioned this pull request Apr 30, 2015
@kanzure
Copy link
Contributor Author

kanzure commented Apr 30, 2015

Closed in favor of #6088.

@kanzure kanzure closed this Apr 30, 2015
TheBlueMatt added a commit to TheBlueMatt/bitcoin that referenced this pull request Jul 20, 2015
Some code and test cases stolen from
Bryan Bishop <bryan@ledgerx.com> (pull bitcoin#5524).
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jun 4, 2020
- backports bitcoin/bitcoin@6bdb474

Some code and test cases stolen from Bryan Bishop <bryan@ledgerx.com>
(pull bitcoin#5524).

- backports bitcoin/bitcoin@d042854

[squash commit]
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jun 5, 2020
- backports bitcoin/bitcoin@6bdb474

Some code and test cases stolen from Bryan Bishop <bryan@ledgerx.com>
(pull bitcoin#5524).

- backports bitcoin/bitcoin@d042854

[squash commit]
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jun 9, 2020
- backports bitcoin/bitcoin@6bdb474

Some code and test cases stolen from Bryan Bishop <bryan@ledgerx.com>
(pull bitcoin#5524).

- backports bitcoin/bitcoin@d042854

[squash commit]
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jun 28, 2020
- backports bitcoin/bitcoin@6bdb474

Some code and test cases stolen from Bryan Bishop <bryan@ledgerx.com>
(pull bitcoin#5524).

- backports bitcoin/bitcoin@d042854

[squash commit]
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Jul 4, 2020
- backports bitcoin/bitcoin@6bdb474

Some code and test cases stolen from Bryan Bishop <bryan@ledgerx.com>
(pull bitcoin#5524).

- backports bitcoin/bitcoin@d042854

[squash commit]
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants