Add BIP70 deprecation warning and allow building GUI without BIP70 support

[jameshilliard]

This is based off of #11622 and adds a deprecation warning when a BIP70 URL is used.

Rational:

• BIP70 increases attack surface in multiple ways and is difficult for third party wallets to implement in a secure manner
• Very few merchants use the standard BIP70 variant supported by Bitcoin Core
• The one major payment processor that doesn't support BIP21 and currently uses a customized non-standard version of BIP70 has indicated that "Unfortunately the original BIP70 is not useful for us."

[DrahtBot]

Reviewers, this pull request conflicts with the following ones:

• Add BitcoinApplication & RPCConsole tests #11625 (Add BitcoinApplication & RPCConsole tests by ryanofsky)

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.

[fanquake]

Concept ACK

[TheBlueMatt]

Concept ACK

[fanquake]

Can drop the Qt 5 check here, master now requires > 5.0

[fanquake]

Can also remove the version check here

[meshcollider]

Concept ACK

[MarcoFalke]

I don't know enough of automake if this is possible, but could you please not link with SSL_LIBS when this flag is set? This way we'd get linker errors when new usage of the ssl libs are introduced.

[jameshilliard]

@MarcoFalke Something like this?

diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index dfc4903cf..1d70b79d4 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -419,8 +419,11 @@ endif
 if ENABLE_ZMQ
 qt_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 endif
+if ENABLE_BIP70
+qt_bitcoin_qt_LDADD += $(SSL_LIBS) $(CRYPTO_LIBS)
+endif
 qt_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) $(LIBLEVELDB_SSE42) $(LIBMEMENV) \
-  $(BOOST_LIBS) $(QT_LIBS) $(QT_DBUS_LIBS) $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
+  $(BOOST_LIBS) $(QT_LIBS) $(QT_DBUS_LIBS) $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
   $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
 qt_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
 qt_bitcoin_qt_LIBTOOLFLAGS = $(AM_LIBTOOLFLAGS) --tag CXX
diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
index db7873e8b..d8bbb2851 100644
--- a/src/Makefile.qttest.include
+++ b/src/Makefile.qttest.include
@@ -68,9 +68,12 @@ endif
 if ENABLE_ZMQ
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 endif
+if ENABLE_BIP70
+qt_test_test_bitcoin_qt_LDADD += $(SSL_LIBS) $(CRYPTO_LIBS)
+endif
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) \
   $(LIBLEVELDB_SSE42) $(LIBMEMENV) $(BOOST_LIBS) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) $(QT_LIBS) \
-  $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
+  $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
   $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
 qt_test_test_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
 qt_test_test_bitcoin_qt_CXXFLAGS = $(AM_CXXFLAGS) $(QT_PIE_FLAGS)

[MarcoFalke]

I think CRYPTO_LIBS is always needed for the rng. Otherwise ACK if it compiles and links.

[Sjors]

Concept ACK, though if we can't agree on deprecation (commit 3a093dd), let's at least merge the other commits.

I'm fine with deprecation, less fine with actually removing it until there's a good alternative (e.g. w3c payment standard). If this deprecation goes into 0.18 then I assume it won't get removed earlier than 0.19. That gives us some time to see if a better standard is proposed, and/or if merchants move away from payment processors that mandate BIP70.

I don't like the current situation where there is a lot of uncertainty around BIP70, we don't use some of its features (like enforcing a fee, and submitting tx directly to merchant), where nobody is really maintaining this part of the code and it's dependencies (e.g. ancient protobuf library), and it's getting in the way of other goals like removing OpenSSL. Deprecation gives us flexibility here.

Warning currently reads:

You are using a BIP70 URL which will be unsupported in the future.

That wording works for me. It's important to make it clear that it's just the Bitcoin Core wallet deprecating support for this feature, not some universal decision to kill BIP-70. Release notes should make that distinction extra clear.

Can someone clarify in what way BitPay deviates from the BIP? I've used it multiple times with the Core wallet. Other than an error message at the end (and broken Tor support), it composes and broadcasts the transaction just fine.

Lightly tested 3a093dd on macOS.

When running ./configure --disable-wallet it should probably disable BIP70 too, like it does for --disable-gui. The deprecation warning shouldn't show when compiled without BIP70 support.

[jameshilliard]

Can someone clarify in what way BitPay deviates from the BIP?

Technically BitPay only requires BIP70 to decode the receiving address and amount for the transaction, you don't actually have to use a BIP70 compatible wallet at all if you use an external decoder, they mostly use BIP70 as a way to try and block manual address and transaction amount entry, some of their non-standard BIP70 features are problematic in other ways(I can go over details in private if you want).

[jnewbery]

Concept ACK

The one major payment processor that doesn't support BIP21 and currently uses a customized non-standard version of BIP70 has indicated that "Unfortunately the original BIP70 is not useful for us."

Do you have a source for this?

[jameshilliard]

Do you have a source for this?

Stephen Pair(BitPay CEO) in an email exchange.

[sipa]

Concept ACK

[gmaxwell]

Concept ACK.

[jonasschnelli]

Concept ACK

[gasteve]

Can someone clarify in what way BitPay deviates from the BIP?

Technically BitPay only requires BIP70 to decode the receiving address and amount for the transaction, you don't actually have to use a BIP70 compatible wallet at all if you use an external decoder, they mostly use BIP70 as a way to try and block manual address and transaction amount entry, some of their non-standard BIP70 features are problematic in other ways(I can go over details in private if you want).

While we currently do allow a transaction to be sent only over p2p (and not posted back to the server), this is not something we want to support forever. We allow it for temporary backward compatibility (on the Bitcoin chain only) with a number of wallets that don't fully implement BIP70. I agree that bitcoind should remove BIP70.

Separately, I think consideration should be given to split out the wallet into a separate project and executable.

[luke-jr]

IMO BIP70 should be made optional before a separate PR deprecates it.

[jameshilliard]

@luke-jr this just adds a warning message and makes BIP70 optional via a configure flag it doesn't remove it in default builds

[MarcoFalke]

If the deprecation message is controversial, then maybe add it in a separate pull request?

[luke-jr]

I don't think it's controversial, just a logically separate change. My comment is NOT intended to be a "NACK" or anything like it.

[wtogami]

I asked Luke. He clarified that he does not feel deprecation would be controversial because nobody uses BIP70. He only thinks that message should be in a different PR from the build-time option. That is a reasonable statement.

[laanwj]

yes, kill it with fire, please — concept ACK

[jameshilliard]

I've made a follow up PR #14564 which disables BIP70 instead of the GUI when protobuf is missing.

@MarcoFalke Is that with bip70 enabled or disabled?

[MarcoFalke]

This is the gitian build, so the default option is used (enabled)

[MarcoFalke]

The whole build log is https://drahtbot.space/gitian/bitcoin/bitcoin/dd9eb3ae9c27467cf06259fb401510f6d9f06fd7/bitcoin-win-build.log

[jameshilliard]

Weird, I see no obvious cause of that error, is it possible there's a bad cached version of qt?

[MarcoFalke]

Indeed weird, travis should have failed because it mimics the gitian cross build with its out of tree build...

[ken2812221]

@MarcoFalke travis-ci does not build qt binaries for Windows. #13515 enable it, but it's a little bit complicate.

[NicolasDorier]

Post ACK on that, I also deprecated BIP70 in NBitcoin and removed support from BTCPay after I found out that even Bitpay broke the standard. (My BIP70 compliant parser was crashing on Bitpay's payment request)