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

Refactor: separate wallet from node #10973

Merged
merged 4 commits into from Mar 21, 2019

Conversation

@ryanofsky
Copy link
Contributor

commented Aug 1, 2017

This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract Chain class in src/interfaces/ instead of using global variables like cs_main, chainActive, and g_connman. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

-    wtx.nTimeReceived = GetAdjustedTime();
+    wtx.nTimeReceived = m_chain->getAdjustedTime();

This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through CValidationInterface, CRPCTable, and CCoinsViewMemPool interfaces) with code that uses the Chain interface.

These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

  • Provide a single place to describe the interface between wallet and node code.
  • Can make better wallet testing possible, because the Chain object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from 92d3914 to d1f0aef Aug 2, 2017

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 2, 2017

Move some static functions out of wallet.h/cpp
This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.

This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.

Another proximate motivation is the wallet process separation work in
bitcoin#10973, where (at least initially)
parameter parsing and fee estimation are still done in the main process rather
than the wallet process, and having functions that run in different processes
scrambled up throughout wallet.cpp is unnecessarily confusing.
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

Concept ACK

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from d1f0aef to 26383cc Aug 14, 2017

@ryanofsky ryanofsky changed the title WIP: Add IPC layer between node and wallet Add IPC layer between node and wallet Aug 14, 2017

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 14, 2017

Move some static functions out of wallet.h/cpp
This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.

This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.

Another proximate motivation is the wallet process separation work in
bitcoin#10973, where (at least initially)
parameter parsing and fee estimation are still done in the main process rather
than the wallet process, and having functions that run in different processes
scrambled up throughout wallet.cpp is unnecessarily confusing.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from 26383cc to 8585b63 Aug 16, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from 8585b63 to d6e932f Aug 25, 2017

laanwj added a commit that referenced this pull request Aug 25, 2017

Merge #10976: [MOVEONLY] Move some static functions out of wallet.h/cpp
f01103c MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (Russell Yanofsky)
e7fe320 MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (Russell Yanofsky)
d97fe20 Move some static functions out of wallet.h/cpp (Russell Yanofsky)

Pull request description:

  This just moves some static wallet fee and init functions out of `wallet/wallet.cpp` and into new `wallet/fees.cpp` and `wallet/init.cpp` source files. There is one commit updating declarations and callers, followed by two MOVEONLY commits actually moving the function bodies.

  This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization.

  Another motivation is the wallet process separation work in #10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.

Tree-SHA512: 6e6982ff82b2ab4e681c043907e2b1801ceb9513394730070f16c46ad338278a863f5b3759aa13db76a259b268b1c919c81f4e339f0796a3cfb990161e8c316d

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch 2 times, most recently from 8e7fdd5 to 8fb011e Aug 25, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

@jnewbery, the change to stop deduping link arguments is in Makefile.am here

@JeremyRubin
Copy link
Contributor

left a comment

I did a superficial review of the code & the approach seems reasonable to me!

utACKs from me for the commits checked below...

  • 40afc26 Add skeleton chain and client classes
  • 5269b56 Pass chain and client variables where needed
  • 7bfb409 Remove uses of wallet functions in init.cpp
  • e26e00b Remove uses of cs_main in wallet code
  • 0a3464b Pass chain locked variables where needed
  • c119463 Remove uses of chainActive and mapBlockIndex in wallet code
  • 2dd492b Remove uses of CheckFinalTx in wallet code
  • ebb23cb Remove use of IsWitnessEnabled in wallet code
  • dd180da Remove use of AcceptToMemoryPool in wallet code
  • 83be2e6 Remove uses of GetVirtualTransactionSize in wallet code
  • 8931629 Remove use of IsRBFOptIn in wallet code
  • 684e807 Remove use of GetCountWithDescendants in wallet code
  • f3f36e8 Remove use of g_connman / PushInventory in wallet code
  • c9d049c Remove use of TransactionWithinChainLimit in wallet code
  • 04f6a9e Remove use of CalculateMemPoolAncestors in wallet code
  • c510dde Remove uses of fee globals in wallet code
  • 9849c4d Remove uses of fPruneMode in wallet code
  • ab488c8 Remove uses of g_connman in wallet code
  • 090411a Remove uses of GetAdjustedTime in wallet code
  • 64ad91d Remove uses of InitMessage/Warning/Error in wallet code
  • 3ab3335 Remove use CValidationInterface in wallet code
  • 8756810 Remove use of CRPCTable::appendCommand in wallet code
  • 35bfb7f Remove use of generateBlocks in wallet code
  • 8fb011e Remove uses of ParseConfirmTarget in wallet code
Show resolved Hide resolved src/ipc/local/bitcoind.cpp Outdated
Show resolved Hide resolved src/ipc/interfaces.h Outdated
Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/ipc/interfaces.h Outdated

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch 5 times, most recently from e0688ad to 2015123 Sep 21, 2017

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 22, 2017

fixup! Remove uses of chainActive and mapBlockIndex in wallet code
Return unset optional instead of -1 from functions trying to return heights of
blocks that might not exist.

Change suggested by Jeremy Rubin <jeremy.l.rubin@gmail.com>
bitcoin#10973 (comment)

@fanquake fanquake added this to Blockers in High-priority for review Mar 7, 2019

@promag
Copy link
Member

left a comment

Looks good, as @jnewbery points out, a bit hard to review, but I think it's worth the effort. Sorry for these nits, feel free to ignore them.

After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

Do you think this should be committed and verified in CI?

m_cache.SetBackend(m_backend);
}

private:

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

nit, align.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

nit, align.

Removed in simplification.

}

private:
bool GetCoin(const COutPoint &output, Coin &coin) const override {

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

nit, format.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

nit, format.

Removed in simplification.

@@ -160,6 +163,9 @@ class Chain
int64_t* time = nullptr,
int64_t* max_time = nullptr) = 0;

//! Get unspent outputs associated with a transaction.
virtual std::vector<Coin> findCoins(const std::vector<COutPoint>& outputs) = 0;

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

I wonder if this should return an std::vector<std::pair<COutPoint, Coin>>? Otherwise could note in the comment that result follows outputs order.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

I wonder if this should return an std::vector<std::pair<COutPoint, Coin>>? Otherwise could note in the comment that result follows outputs order.

Agree this was awkward. Replaced with in/out param in simplification.

}
CCoinsViewCache& m_cache;
const COutPoint& m_output;
Coin&& m_coin;

This comment has been minimized.

Copy link
@promag

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

In commit "Remove use of CCoinsViewMemPool::GetCoin in wallet code" (016b121)

Why?

Removed in simplification. & would have worked here too, but && seemed to make sense since this was a reference to a temporary that was moved from.

rpcfn_type pfn = pcmd->actor;
if (setDone.insert(pfn).second)
(*pfn)(jreq);
if (setDone.insert(pcmd->unique_id).second)

This comment was marked as resolved.

Copy link
@promag

promag Mar 11, 2019

Member

nit, {

This comment was marked as resolved.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

#10973 (comment)

nit, {

Will leave since I'm just replacing pfn with pcmd->unique_id here and I think it's nice that this function uses a consistent style.

}
std::vector<Coin> coins = chain.findCoins(outputs);
for (size_t i = 0; i < coins.size(); ++i) {
CoinFill(view, outputs[i], std::move(coins[i]), viewDummy);

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

Can you explain why not do the following:

CoinFill view(...);
for (...) {
   view.AccessCoin(...)
}

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

Can you explain why not do the following:

Prefer short lifetimes and less persistent state. In any case, this has been removed in a simplification.

{
auto inserted = m_rpc_forwarders.emplace(
std::piecewise_construct, std::forward_as_tuple(command.name), std::forward_as_tuple(command));
if (inserted.second && !inserted.first->second.registerForwarder()) {

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

When can inserted.second fail?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

When can inserted.second fail?

Added comment, but inserted.second being false isn't a failure. It's just checked to prevent work being repeated when there are multiple wallet processes.

{
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
t.appendCommand(commands[vcidx].name, &commands[vcidx]);
handlers.emplace_back(chain.handleRpc(commands[vcidx]));

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

handleRpc can return an "empty" handler.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

handleRpc can return an "empty" handler.

Not sure if there is a suggested change, but handleRpc will return null if CRPCTable::appendCommand returns false. There should be no change in behavior as this is.

This comment has been minimized.

Copy link
@promag

promag Mar 12, 2019

Member

But why let handlers have a null entry?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

But why let handlers have a null entry?

What is the suggestion? This condition won't happen and it wouldn't be a problem if it did happen. I'm trying not to change behavior and this is a straight translation of the previous code. Neither the old code nor the new code cares about this condition. If there are concerns about rpc registration they would probably be better to address in another PR.

return MakeUnique<HandlerImpl>(notifications);
}
void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override;

This comment has been minimized.

Copy link
@promag

promag Mar 11, 2019

Member

nit, maybe a bit late, but should be uppercase RPC?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

nit, maybe a bit late, but should be uppercase RPC?

IMO, strict camel case without exceptions for acronyms is easier to read. (JSONRPCRequest JsonRpcRequest)

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from 6898f53 to 244edfe Mar 12, 2019

@ryanofsky
Copy link
Contributor Author

left a comment

Thanks for reviews!

Updated 6898f53 -> 244edfe (pr/wipc-sep.97 -> pr/wipc-sep.98, compare).

  • Main change is adding a new commit "Remove remaining wallet accesses to node globals" (244edfe).
  • Commit "Remove use CValidationInterface in wallet code" (5c7f411, formerly fce0e3c) is simplified to remove workaround for UnregisterValidationInterface crash fixed by 2196c51 in #13743.
  • Commit "Remove use of CRPCTable::appendCommand in wallet code" (a15a7bf, formerly 53a730a) is unchanged except for a code comment.
  • Commit "Remove use of CCoinsViewMemPool::GetCoin in wallet code" (9863ddc formerly 6898f53) has been simplified by replacing a custom CCoinsView subclass with a plain std::map.

re: #10973 (review)

After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

Do you think this should be committed and verified in CI?

I'd like to verify this, but not with a script, since it would be awkward to set up and maintain. Instead, I'd like to just add a new libbitcoin_node.a library, analogous to the existing libbitcoin_wallet.a library, and move symbol definitions like cs_main pcoinsTip chainActive there that wallet and gui code shouldn't access. That way any bad references will just result in link errors while building bitcoin-wallet and bitcoin-gui executables.

Since we have src/node/ src/wallet/ and src/qt/ folders we could also set up rules preventing source files in one of these folders including headers in the other folders. This would provide the clearest separation, but would also require moving a bunch of code around, so would not be a short-term goal.

return MakeUnique<HandlerImpl>(notifications);
}
void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

nit, maybe a bit late, but should be uppercase RPC?

IMO, strict camel case without exceptions for acronyms is easier to read. (JSONRPCRequest JsonRpcRequest)

{
auto inserted = m_rpc_forwarders.emplace(
std::piecewise_construct, std::forward_as_tuple(command.name), std::forward_as_tuple(command));
if (inserted.second && !inserted.first->second.registerForwarder()) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

When can inserted.second fail?

Added comment, but inserted.second being false isn't a failure. It's just checked to prevent work being repeated when there are multiple wallet processes.

rpcfn_type pfn = pcmd->actor;
if (setDone.insert(pfn).second)
(*pfn)(jreq);
if (setDone.insert(pcmd->unique_id).second)

This comment was marked as resolved.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

#10973 (comment)

nit, {

Will leave since I'm just replacing pfn with pcmd->unique_id here and I think it's nice that this function uses a consistent style.

{
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
t.appendCommand(commands[vcidx].name, &commands[vcidx]);
handlers.emplace_back(chain.handleRpc(commands[vcidx]));

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

handleRpc can return an "empty" handler.

Not sure if there is a suggested change, but handleRpc will return null if CRPCTable::appendCommand returns false. There should be no change in behavior as this is.

@@ -160,6 +163,9 @@ class Chain
int64_t* time = nullptr,
int64_t* max_time = nullptr) = 0;

//! Get unspent outputs associated with a transaction.
virtual std::vector<Coin> findCoins(const std::vector<COutPoint>& outputs) = 0;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

I wonder if this should return an std::vector<std::pair<COutPoint, Coin>>? Otherwise could note in the comment that result follows outputs order.

Agree this was awkward. Replaced with in/out param in simplification.

m_cache.SetBackend(m_backend);
}

private:

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

nit, align.

Removed in simplification.

}

private:
bool GetCoin(const COutPoint &output, Coin &coin) const override {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

nit, format.

Removed in simplification.

}
CCoinsViewCache& m_cache;
const COutPoint& m_output;
Coin&& m_coin;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

In commit "Remove use of CCoinsViewMemPool::GetCoin in wallet code" (016b121)

Why?

Removed in simplification. & would have worked here too, but && seemed to make sense since this was a reference to a temporary that was moved from.

}
std::vector<Coin> coins = chain.findCoins(outputs);
for (size_t i = 0; i < coins.size(); ++i) {
CoinFill(view, outputs[i], std::move(coins[i]), viewDummy);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 12, 2019

Author Contributor

re: #10973 (comment)

Can you explain why not do the following:

Prefer short lifetimes and less persistent state. In any case, this has been removed in a simplification.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from 244edfe to 591434b Mar 12, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Updated 244edfe -> 591434b (pr/wipc-sep.98 -> pr/wipc-sep.99, compare) to fix travis error with old boost.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

nit: In commit 591434b

Could also update the TODO comment for GetAvailableCredit and remove the cs_main from it?

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from 591434b to b7c7821 Mar 12, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Updated 591434b -> b7c7821 (pr/wipc-sep.99 -> pr/wipc-sep.100, compare) with Marco's GetAvailableCredit suggestion from #10973 (comment)

@promag

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Instead, I'd like to just add a new libbitcoin_node.a library, analogous to the existing libbitcoin_wallet.a library, and move symbol definitions like cs_main pcoinsTip chainActive there that wallet and gui code shouldn't access.

Nice!

@jnewbery
Copy link
Member

left a comment

I've reviewed 5c7f411 (Remove use CValidationInterface in wallet code) and it looks good so far! A few comments inline.

Incidentally, this github PR is pretty unwieldly - the notes are very long, and without reading through them all it's difficult to tell which comments are for the overall approach and which are for the commits under review now. I don't know if's worth it at this point, but it might be clearer to open one final PR for the last commits, and close this or leave it as the overarching PR.

virtual ~Notifications() {}
virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {}
virtual void BlockConnected(const CBlock& block,

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 18, 2019

Member

Can you simplify this interface to (cont CBlock& block, const std::vector<CTransactionRef>& tx_conflicted)? The client can get the block hash using CBlock->GetHash().

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

Can you simplify this interface to (cont CBlock& block, const std::vector<CTransactionRef>& tx_conflicted)?

Simplified as suggested

@@ -4386,7 +4384,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
chain.loadWallet(interfaces::MakeWallet(walletInstance));

// Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 18, 2019

Member

I think this comment could be changed slightly now, since 'we' (the wallet) don't ever hold cs_main now.

since we're still holding cs_main -> since we're still holding the Chain::Lock interface.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

I think this comment could be changed slightly now, since 'we' (the wallet) don't ever hold cs_main now.

Changed this to reference locked_chain instead of cs_main (it seemed to good to reference the actual variable name).

@@ -808,6 +809,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);

/** Validation event callback handler. */
std::unique_ptr<interfaces::Handler> m_handler;

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 18, 2019

Member

I don't really like this name m_handler, (or the class name HandlerImpl), since the name handler is generic for any interface handler. It's more verbose, but I think m_validation_interface_handler and ValidationInterfaceHandlerImpl are more explicit.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

I don't really like this name m_handler, (or the class name HandlerImpl)

Renamed to m_chain_notifications_handler to match Chain::Notifications, Chain::handleNotifications, Chain::waitForNotifications.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch from b7c7821 to 1a1036a Mar 18, 2019

@ryanofsky
Copy link
Contributor Author

left a comment

Updated b7c7821 -> 1a1036a (pr/wipc-sep.100 -> pr/wipc-sep.101, compare) with suggestions from #10973 (review).

Incidentally, this github PR is pretty unwieldly

A downside of making a new PR is that comments that show up in the diffs will be gone. I think the diff comments are probably more useful than other historical comments which appear above, which I think no one will look at unless they are counting acks. (And if anyone is counting ACKs, there is only one so far from Marco in #10973 (comment) (pr/wipc-sep.97) before some recent cleanups.

virtual ~Notifications() {}
virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {}
virtual void BlockConnected(const CBlock& block,

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

Can you simplify this interface to (cont CBlock& block, const std::vector<CTransactionRef>& tx_conflicted)?

Simplified as suggested

@@ -4386,7 +4384,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
chain.loadWallet(interfaces::MakeWallet(walletInstance));

// Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

I think this comment could be changed slightly now, since 'we' (the wallet) don't ever hold cs_main now.

Changed this to reference locked_chain instead of cs_main (it seemed to good to reference the actual variable name).

@@ -808,6 +809,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);

/** Validation event callback handler. */
std::unique_ptr<interfaces::Handler> m_handler;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

I don't really like this name m_handler, (or the class name HandlerImpl)

Renamed to m_chain_notifications_handler to match Chain::Notifications, Chain::handleNotifications, Chain::waitForNotifications.

@jamesob
Copy link
Member

left a comment

utACK 1a1036a

Changes look fine to me - feel free the ignore the nit. I'll do some manual testing of this tomorrow.

@@ -1220,6 +1224,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

/** Add a KeyOriginInfo to the wallet */
bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info);

friend struct WalletTestingSetup;

This comment has been minimized.

Copy link
@jamesob

jamesob Mar 18, 2019

Member

Maybe outdated? Since m_chain_notifications_handler is public I don't see why we'd need this.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 18, 2019

Author Contributor

re: #10973 (comment)

Maybe outdated? Since m_chain_notifications_handler is public I don't see why we'd need this.

It's needed to avoid "error: cannot cast 'CWallet' to its private base class 'interfaces::Chain::Notifications'" in the WalletTestingSetup constructor:

m_wallet.m_chain_notifications_handler = m_chain->handleNotifications(m_wallet);

because the relevant base class is private:

class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notifications

@jnewbery
Copy link
Member

left a comment

I've reviewed:

  • 0ca5446 (Remove use CValidationInterface in wallet code) utACK
  • f157e7a (Remove use of CRPCTable::appendCommand in wallet code)
  • 0bafbfc (Remove use of CCoinsViewMemPool::GetCoin in wallet code) and utACK
  • 1a1036a (Remove remaining wallet accesses to node globals)

I found the Remove use of CRPCTable::appendCommand in wallet code quite hard to review, probably due to my C++ ineptitude. I certainly wouldn't want to hold up this PR because I'm not able to fully work out what's going on in that commit. I think it could be made marginally simpler by removing support for multiple clients (#10973 (comment)), but Russ points out that most of the complexity is elsewhere. I also think some additional commenting in the RpcForwarder class could help.

Russ is going to look at moving more code into the RPC server to simplify that commit, but again, I don't want to hold this up because of my inability to review. If other reviewers are happy to ACK this, then it should stay as it is.

@@ -942,7 +946,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
void ReacceptWalletTransactions();
void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void ResendWalletTransactions(int64_t nBestBlockTime) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 19, 2019

Member

Is this EXCLUSIVE_LOCKS_REQUIRED(cs_main) still required? ResendWalletTransactions() grabs a locked_chain interface.

See also the LOCKS_EXCLUDED(cs_main annotation on BlockUntilSyncedToCurrentChain() and the comment above it. Can that also go?

The BlockUntilSyncedToCurrentChain() function can definitely use some clean-up after this PR to remove the locked_chain and move isPotentialTip out of the Chain::Lock interface, but that can wait for later.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

Is this EXCLUSIVE_LOCKS_REQUIRED(cs_main) still required? ResendWalletTransactions() grabs a locked_chain interface.

It's not required and should have been removed. I replaced EXCLUSIVE_LOCKS_REQUIRED(cs_main) with a locked_chain argument.

I also added todos in https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.h#L46-L50 and https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.cpp#L208-L212 for simplifying BlockUntilSyncedToCurrentChain and removing ResendWalletTransactions.

@@ -793,20 +794,11 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 19, 2019

Member

I don't very much like that:

  • SignTransction() is taking a Chain interface as an argument
  • rpcwallet is calling into rpc/rawtransaction , which should eventually be part of libbitcoin_node (#10973 (review))

Those can both be done in a future PR.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

Those can both be done in a future PR.

Added todo (https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/rpc/rawtransaction.cpp#L794-L799) and also moved findCoins code into a global method so it will be easier to get rid of the Chain argument later.

CCoinsViewMemPool mempool_view(&chain_view, ::mempool);
for (auto& coin : coins) {
if (!mempool_view.GetCoin(coin.first, coin.second)) {
coin.second.Clear();

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 19, 2019

Member

Might be worth a comment here:

// Either the coin is not in the CCoinsViewCache or is spent. Clear it.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

// Either the coin is not in the CCoinsViewCache or is spent. Clear it.

Added comment

@ryanofsky
Copy link
Contributor Author

left a comment

Updated 1a1036a -> bffd676 (pr/wipc-sep.101 -> pr/wipc-sep.102, compare) with John's suggestions.
Updated bffd676 -> d358466 (pr/wipc-sep.102 -> pr/wipc-sep.103, compare) renaming new src/node/coin.h file to be consistent with existing src/node/transaction.h file.

I found the Remove use of CRPCTable::appendCommand in wallet code [f157e7a] quite hard to review

New version of this commit (4e4d9e9) should be a lot simpler. I was too reluctant before to change rpc server code. Should be much more straightforward now.

CCoinsViewMemPool mempool_view(&chain_view, ::mempool);
for (auto& coin : coins) {
if (!mempool_view.GetCoin(coin.first, coin.second)) {
coin.second.Clear();

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

// Either the coin is not in the CCoinsViewCache or is spent. Clear it.

Added comment


UniValue forwardRequest(const JSONRPCRequest& request) const
{
// Simple forwarding of RPC requests. This just sends the request to the

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

Supporting multiple clients makes this more complex than it needs to be.

This should be a lot simpler with commit f157e7a replaced by 4e4d9e9 (pr/wipc-sep.101 -> pr/wipc-sep.102).

Adding a new rpc server removeCommand method allowed dropping the m_rpc_forwarders map and merging the RpcHandler and RpcForwarder classes. Changing rpc server appendCommand to accept multiple commands was also not too hard and allowed getting rid of the RpcForwarder loop and m_commands vector.

@@ -793,20 +794,11 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

Those can both be done in a future PR.

Added todo (https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/rpc/rawtransaction.cpp#L794-L799) and also moved findCoins code into a global method so it will be easier to get rid of the Chain argument later.

@@ -942,7 +946,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
void ReacceptWalletTransactions();
void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void ResendWalletTransactions(int64_t nBestBlockTime) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Author Contributor

re: #10973 (comment)

Is this EXCLUSIVE_LOCKS_REQUIRED(cs_main) still required? ResendWalletTransactions() grabs a locked_chain interface.

It's not required and should have been removed. I replaced EXCLUSIVE_LOCKS_REQUIRED(cs_main) with a locked_chain argument.

I also added todos in https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.h#L46-L50 and https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.cpp#L208-L212 for simplifying BlockUntilSyncedToCurrentChain and removing ResendWalletTransactions.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wipc-sep branch 2 times, most recently from bffd676 to d358466 Mar 20, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

utACK d358466. This version is much clearer to me than the previous branch. Thank you!

I still think that handling multiple clients is an unnecessary complication for this PR. The RPC_WALLET_NOT_FOUND catch/throw in the RpcHandlerImpl actor and the last_handler bool passed to the actor seem particularly strange to me.

That said, I've reviewed the code and it all seems fine. I wouldn't want to hold this PR up because of my own aesthetic inclinations.

Very nice work Russ. Thanks for seeing this through!

@meshcollider

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

I think this is ready 🎉

@meshcollider meshcollider merged commit d358466 into bitcoin:master Mar 21, 2019

2 checks passed

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

meshcollider added a commit that referenced this pull request Mar 21, 2019

Merge #10973: Refactor: separate wallet from node
d358466 Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b23 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e6 Remove use CValidationInterface in wallet code (Russell Yanofsky)

Pull request description:

  This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract [`Chain`](https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/interfaces/chain.h) class in [`src/interfaces/`](https://github.com/ryanofsky/bitcoin/tree/pr/wipc-sep/src/interfaces) instead of using global variables like `cs_main`, `chainActive`, and `g_connman`. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the `hide-globals` script in #10244).

  This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

  ```diff
  -    wtx.nTimeReceived = GetAdjustedTime();
  +    wtx.nTimeReceived = m_chain->getAdjustedTime();
  ```

  This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through `CValidationInterface`, `CRPCTable`, and `CCoinsViewMemPool` interfaces) with code that uses the `Chain` interface.

  These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

  * Provide a single place to describe the interface between wallet and node code.
  * Can make better wallet testing possible, because the `Chain` object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

Tree-SHA512: e6291d8a3c50bdff18a9c8ad11e729beb30b5b7040d7aaf31ba678800b4a97b2dd2be76340b1e5c01fe2827d67d37ed1bb4c8380cf8ed653aadfea003e9b22e7

@jnewbery jnewbery removed this from Blockers in High-priority for review Mar 21, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

re: #10973 (review)

Instead, I'd like to just add a new libbitcoin_node.a library, analogous to the existing libbitcoin_wallet.a library, and move symbol definitions like cs_main pcoinsTip chainActive there that wallet and gui code shouldn't access

To follow up, I did this in #15638 and #15639, only using the existing libbitcoin_server.a library for simplicity instead of adding a new libbitcoin_node.a library.

domob1812 added a commit to domob1812/namecore that referenced this pull request Mar 25, 2019

Merge branch 'auxpow'
Updated the block connect/disconnect notifications changed in the upstream
bitcoin/bitcoin#10973 to track name conflicts.

Fixed the usage of sendrawtransaction with high fee rate in the Namecoin
tests to match the new meaning of the second argument.
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.