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

backport: merge bitcoin#21207, #22008, #22686, #22742, #19101, #22183, #22009, #22938, #23288, #24592 (wallet backports: part 2) #6529

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: separate out Dash-specific RPCs that rely on wallet logic
In an upcoming commit, wallet variables will be deglobalized. This means
that RPCs that use wallet logic need to get ahold of WalletContext, which
only happens if they're registered as a wallet RPC (i.e. registered
through WalletLoader).

The downside of being registered as a wallet RPC is that you lose access
to NodeContext. For now, we will work around this by giving WalletContext
access to NodeContext and modify EnsureAnyNodeContext to pull it from
WalletContext.
  • Loading branch information
kwvg committed Feb 17, 2025
commit fcfbdca934ed120613e8153f73f1afc5a1a425e9
18 changes: 18 additions & 0 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <index/txindex.h>
#include <interfaces/chain.h>
#include <interfaces/node.h>
#include <interfaces/wallet.h>
#include <mapport.h>
#include <node/miner.h>
#include <net.h>
Expand Down Expand Up @@ -1515,6 +1516,23 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
for (const auto& client : node.chain_clients) {
client->registerRpcs();
}
#ifdef ENABLE_WALLET
// Register non-core wallet-only RPC commands. These are commands that
// aren't a part of the wallet library but heavily rely on wallet logic.
// TODO: Move them to chain client interfaces so they can be called
// with registerRpcs()
if (!args.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
for (const auto& commands : {
GetWalletCoinJoinRPCCommands(),
GetWalletEvoRPCCommands(),
GetWalletGovernanceRPCCommands(),
GetWalletMasternodeRPCCommands(),
}) {
node.wallet_loader->registerOtherRpcs(commands);
}
}
#endif // ENABLE_WALLET

#if ENABLE_ZMQ
RegisterZMQRPCCommands(tableRPC);
#endif
Expand Down
16 changes: 12 additions & 4 deletions src/interfaces/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,22 @@
class CCoinControl;
class CFeeRate;
class CKey;
class CRPCCommand;
class CWallet;
class UniValue;
enum class FeeReason;
enum class TransactionError;
enum isminetype : unsigned int;
struct bilingual_str;
struct CRecipient;
struct NodeContext;
struct PartiallySignedTransaction;
struct WalletContext;
struct bilingual_str;
using isminefilter = std::underlying_type<isminetype>::type;

template <typename T>
class Span;

namespace interfaces {

class Handler;
Expand Down Expand Up @@ -335,8 +339,11 @@ class Wallet
class WalletLoader : public ChainClient
{
public:
//! Create new wallet.
virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
//! Register non-core wallet RPCs
virtual void registerOtherRpcs(const Span<const CRPCCommand>& commands) = 0;

//! Create new wallet.
virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;

//! Load existing wallet.
virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
Expand Down Expand Up @@ -450,7 +457,8 @@ std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet);

//! Return implementation of ChainClient interface for a wallet loader. This
//! function will be undefined in builds where ENABLE_WALLET is false.
std::unique_ptr<WalletLoader> MakeWalletLoader(Chain& chain, ArgsManager& args, const std::unique_ptr<CoinJoin::Loader>& coinjoin_loader);
std::unique_ptr<WalletLoader> MakeWalletLoader(Chain& chain, ArgsManager& args, NodeContext& node_context,
const std::unique_ptr<interfaces::CoinJoin::Loader>& coinjoin_loader);

} // namespace interfaces

Expand Down
45 changes: 35 additions & 10 deletions src/rpc/coinjoin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@
// Distributed under the MIT/X11 software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <node/context.h>
#include <validation.h>
#include <coinjoin/context.h>
#include <coinjoin/server.h>
#include <node/context.h>
#include <rpc/blockchain.h>
#include <rpc/server.h>
#include <rpc/server_util.h>
#include <util/check.h>
#include <rpc/util.h>
#include <util/check.h>
#include <util/strencodings.h>
#include <validation.h>
#include <wallet/rpc/util.h>
#include <walletinitinterface.h>

#ifdef ENABLE_WALLET
#include <coinjoin/client.h>
Expand Down Expand Up @@ -449,14 +450,13 @@ static RPCHelpMan getcoinjoininfo()
};
}

void RegisterCoinJoinRPCCommands(CRPCTable &t)
#ifdef ENABLE_WALLET
Span<const CRPCCommand> GetWalletCoinJoinRPCCommands()
{
// clang-format off
static const CRPCCommand commands[] =
{ // category actor (function)
// --------------------- -----------------------
{ "dash", &getcoinjoininfo, },
#ifdef ENABLE_WALLET
{ // category actor (function)
// --------------------- -----------------------
{ "dash", &coinjoin, },
{ "dash", &coinjoin_reset, },
{ "dash", &coinjoin_start, },
Expand All @@ -465,10 +465,35 @@ static const CRPCCommand commands[] =
{ "dash", &coinjoinsalt_generate, },
{ "dash", &coinjoinsalt_get, },
{ "dash", &coinjoinsalt_set, },
{ "dash", &getcoinjoininfo, },
};
// clang-format on
return commands;
}
#endif // ENABLE_WALLET

void RegisterCoinJoinRPCCommands(CRPCTable& t)
{
// clang-format off
static const CRPCCommand commands_wallet[] =
{ // category actor (function)
// --------------------- -----------------------
{ "dash", &getcoinjoininfo, },
};
// clang-format on
for (const auto& command : commands) {
t.appendCommand(command.name, &command);
// If we aren't compiling with wallet support, we still need to register RPCs that are
// capable of working without wallet support. We have to do this even if wallet support
// is compiled in but is disabled at runtime because runtime disablement prohibits
// registering wallet RPCs. We still want the reduced functionality RPC to be registered.
// TODO: Spin off these hybrid RPCs into dedicated wallet-only and/or wallet-free RPCs
// and get rid of this workaround.
if (!g_wallet_init_interface.HasWalletSupport()
#ifdef ENABLE_WALLET
|| gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)
#endif // ENABLE_WALLET
) {
for (const auto& command : commands_wallet) {
tableRPC.appendCommand(command.name, &command);
}
}
}
50 changes: 41 additions & 9 deletions src/rpc/evo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <util/translation.h>
#include <validation.h>
#include <wallet/rpc/util.h>
#include <walletinitinterface.h>

#ifdef ENABLE_WALLET
#include <wallet/coincontrol.h>
Expand Down Expand Up @@ -1777,17 +1778,15 @@ static RPCHelpMan bls_help()
};
}

void RegisterEvoRPCCommands(CRPCTable &tableRPC)
#ifdef ENABLE_WALLET
Span<const CRPCCommand> GetWalletEvoRPCCommands()
{
// clang-format off
static const CRPCCommand commands[] =
{ // category actor (function)
// --------------------- -----------------------
{ "evo", &bls_help, },
{ "evo", &bls_generate, },
{ "evo", &bls_fromsecret, },
{ "evo", &protx_help, },
#ifdef ENABLE_WALLET
{ "evo", &protx_list, },
{ "evo", &protx_info, },
{ "evo", &protx_register, },
{ "evo", &protx_register_evo, },
{ "evo", &protx_register_legacy, },
Expand All @@ -1803,14 +1802,47 @@ static const CRPCCommand commands[] =
{ "evo", &protx_update_registrar, },
{ "evo", &protx_update_registrar_legacy, },
{ "evo", &protx_revoke, },
#endif
{ "evo", &protx_list, },
{ "evo", &protx_info, },
};
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix Clang format issues.

The pipeline indicates formatting issues. Please run clang-format on the code to ensure consistent style.

🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check

[warning] 1805-1805: Clang format differences found. Please format the code according to Clang standards.

// clang-format on
return commands;
}
#endif // ENABLE_WALLET

void RegisterEvoRPCCommands(CRPCTable& tableRPC)
{
// clang-format off
static const CRPCCommand commands[] =
{ // category actor (function)
// --------------------- -----------------------
{ "evo", &bls_help, },
{ "evo", &bls_generate, },
{ "evo", &bls_fromsecret, },
{ "evo", &protx_help, },
{ "evo", &protx_diff, },
{ "evo", &protx_listdiff, },
};
static const CRPCCommand commands_wallet[] =
{
{ "evo", &protx_list, },
{ "evo", &protx_info, },
};
// clang-format on
for (const auto& command : commands) {
tableRPC.appendCommand(command.name, &command);
}
// If we aren't compiling with wallet support, we still need to register RPCs that are
// capable of working without wallet support. We have to do this even if wallet support
// is compiled in but is disabled at runtime because runtime disablement prohibits
// registering wallet RPCs. We still want the reduced functionality RPC to be registered.
// TODO: Spin off these hybrid RPCs into dedicated wallet-only and/or wallet-free RPCs
// and get rid of this workaround.
if (!g_wallet_init_interface.HasWalletSupport()
#ifdef ENABLE_WALLET
|| gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)
#endif // ENABLE_WALLET
) {
for (const auto& command : commands_wallet) {
tableRPC.appendCommand(command.name, &command);
}
}
}
23 changes: 17 additions & 6 deletions src/rpc/governance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1071,6 +1071,23 @@ static RPCHelpMan getsuperblockbudget()
};
}

#ifdef ENABLE_WALLET
Span<const CRPCCommand> GetWalletGovernanceRPCCommands()
{
// clang-format off
static const CRPCCommand commands[] =
{ // category actor (function)
// --------------------- -----------------------
{ "dash", &gobject_prepare, },
{ "dash", &gobject_list_prepared, },
{ "dash", &gobject_vote_many, },
{ "dash", &gobject_vote_alias, },
};
// clang-format on
return commands;
}
#endif // ENABLE_WALLET
Comment on lines +1074 to +1089
Copy link

@coderabbitai coderabbitai bot Feb 14, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix clang-format issues.

The pipeline reports formatting issues in this section. Please run clang-format to fix the formatting.

🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check

[warning] 1075-1075: Clang format differences found. Please format the code according to Clang standards.

Copy link
Collaborator Author

@kwvg kwvg Feb 14, 2025

Choose a reason for hiding this comment

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

We want this code to be excluded from clang-format, hence the comment

// clang-format off

Copy link

Choose a reason for hiding this comment

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

@kwvg I apologize for the incorrect suggestion. You are absolutely right - the // clang-format off directive is intentionally used here to maintain the readable alignment of the command registration array. I will remember this for future reviews.


✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:57.974Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


void RegisterGovernanceRPCCommands(CRPCTable &t)
{
// clang-format off
Expand All @@ -1084,12 +1101,6 @@ static const CRPCCommand commands[] =
{ "dash", &gobject_count, },
{ "dash", &gobject_deserialize, },
{ "dash", &gobject_check, },
#ifdef ENABLE_WALLET
{ "dash", &gobject_prepare, },
{ "dash", &gobject_list_prepared, },
{ "dash", &gobject_vote_many, },
{ "dash", &gobject_vote_alias, },
#endif
{ "dash", &gobject_submit, },
{ "dash", &gobject_list, },
{ "dash", &gobject_diff, },
Expand Down
17 changes: 14 additions & 3 deletions src/rpc/masternode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,20 @@ static RPCHelpMan masternodelist_composite()
return masternodelist_helper(true);
}

#ifdef ENABLE_WALLET
Span<const CRPCCommand> GetWalletMasternodeRPCCommands()
{
// clang-format off
static const CRPCCommand commands[] =
{ // category actor (function)
// --------------------- -----------------------
{ "dash", &masternode_outputs, },
};
// clang-format on
return commands;
}
#endif // ENABLE_WALLET

void RegisterMasternodeRPCCommands(CRPCTable &t)
{
// clang-format off
Expand All @@ -682,9 +696,6 @@ static const CRPCCommand commands[] =
{ "dash", &masternodelist, },
{ "dash", &masternode_connect, },
{ "dash", &masternode_count, },
#ifdef ENABLE_WALLET
{ "dash", &masternode_outputs, },
#endif // ENABLE_WALLET
{ "dash", &masternode_status, },
{ "dash", &masternode_payments, },
{ "dash", &masternode_winners, },
Expand Down
12 changes: 12 additions & 0 deletions src/rpc/register.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@

/** These are in one header file to avoid creating tons of single-function
* headers for everything under src/rpc/ */
class CRPCCommand;
class CRPCTable;

template <typename T>
class Span;

void RegisterBlockchainRPCCommands(CRPCTable &tableRPC);
void RegisterFeeRPCCommands(CRPCTable&);
void RegisterMempoolRPCCommands(CRPCTable&);
Expand All @@ -24,6 +28,14 @@ void RegisterGovernanceRPCCommands(CRPCTable &tableRPC);
void RegisterEvoRPCCommands(CRPCTable &tableRPC);
void RegisterQuorumsRPCCommands(CRPCTable &tableRPC);

#ifdef ENABLE_WALLET
// Dash-specific wallet-only RPC commands
Span<const CRPCCommand> GetWalletCoinJoinRPCCommands();
Span<const CRPCCommand> GetWalletEvoRPCCommands();
Span<const CRPCCommand> GetWalletGovernanceRPCCommands();
Span<const CRPCCommand> GetWalletMasternodeRPCCommands();
#endif // ENABLE_WALLET

static inline void RegisterAllCoreRPCCommands(CRPCTable &t)
{
RegisterBlockchainRPCCommands(t);
Expand Down
20 changes: 17 additions & 3 deletions src/rpc/server_util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,29 @@
#include <util/system.h>
#include <validation.h>

#ifdef ENABLE_WALLET
#include <wallet/context.h>
#endif // ENABLE_WALLET

#include <any>

NodeContext& EnsureAnyNodeContext(const CoreContext& context)
{
auto* const node_context = GetContext<NodeContext>(context);
if (!node_context) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Node context not found");
if (node_context) {
return *node_context;
}
#ifdef ENABLE_WALLET
// We're now going to try our luck with WalletContext on the off chance
// we're being called by a wallet RPC that's trying to access NodeContext
// See comment on WalletContext::node_context for more information.
// TODO: Find a solution that removes the need for this workaround
auto* const wallet_context = GetContext<WalletContext>(context);
if (wallet_context && wallet_context->node_context) {
return *wallet_context->node_context;
}
return *node_context;
#endif // ENABLE_WALLET
throw JSONRPCError(RPC_INTERNAL_ERROR, "Node context not found");
}

CTxMemPool& EnsureMemPool(const NodeContext& node)
Expand Down
7 changes: 7 additions & 0 deletions src/wallet/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <memory>

class ArgsManager;
struct NodeContext;
namespace interfaces {
class Chain;
namespace CoinJoin {
Expand All @@ -31,6 +32,12 @@ struct WalletContext {
// TODO: replace this unique_ptr to a pointer
// probably possible to do after bitcoin/bitcoin#22219
const std::unique_ptr<interfaces::CoinJoin::Loader>& coinjoin_loader;
// Some Dash RPCs rely on WalletContext yet access NodeContext members
// even though wallet RPCs should refrain from accessing non-wallet
// capabilities (even though it is a hard ask sometimes). We should get
// rid of this at some point but until then, here's NodeContext.
// TODO: Get rid of this. It's not nice.
NodeContext* node_context{nullptr};

//! Declare default constructor and destructor that are not inline, so code
//! instantiating the WalletContext struct doesn't need to #include class
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ void WalletInit::Construct(NodeContext& node) const
LogPrintf("Wallet disabled!\n");
return;
}
auto wallet_loader = interfaces::MakeWalletLoader(*node.chain, args, node.coinjoin_loader);
auto wallet_loader = interfaces::MakeWalletLoader(*node.chain, args, node, node.coinjoin_loader);
node.wallet_loader = wallet_loader.get();
node.chain_clients.emplace_back(std::move(wallet_loader));
}
Expand Down
Loading