Page MenuHomePhabricator

remove walletmodel circular dependency
ClosedPublic

Authored by PiRK on Mon, Sep 28, 14:27.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCd75704e2fa7e: remove walletmodel circular dependency
Summary

This is part 2 of 4 of the backport of Core PR17513.

It includes commits a53e989 and 567cb44.

Additional changes were necessary on our side to remove the circular dependency because of codebase divergence related to BIP70 support. The PaymentServer::fetchPaymentACK method was changed to use a interfaces::Wallet& instead of a WalletModel*.

Depends on D7572

Test Plan

ninja && ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mon, Sep 28, 14:27
PiRK requested review of this revision.Mon, Sep 28, 14:27

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

PiRK retitled this revision from backport PR17513 - part 2-5 to refactor headers.Mon, Sep 28, 14:49
deadalnix requested changes to this revision.Mon, Sep 28, 15:28
deadalnix added a subscriber: deadalnix.

Can you update the circular dependency in the linter?

This revision now requires changes to proceed.Mon, Sep 28, 15:28
Fabien requested changes to this revision.Mon, Sep 28, 15:35
Fabien added a subscriber: Fabien.

This breaks the build with BIP70 disabled:
cmake -GNinja .. -DENABLE_BIP70=OFF

Output:

FAILED: src/qt/CMakeFiles/bitcoin-qt-base.dir/paymentserver.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_AC_USE_STD_ATOMIC -DBOOST_ALL_NO_LIB -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_SP_USE_STD_ATOMIC -DBOOST_THREAD_DYN_LINK -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_WIDGETS_LIB -Isrc/qt/bitcoin-qt-base_autogen/include -I../src/. -Isrc -I../src/univalue/include -Isrc/crypto/.. -I../src/secp256k1/include -I../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -isystem /usr/include/qt -isystem /usr/include/qt/QtWidgets -isystem /usr/include/qt/QtGui -isystem /usr/include/qt/QtCore -isystem /usr/lib/qt/mkspecs/linux-g++ -isystem /usr/include/qt/QtNetwork -isystem /usr/include/qt/QtDBus -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wshadow -Wredundant-decls -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -fPIC -std=gnu++14 -MD -MT src/qt/CMakeFiles/bitcoin-qt-base.dir/paymentserver.cpp.o -MF src/qt/CMakeFiles/bitcoin-qt-base.dir/paymentserver.cpp.o.d -o src/qt/CMakeFiles/bitcoin-qt-base.dir/paymentserver.cpp.o -c ../src/qt/paymentserver.cpp
Dans le fichier inclus depuis ../src/qt/paymentserver.cpp:5:
../src/./qt/paymentserver.h:154:26: erreur: « CChainParams » ne nomme pas un type
  154 |     bool handleURI(const CChainParams &params, const QString &s);
      |                          ^~~~~~~~~~~~
../src/qt/paymentserver.cpp:308:6: erreur: aucune déclaration ne correspond à « bool PaymentServer::handleURI(const CChainParams&, const QString&) »
  308 | bool PaymentServer::handleURI(const CChainParams &params, const QString &s) {
      |      ^~~~~~~~~~~~~
Dans le fichier inclus depuis ../src/qt/paymentserver.cpp:5:
../src/./qt/paymentserver.h:154:10: note: le candidat est : « bool PaymentServer::handleURI(const int&, const QString&) »
  154 |     bool handleURI(const CChainParams &params, const QString &s);
      |          ^~~~~~~~~
../src/./qt/paymentserver.h:63:7: note: « class PaymentServer » est défini ici
   63 | class PaymentServer : public QObject {
      |       ^~~~~~~~~~~~~
../src/qt/paymentserver.cpp: Dans la fonction membre « void PaymentServer::handleURIOrFile(const QString&) »:
../src/qt/paymentserver.cpp:375:25: erreur: ne peut convertir « const CChainParams » en « const int& »
  375 |     if (handleURI(Params(), s)) {
      |                   ~~~~~~^~
      |                         |
      |                         const CChainParams
Dans le fichier inclus depuis ../src/qt/paymentserver.cpp:5:
../src/./qt/paymentserver.h:154:40: note:   initialisation de l'argument 1 de « bool PaymentServer::handleURI(const int&, const QString&) »
  154 |     bool handleURI(const CChainParams &params, const QString &s);
      |                    ~~~~~~~~~~~~~~~~~~~~^~~~~~
[471/477] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/walletmodel.cpp.o
ninja: build stopped: subcommand failed.

fix broken build with ENABLE_BIP70=OFF

This breaks the build with BIP70 disabled:

I think I fixed that. Next time I will now how to test for this.

Can you update the circular dependency in the linter?

As I mentioned in the revision message, this does not get rid of the circular dependency unlike the original commit. This is because our version of paymentserver.h still uses WalletModel while Core got rid of that dependency when they dropped BIP70 support.

deadalnix requested changes to this revision.Mon, Sep 28, 16:24
In D7602#180311, @PiRK wrote:

As I mentioned in the revision message, this does not get rid of the circular dependency unlike the original commit. This is because our version of paymentserver.h still uses WalletModel while Core got rid of that dependency when they dropped BIP70 support.

What do we use it for? What does core use instead? Can we use that? If not, why not?

This revision now requires changes to proceed.Mon, Sep 28, 16:24

We use it for BIP70 paymentACK, which was dropped from the code by Core.

In D7602#180693, @PiRK wrote:

We use it for BIP70 paymentACK, which was dropped from the code by Core.

That doesn't really answer the question. Why does paymentACK need the WalletModel for? Can it find that same thing elsewhere?

remove the circular dependency using interfaces

Snippet of first build failure:

/work /work/abc-ci-builds/lint-circular-dependencies
A new circular dependency in the form of "qt/guiutil -> qt/walletmodel -> qt/paymentserver -> qt/guiutil" appears to have been introduced.

A new circular dependency in the form of "qt/guiutil -> qt/walletmodel -> qt/paymentserver -> qt/optionsmodel -> qt/guiutil" appears to have been introduced.

A new circular dependency in the form of "qt/guiutil -> qt/walletmodel -> qt/paymentserver -> qt/optionsmodel -> qt/intro -> qt/guiutil" appears to have been introduced.

/work/abc-ci-builds/lint-circular-dependencies
Build lint-circular-dependencies failed with exit code 1

This commit seems to now introduce 3 new circular dependencies, but they will all be removed in D7604. Should I amend these 2 diffs to temporarily add them to lint-circukar-dependencies.sh in D7602 and remove them again in D7604?

deadalnix requested changes to this revision.Thu, Oct 1, 10:45

Then squash them together. No need to introduce regression without any good reason to do so.

This revision now requires changes to proceed.Thu, Oct 1, 10:45

fix newly introduced circular dependencies (this makes D7604 a no-op)

Please update the summary accordingly

PiRK retitled this revision from refactor headers to remove walletmodel circular dependency.Thu, Oct 1, 11:26
PiRK edited the summary of this revision. (Show Details)

Please update the summary accordingly

Done.

This revision is now accepted and ready to land.Thu, Oct 1, 11:58