/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

build: Add --disable-bip70 configure option #11622

Open
wants to merge 2 commits into
from
+335 −168

Conversation

Reviewers

@Sjors Sjors

@luke-jr luke-jr

@promag promag

Assignees
No one assigned
Labels
Build system GUI Wallet
Projects
None yet
Milestone
No milestone
10 participants
@laanwj @TheBlueMatt @MeshCollider @promag @NicolasDorier @Sjors @luke-jr @sipa @jonasschnelli @fanquake
@laanwj
Owner

laanwj commented Nov 6, 2017

This patch adds a --disable-bip70 configure option that disables BIP70 payment request support in the wallet GUI. BIP70 support remains enabled by default.

When disabled, this breaks the dependency of the GUI on OpenSSL and Protobuf.

(triggered by discussion on IRC today)

@laanwj laanwj added Build system GUI Wallet labels Nov 6, 2017

@TheBlueMatt
Contributor

TheBlueMatt commented Nov 6, 2017

Concept ACK generally, though I'm curious where you see this going longer-term - do we want to deprecate BIP 70 support (I think I'd be generally in favor of this, it seems to provide almost 0 utility which results in it being mostly unused, and even if it were used broadly, its unclear that it provides any real security benefit) or start shipping binaries without BIP 70 support or will this just be yet another build-time option we support (which I'd definitely use)?
@MeshCollider
Member

MeshCollider commented Nov 6, 2017

Concept ACK after reading the discussion on IRC
@promag
Contributor

promag commented Nov 7, 2017

Should add to travis matrix?
@NicolasDorier
Member

NicolasDorier commented Nov 7, 2017 •
Edited 4 times

For information, I plan to make BIP70 deprecated into NBitcoin. (By removing it from main lib, and moving it to separate package)

This led me to too much dependency issues, as well as cross implementation issues as you can't check correctly the signature of the payment request without serializing the payment request exactly as all other implementations does. (Typically, my implementation works against all wallets, except copay for reasons...)

As a library/service developer, I would love to see another simple standard, filling the same need, Json simple binary based, just using HTTPS for securing the communication between wallet and server.

I remember there was discussions about making a new payment protocol long time ago, I think it was initiated by @sipa.

Concept ACK
@laanwj
Owner

laanwj commented Nov 7, 2017 •
Edited 5 times

Concept ACK generally, though I'm curious where you see this going longer-term

My opinion on it is really divided. I like BIP70 in concept (automatic refund addresses, key expiration, allowing the wallet to directly authenticate vendors, direct transaction submission), but not the technical implementation, and also not the dependency burden it puts on bitcoin core.

Additionally it also seems the code is not maintained. No one is working on BIP70 support in bitcoin core.

So I'm ok with signalling that in the future we might be going to deprecate it, without any commitments - this would be the first step, to allow builders to disable it.

(my personal motivation is that I want to have the option to build the GUI without protobuf and without OpenSSL after the last remnants of OpenSSL use are removed from the rest of the code)

For information, I plan to make BIP70 deprecated into NBitcoin. (By removing it from main lib, and moving it to separate package)

Seems to be better design anyhow. Here it's been peppered all over the GUI code :( Also for lib-ifying, isolating the parts affected like this is the first step.

Should add to travis matrix?

Agree that that's a good idea to warrant that this keeps working.
@luke-jr

luke-jr reviewed Nov 7, 2017
View changes

Concept ACK.

I suggest also indenting nested #ifs with a single space.

configure.ac
@@ -1292,6 +1313,7 @@ echo " with wallet = $enable_wallet"
echo " with gui / qt = $bitcoin_enable_qt"
if test x$bitcoin_enable_qt != xno; then
echo " qt version = $bitcoin_qt_got_major_vers"
+ echo " with bip70 = $enable_bip70"
@luke-jr

luke-jr Nov 7, 2017

Member

Probably would be better to mention "(payment protocol)" here as well.

@laanwj

laanwj Nov 7, 2017 •
Edited 1 time

Owner

Yep. Although that breaks the alignment...

src/Makefile.qt.include
@@ -161,6 +160,9 @@ QT_MOC_CPP = \
qt/moc_walletmodel.cpp \
qt/moc_walletview.cpp
+QT_MOC_CPP_BIP70 = \
@luke-jr

luke-jr Nov 7, 2017

Member

IMO would be cleaner to just QT_MOC_CPP += (and BITCOIN_QT_CPP +=) the actual files below, keeping all the stuff together.

@laanwj

laanwj Nov 7, 2017 •
Edited 1 time

Owner

I agree, but I just followed the flow already used in the makefile for the wallet, to not have to move large blocks around (which complicates review). A refactor like that could be done separately.

src/qt/bitcoin.cpp
@@ -250,8 +252,10 @@ public Q_SLOTS:
ClientModel *clientModel;
BitcoinGUI *window;
QTimer *pollShutdownTimer;
-#ifdef ENABLE_WALLET
+#if defined(ENABLE_WALLET) && defined(ENABLE_BIP70)
@luke-jr

luke-jr Nov 7, 2017

Member

Might be less messy to just leave the pointer here and not use it.

@laanwj

laanwj Nov 7, 2017

Owner

I don't think I agree. Accidentally using it would not generate a compile error in that case anymore.

@promag

promag Nov 7, 2017 •
Edited 1 time

Contributor

Can't we just allow ENABLE_BIP70 if ENABLE_WALLET is true? In that case #if defined(ENABLE_BIP70) would be enough all over the place.

@laanwj

laanwj Nov 7, 2017 •
Edited 1 time

Owner

That'd make sense (ENABLE_BIP70 implying ENABLE_WALLET).

src/qt/bitcoin.cpp
@@ -659,7 +667,7 @@ int main(int argc, char *argv[])
// Re-initialize translations after changing application name (language in network-specific settings can be different)
initTranslations(qtTranslatorBase, qtTranslator, translatorBase, translator);
-#ifdef ENABLE_WALLET
+#if defined(ENABLE_WALLET) && defined(ENABLE_BIP70)
@luke-jr

luke-jr Nov 7, 2017

Member

Won't this break opening non-BIP70 bitcoin: URIs?

@laanwj

laanwj Nov 7, 2017

Owner

Yes, as a by-effect this currently removes bitcoin: URL support too.
(it probably shouldn't)

@Sjors

Sjors Nov 9, 2017

Contributor

That would break opening links from a browser or a mail client.

src/qt/coincontroldialog.cpp
#include "coincontroldialog.h"
#include "ui_coincontroldialog.h"
#include "addresstablemodel.h"
+#include "base58.h"
@luke-jr

luke-jr Nov 7, 2017

Member

These changes seem out of place here...?

@laanwj

laanwj Nov 7, 2017 •
Edited 1 time

Owner

These extra includes are necessary now that paymentserver.h doesn't indirectly include them anymore.

src/qt/guiutil.cpp
@@ -9,6 +9,8 @@
#include "qvalidatedlineedit.h"
#include "walletmodel.h"
+#include "base58.h"
+#include "chainparams.h"
@luke-jr

luke-jr Nov 7, 2017

Member

More out of place...

src/qt/test/compattests.cpp
#include "paymentrequestplus.h" // this includes protobuf's port.h which defines its own bswap macos
+#endif
@luke-jr

luke-jr Nov 7, 2017

Member

Do we need to get bswap macros from somewhere else?

@laanwj

laanwj Nov 7, 2017

Owner

No, we have our own bswap macros. The only reason this is tested is that there was a collision between our bswap macros and protobuf's. So commenting this out is harmless.

src/qt/test/wallettests.cpp
@@ -1,5 +1,6 @@
#include "wallettests.h"
+#include "base58.h"
@luke-jr

luke-jr Nov 7, 2017

Member

Out of place

@Sjors
Contributor

Sjors commented Nov 9, 2017

"this breaks the dependency" -> "this removes the dependency"?

I know the goal is to get rid of OpenSLL, but what's with Protobuf? Just fewer dependencies, or is there a specific problem with it?

Breaking bitcoin://1A1...Na&amount=0.001 would be bad; it's used by various services. It's often embedded in a QR code, which isn't that useful on a desktop, but it can also be a link on a webpage or email too.
@laanwj
Owner

laanwj commented Nov 9, 2017 •
Edited 1 time

I know the goal is to get rid of OpenSLL, but what's with Protobuf? Just fewer dependencies, or is there a specific problem with it?

Advantage of less dependencies is mainly: less stuff to build while cross-compiling, less attack surface (yet another parsing library), etc.

Breaking bitcoin://1A1...Na&amount=0.001 would be bad; it's used by various services. It's often embedded in a QR code, which isn't that useful on a desktop, but it can also be a link on a webpage or email too.

I agree. It's --disable-bip70 not --disable-bip21. @luke-jr already commented that, though. I'll see if I can keep that part.
@laanwj
Owner

laanwj commented Nov 9, 2017

I've restored BIP21 functionality, sifting through paymentserver.cpp/h to disable the parts relating to payment requests, instead of removing the whole file from the build.
@luke-jr
Member

luke-jr commented Nov 10, 2017

But if (PaymentServer::ipcSendCommandLine()) is still #ifdef'd out...
@laanwj
Owner

laanwj commented Nov 10, 2017

But if (PaymentServer::ipcSendCommandLine()) is still #ifdef'd out...

Whoops, missed that. Fixed.
@sipa
Owner

sipa commented Nov 11, 2017

Concept ACK
@jonasschnelli
Member

jonasschnelli commented Nov 11, 2017

Nice!
Concept ACK. I could see this for 0.16 and a default to disable in 0.17.

(my personal motivation is that I want to have the option to build the GUI without protobuf and without OpenSSL after the last remnants of OpenSSL use are removed from the rest of the code)

AFAIK OpenSSL (crypto) is still in use for the PRNG seeding (see currently closed #10299, waiting for new approach).
And as far as I can see there is a RAND_event in winshutdownmonitor.cpp.
@laanwj
Owner

laanwj commented Nov 11, 2017 •
Edited 1 time

AFAIK OpenSSL (crypto) is still in use for the PRNG seeding (see currently closed #10299, waiting for new approach).

Agree. The rand_ stuff should be removed in one go in a separate PR, it's orthogonal to the changes here.
(I don't change the build system with regard to OpenSSL in this PR)
src/qt/bitcoin.cpp
@@ -647,7 +651,7 @@ int main(int argc, char *argv[])
QMessageBox::critical(0, QObject::tr(PACKAGE_NAME), QObject::tr("Error: %1").arg(e.what()));
return EXIT_FAILURE;
}
-#ifdef ENABLE_WALLET
@luke-jr

luke-jr Nov 11, 2017

Member

We need this for BIP21 too.

@Sjors
Contributor

Sjors commented Nov 13, 2017

I did a make clean, ./autogen.sh, ./configure --disable-bip70 (which shows with bip70 = no) and make deploy.

I then tested BIP-21 using: ./Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt bitcoin://1F1tAaz5x1HUXrCNLbtMDqcw6o5GNn4xqX?amount=0.1. This worked, but it did throw a warning (?):

QObject::connect: No such signal PaymentServer::receivedPaymentACK(QString) in qt/paymentserver.cpp:234

I think you need another #ifdef there.

laanwj added some commits Nov 6, 2017

@laanwj
build: Add --disable-bip70 configure option 
This patch adds a --disable-bip70 configure option that disables BIP70
payment request support. When disabled, this removes the dependency of
the GUI on OpenSSL and Protobuf.
b16ba03
@laanwj
qt: cleanup: Move BIP70 functions together in paymentserver 
Reduces the number of separate `#ifdefs` spans.
7311492
@laanwj
Owner

laanwj commented Nov 13, 2017

QObject::connect: No such signal PaymentServer::receivedPaymentACK(QString) in qt/paymentserver.cpp:234

Thanks, added, also rebased and squashed.
@Sjors
Contributor

Sjors commented Nov 13, 2017 •
Edited 1 time

Warning is gone (also without the //, see #11645 (comment)).
@fanquake
Member

fanquake commented Nov 17, 2017

Concept ACK. Needs another rebase.

@Sjors Sjors referenced this pull request Nov 18, 2017

Open

iOS Deployment Target for RPC #11720

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment