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

Remove accounts rpcs #14023

Merged
merged 4 commits into from Aug 27, 2018

Conversation

Projects
None yet
9 participants
@jnewbery
Copy link
Member

commented Aug 22, 2018

This is the first part of #13825. It simply removes the RPC methods and tests.

#13825 touches lots of files and will require frequent rebasing.

Breaking it down for easier reviewing and fewer rebases.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
  • #13945 (Refactoring CRPCCommand with enum category by isghe)
  • #13249 (Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. by practicalswift)
  • #12490 ([Wallet] [RPC] Remove deprecated wallet rpc features from bitcoin_server by jnewbery)
  • #11652 (Add missing locks: validation.cpp + related by practicalswift)
  • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
  • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@promag

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

LGTM. Should update contrib/bitcoin-cli.bash-completion?

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

Is it still possible for a wallet to be in a state where it cannot spend funds unless they're first 'move'ed into the default account?

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2018

Is it still possible for a wallet to be in a state where it cannot spend funds unless they're first 'move'ed into the default account?

I'm not aware of that issue. If you can point to a github issue or some other reference describing what you're alluding to, that would be helpful.

In any case, the sendmany and sendtoaddress RPCs will not consider accounts if an account argument is not passed, so I don't see how the wallet could ever be in that state, either before or after this PR.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

utACK 01137c1. This only removes test and rpc code, not touching the wallet code.

@PierreRochard

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2018

Tested ACK 01137c1, will update after the rebase

[tests] Remove wallet accounts test
The accounts API will be removed in the next commit. Remove all
functional tests for the accounts API.

@jnewbery jnewbery force-pushed the jnewbery:remove_accounts_rpcs branch from 01137c1 Aug 27, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

Rebased and release note added.

@laanwj laanwj added this to Blockers in High-priority for review Aug 27, 2018

src/wallet/rpcwallet.cpp Outdated
}
if (wtx.GetDepthInMainChain() < 1)
entry.pushKV("category", "orphan");
else if (wtx.GetBlocksToMaturity() > 0)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 27, 2018

Member

Any reason for this change?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Aug 27, 2018

Author Member

No reason, except a bad rebase. Thanks for catching this!

Fixed in bb08423

This comment has been minimized.

Copy link
@PierreRochard

PierreRochard Aug 27, 2018

Contributor

Good catch Marco. Since this slipped past me, I did some research and found that the CLion IDE effectively highlights the change, while github and diff2html do not. Adding it to my review process.

clion ide

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Almost utACK 288bccc472fe5a91acc0f4029a1daff689aead2f

jnewbery added some commits Jul 27, 2018

[wallet] Remove wallet account RPCs
Also remove the RPC deprecation tests for accounts, and make one small
change to another wallet test that relies on account behaviour.
[wallet] Re-sort wallet RPC commands
This wasn't done in previous commit to make diff more reviewable.

@jnewbery jnewbery force-pushed the jnewbery:remove_accounts_rpcs branch to bb08423 Aug 27, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 27, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

utACK bb08423

@laanwj laanwj merged commit bb08423 into bitcoin:master Aug 27, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 27, 2018

Merge #14023: Remove accounts rpcs
bb08423 [doc] Add release notes for 'account' API removal (John Newbery)
1f4b865 [wallet] Re-sort wallet RPC commands (John Newbery)
f0dc850 [wallet] Remove wallet account RPCs (John Newbery)
c410f41 [tests] Remove wallet accounts test (John Newbery)

Pull request description:

  This is the first part of #13825. It simply removes the RPC methods and tests.

  #13825 touches lots of files and will require frequent rebasing.

  Breaking it down for easier reviewing and fewer rebases.

Tree-SHA512: d29af8e7a035e4484e6b9bb56cb86592be0ec112d8ba4ce19c15d15366ff3086e89e99fca26b90c9d66f6d3e06894486d0f29948df0bb7dcb1e2c49c6887a85a
@promag
Copy link
Member

left a comment

utACK bb08423, maybe fix typo in the next PR.

@@ -0,0 +1,8 @@
Accout API removed

This comment has been minimized.

Copy link
@promag

promag Aug 27, 2018

Member

Typo.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 30, 2018

Member

FWIW, codespell catched this typo. Fixed in the codespell PR #13954.

Please review :-)

@laanwj laanwj removed this from Blockers in High-priority for review Aug 28, 2018

@PierreRochard

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

Post-merge tested ACK bb08423

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 6, 2018

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 10, 2018

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 10, 2018

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Oct 10, 2018

[wallet] Restore ability to list incoming transactions by label
Backport of PR 14411 to v0.17.

This change partially reverts bitcoin#13075 and bitcoin#14023.

Fixes bitcoin#14382

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 15, 2018

laanwj added a commit that referenced this pull request Nov 10, 2018

Merge #14441: [wallet] Backport(0.17): Restore ability to list incomi…
…ng transactions by label

89306ab [wallet] Restore ability to list incoming transactions by label (Russell Yanofsky)

Pull request description:

  Backport of PR #14411 to v0.17.

  This change partially reverts #13075 and #14023.

  Fixes #14382

Tree-SHA512: 1f8300e1a79e826cd706561265b8788deef505fa510be1a76ed9a62e5fca37cf6a741423ac0e5de2a36d6e8b9f25f141885455aacacbbf6474814e6eae406a27

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 12, 2018

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 13, 2018

MarcoFalke added a commit that referenced this pull request Nov 14, 2018

Merge #14411: [wallet] Restore ability to list incoming transactions …
…by label

da427db Rename ListTransactions filter variable (Russell Yanofsky)
65b740f [wallet] Restore ability to list incoming transactions by label (Russell Yanofsky)

Pull request description:

  This change partially reverts #13075 and #14023.

  Fixes #14382

Tree-SHA512: 8c4e56104b3a45784cdc06bae8e5facdfff04fe3545b63a35e0ec2e440a41b79d84833ca4c4e728d8af7ebb8a519303a9eda7bee4bbfb92bd50c58587a33eb30

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

bvbfan added a commit to bvbfan/bitcoin that referenced this pull request Feb 15, 2019

[wallet] Restore ability to list incoming transactions by label
Backport of PR 14411 to v0.17.

This change partially reverts bitcoin#13075 and bitcoin#14023.

Fixes bitcoin#14382
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.