Remove g_rpc_chain global #19096

ryanofsky · 2020-05-28T18:04:49Z

Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference.

This PR is a followup to #18740 removing the g_rpc_node global.

Some later PRs will follow this up and move more wallet globals to the WalletContext struct.

Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to 25ad2c6 bitcoin#18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

practicalswift · 2020-05-28T19:10:16Z

Concept ACK

DrahtBot · 2020-05-28T21:07:07Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions #19100 (refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions by ryanofsky)
wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923 (wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off by MarcoFalke)
Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)

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

Approach ACK, just two comments before ACK code change.

src/interfaces/wallet.cpp

src/wallet/rpcwallet.cpp

ryanofsky

Thanks for simplification and review

Updated 2058621 -> e1cfcd4 (pr/wc.1 -> pr/wc.2, compare) with suggested change

src/interfaces/wallet.cpp

src/wallet/rpcwallet.cpp

promag · 2020-05-31T23:38:15Z

ACK e1cfcd4.

maflcko

Concept ACK e1cfcd4

src/rpc/request.h

src/wallet/rpcwallet.cpp

ryanofsky

Thanks for review!

Updated e1cfcd4 -> 4a7253a (pr/wc.2 -> pr/wc.3, compare)

src/rpc/request.h

src/wallet/rpcwallet.cpp

ariard

Code Review ACK 4a7253a, feel free to ignore comment it's super nit.

Thanks to removing global in RPC, I think it would make it simpler to remove RPC methods from the Chain interface, and have the wallet listen on its own ports?

ariard · 2020-06-01T23:38:38Z

src/wallet/context.h

+class Chain;
+} // namespace interfaces
+
+//! WalletContext struct containing references to state shared between CWallet


If it has to contain references for multiple wallet why not call it WalletsContext ? We have one node but maybe multiple wallets connected to it.

re: #19096 (comment)

If it has to contain references for multiple wallet why not call it WalletsContext ? We have one node but maybe multiple wallets connected to it.

I think using "wallets" instead of "wallet" is good where it can prevent ambiguity and misuse (#12221), but using plural where a name might apply to more than one object doesn't always sound right (hairscut, sailsboat) and can create confusion on its on (-disablewallets, -walletsnotify) because it can imply some instead of all. In this case, I think wallets is more distracting and doesn't sound right.

maflcko · 2020-06-02T14:33:00Z

ACK 4a7253a 🎋

Show signature and timestamp

Signature:

Timestamp of file with hash 4ff2c313852eb5872f3d1eec1aa996276e07d1a8f4efe878195311834e130a3c -

maflcko · 2020-06-05T12:11:35Z

@ryanofsky What is the status of this? I am currently undecided on whether to send this into rebase hell or merge.

ryanofsky

re: MarcoFalke #19096 (comment)

@ryanofsky What is the status of this? I am currently undecided on whether to send this into rebase hell or merge.

Status is fine to merge as far as I know. It doesn't seem to conflict with very much #19096 (comment):

refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions #19100 (refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions by ryanofsky)

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923 (wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off by MarcoFalke)

Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)

re: ariard #19096 (review)

Thanks to removing global in RPC, I think it would make it simpler to remove RPC methods from the Chain interface, and have the wallet listen on its own ports?

Adding listening ports is a good idea and has been suggested before. It's true that if it were implemented and backwards compatibility were dropped this PR would be slightly simpler. But I wouldn't want to implement a new feature and break existing usage cases just to remove a global variable.

ryanofsky · 2020-06-04T11:00:41Z

src/wallet/context.h

+class Chain;
+} // namespace interfaces
+
+//! WalletContext struct containing references to state shared between CWallet


re: #19096 (comment)

If it has to contain references for multiple wallet why not call it WalletsContext ? We have one node but maybe multiple wallets connected to it.

I think using "wallets" instead of "wallet" is good where it can prevent ambiguity and misuse (#12221), but using plural where a name might apply to more than one object doesn't always sound right (hairscut, sailsboat) and can create confusion on its on (-disablewallets, -walletsnotify) because it can imply some instead of all. In this case, I think wallets is more distracting and doesn't sound right.

4a7253a Remove g_rpc_chain global (Russell Yanofsky) e783197 refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands (Russell Yanofsky) Pull request description: Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to bitcoin#18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. ACKs for top commit: MarcoFalke: ACK 4a7253a 🎋 ariard: Code Review ACK 4a7253a, feel free to ignore comment it's super nit. Tree-SHA512: 5bb5561c89f81811ca5232a58bf450e230d4218e62471c03227d142395fd36131672e99cb88329b33b9680a235db01e8b9d1c1e2a18288349e57205528deabab

getauxblock is still broken due to bitcoin/bitcoin#19096 and will be fixed in a follow-up commit.

Unlike any upstream method, getauxblock is a wallet method but needs the NodeContext. This got broken with the upstream change in bitcoin/bitcoin#19096, because now the request context is a WalletContext. To fix it, we add a NodeContext pointer to WalletContext and in this way pass both to the wallet methods. EnsureNodeContext is able to extract the NodeContext then accordingly in both cases.

Summary: ``` Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to #18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. ``` Backport of core [[bitcoin/bitcoin#19096 | PR19096]]. Adapted to match our wallet dump RPCs separation. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8586

ryanofsky and others added 2 commits May 28, 2020 02:13

refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands

e783197

maflcko added Refactoring Wallet labels May 28, 2020

ryanofsky mentioned this pull request May 28, 2020

refactor: remove ::vpwallets and related global variables #19101

Merged

promag reviewed May 28, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Show resolved Hide resolved

This was referenced May 28, 2020

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923

Merged

Use shared pointers only in validation interface #18354

Closed

refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions #19100

Merged

ryanofsky force-pushed the pr/wc branch from 2058621 to e1cfcd4 Compare May 29, 2020 12:24

ryanofsky commented May 29, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Show resolved Hide resolved

maflcko approved these changes Jun 1, 2020

View reviewed changes

src/rpc/request.h Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Show resolved Hide resolved

ryanofsky force-pushed the pr/wc branch from e1cfcd4 to 4a7253a Compare June 1, 2020 16:59

ryanofsky commented Jun 1, 2020

View reviewed changes

src/rpc/request.h Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Show resolved Hide resolved

ariard reviewed Jun 1, 2020

View reviewed changes

ryanofsky commented Jun 5, 2020

View reviewed changes

maflcko merged commit 0fc6ea2 into bitcoin:master Jun 5, 2020

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Jun 8, 2020

Merge branch 'bitcoin' into auxpow

1b6396d

getauxblock is still broken due to bitcoin/bitcoin#19096 and will be fixed in a follow-up commit.

kwvg mentioned this pull request Oct 7, 2021

merge bitcoin#15931, #16839, #17192, #17407, #18037, #17997, partial #15639, #17989: deglobalisation and mining rpc backports dashpay/dash#4357

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove g_rpc_chain global #19096

Remove g_rpc_chain global #19096

ryanofsky commented May 28, 2020

practicalswift commented May 28, 2020

DrahtBot commented May 28, 2020 •

edited

Loading

promag left a comment

ryanofsky left a comment

promag commented May 31, 2020

maflcko left a comment

ryanofsky left a comment

ariard left a comment

ariard Jun 1, 2020

ryanofsky Jun 4, 2020

maflcko commented Jun 2, 2020

maflcko commented Jun 5, 2020

ryanofsky left a comment

ryanofsky Jun 4, 2020

Remove g_rpc_chain global #19096

Remove g_rpc_chain global #19096

Conversation

ryanofsky commented May 28, 2020

practicalswift commented May 28, 2020

DrahtBot commented May 28, 2020 • edited Loading

Conflicts

promag left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

promag commented May 31, 2020

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

ariard Jun 1, 2020

Choose a reason for hiding this comment

ryanofsky Jun 4, 2020

Choose a reason for hiding this comment

maflcko commented Jun 2, 2020

maflcko commented Jun 5, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jun 4, 2020

Choose a reason for hiding this comment

DrahtBot commented May 28, 2020 •

edited

Loading