Remove BIP70 support #17165

fanquake · 2019-10-16T20:15:38Z

This removes BIP70 support. It also removes OpenSSL linking from Qt and building OpenSSLs lib_ssl in depends, as well as SSL lib detection from the build system. It's something that I'd optimistically like to do for 0.20.0.

cvengler · 2019-10-16T20:26:06Z

Not tested yet but a grep -rnI bip70 only finds matches in release notes so it looks good to me

maflcko · 2019-10-16T21:24:20Z

Concept ACK

maflcko · 2019-10-16T21:29:37Z

Can the ssl libs be removed from the msvc build in build_msvc/README.md as well?

fanquake · 2019-10-17T00:55:24Z

Can the ssl libs be removed from the msvc build in build_msvc/README.md as well?

@sipsorcery Any chance you can take a look here, and put together some changes I can cherry-pick in?

laanwj · 2019-10-17T08:25:36Z

Concept ACK

Looks like there are still some extra SSL related symbols sneaking into the binaries for the Windows and Linux builds

That's strange! How is this possible if it doesn't link to OpenSSL? Can you list which ones?

practicalswift · 2019-10-17T11:23:20Z

Concept ACK

+22 −2,079 - very nice to see! :)

hebasto · 2019-10-17T13:33:25Z

Concept ACK

fanquake · 2019-10-17T13:55:28Z

That's strange! How is this possible if it doesn't link to OpenSSL? Can you list which ones?

@laanwj The following gists contain the output of nm -C src/qt/bitcoin-qt | grep -i ssl after doing a depends build with this PR. I have cleaned up the output somewhat (check the revision for the symbols I've trimmed). A bunch of these will also disappear once we remove the network module from the Qt build.

Windows Linux macOS

maflcko · 2019-10-17T14:53:56Z

(needs rebase for the macOS bip70 run in ./ci/)

maflcko · 2019-10-17T17:13:21Z

The following gists contain the output of nm -C src/qt/bitcoin-qt | grep -i ssl after doing a depends build with this PR

None of those symbols are in bitcoind, right?

DrahtBot · 2019-10-17T18:52:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17078 (Android depends by BlockMechanic)
#16883 (WIP: Qt: add QML based mobile GUI by icota)
#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16834 (Fetch Headers over DNS by TheBlueMatt)
#16762 (Rust-based Backup over-REST block downloader by TheBlueMatt)
#16738 (gui: Remove needless GCC diagnostic pragmas by hebasto)
#16710 (build: Enable -Wsuggest-override if available by hebasto)
#16367 (Multiprocess build support by ryanofsky)
#15768 (gui: Add close window shortcut by IPGlider)
#14066 (gitian-linux: Build binaries for 64-bit POWER by luke-jr)
#10785 (Serialization improvements by sipa)

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.

laanwj · 2019-10-18T09:43:42Z

@laanwj The following gists contain the output of nm -C src/qt/bitcoin-qt | grep -i ssl after doing a depends build with this PR.

Huh. I'm really confused. Can we be sure that Qt isn't doing an internal build of OpenSSL? (e.g. like the fallback it has for image format parsers)

Or is it gasp linking to the system OpenSSL?

A bunch of these will also disappear once we remove the network module from the Qt build.

Yeah. I guess we don't need Qt networking at all right now. Though maybe in the future, when the GUI is separate and communicates with a backend?

fanquake · 2019-10-18T17:42:43Z

None of those symbols are in bitcoind, right?

Some of them are, because we still using OpenSSL crypto in bitcoind.

Huh. I'm really confused. Can we be sure that Qt isn't doing an internal build of OpenSSL? (e.g. like the fallback it has for image format parsers)

I don't think it is. I'm also working on slimming down our OpenSSL build in a branch on top of these changes. Which should make it a lot clearer what is being built and included.

configure.ac

theuni · 2019-10-18T18:17:04Z

src/qt/optionsmodel.cpp

@@ -483,24 +483,6 @@ void OptionsModel::setDisplayUnit(const QVariant &value)
    }
 }

-bool OptionsModel::getProxySettings(QNetworkProxy& proxy) const


I guess the paymentprotocol was the only user of QNetwork* ?

theuni

Concept ACK. Only reviewed very briefly.

theuni · 2019-10-18T18:21:42Z

src/qt/paymentserver.h

 class QUrl;
 QT_END_NAMESPACE

-// BIP70 max payment request size in bytes (DoS protection)
-static const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE = 50000;
-
 class PaymentServer : public QObject


I imagine the necessary scraps can probably move out of here and we can nuke PaymentServer?

Yes, that, and removing the network module from the Qt build can all be done as followups.

src/qt/sendcoinsentry.cpp

src/qt/transactiondesc.cpp

fanquake · 2019-10-19T16:09:15Z

Rebased on master and #17191. Reverted the HAVE_CONFIG_H deletions and swapped to throwing an error on --enable-bip70 rather than dropping the check entirely as suggested by theuni.

Also re-ordered the commits so the removal steps are a bit more logical and added another change so that we are now only building OpenSSLs lib_crypto (no longer building lib_ssl).

fanquake · 2020-05-23T07:28:24Z

This has a release note in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft.

ea9fcfd doc: Drop protobuf stuff (Hennadii Stepanov) Pull request description: This is a follow-up to #17165. ACKs for top commit: fanquake: ACK ea9fcfd - clicked the links and they seem to work. Tree-SHA512: 0861bbac3a3ff781a413e15f5ed02c624bc15d572a001a53cd2fb9f7683456175f69e9d666b72f260abbb5114b67cefca9fada4d179c62384c90479534ae63d5

This was left in after bitcoin#17165, so that anyone who had been compiling with (already disabled by default) BIP70 would realise that support had been completely removed in 0.20.0. However we should be able to remove it for 0.21.0.

c4ffcf0 build: remove BIP70 configure option (fanquake) Pull request description: This was left in after #17165, so that anyone who had been compiling with (already disabled by default) BIP70 would realise that support had been completely removed in 0.20.0. However we should be able to remove it for 0.21.0. ACKs for top commit: jnewbery: utACK c4ffcf0 MarcoFalke: ACK c4ffcf0 with or without the "catch-all reject" Tree-SHA512: a5dd4231ed97c9dd1984fb90d69a8725df2fdda0b963269b0575601c74528e5d820a4a863c428f8ede86eaae2a1606671fe1fcebdeb96b1023f7a5f899270284

c4ffcf0 build: remove BIP70 configure option (fanquake) Pull request description: This was left in after bitcoin#17165, so that anyone who had been compiling with (already disabled by default) BIP70 would realise that support had been completely removed in 0.20.0. However we should be able to remove it for 0.21.0. ACKs for top commit: jnewbery: utACK c4ffcf0 MarcoFalke: ACK c4ffcf0 with or without the "catch-all reject" Tree-SHA512: a5dd4231ed97c9dd1984fb90d69a8725df2fdda0b963269b0575601c74528e5d820a4a863c428f8ede86eaae2a1606671fe1fcebdeb96b1023f7a5f899270284

Remove OpenSSL Includes changes cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7095 - bitcoin/bitcoin#17165 - Only the commit removing SSL lib detection (we have long since removed the rest). - bitcoin/bitcoin#17265 - We had already migrated away from OpenSSL for randomness. - bitcoin/bitcoin#17515 - Only the second commit.

Remove OpenSSL Includes changes cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7095 - bitcoin/bitcoin#11024 - bitcoin/bitcoin#17165 - Only the commit removing SSL lib detection (we have long since removed the rest). - bitcoin/bitcoin#17265 - We had already migrated away from OpenSSL for randomness. - bitcoin/bitcoin#17515 - Only the second commit. Closes #145.

…SL linking 162d003 doc: compiling with Visual Studio is now supported on Windows (fanquake) b1f1fb5 doc: update MSVC instructions to remove Qt configuration (fanquake) Pull request description: Follow up from bitcoin#17165. Flips `-openssl-linked` to `-no-openssl`. Also adds some missing packages to the vcpkg install instructions. ACKs for top commit: sipsorcery: tACK 162d003. Tree-SHA512: 40577a3759a30170a14fd27e3eeac5a78a7852ae88daacecf584ad46c685708c167153d39357aa77a4f65bfd5a349f7589f20aa16fdf3d2895b4076b381e2c9c

3ed8e3d doc: Remove explicit network name references (Fabian Jahr) d6e493f wallet: Remove left-over BIP70 comment (Fabian Jahr) Pull request description: A small follow-up to bitcoin#17165 which removed BIP70 support. 1. Removes one leftover mention of BIP70 in a comment. 2. Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway. If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to `CBaseChainParams`. That would also mean we don't have to change these lines again for signet. ACKs for top commit: MarcoFalke: ACK 3ed8e3d Tree-SHA512: 9a7c0b9cacbb67bd31a089ffdc6f1ebc7f336493e2c8266eb697da34dce2b505a431d5639a3e4fc34f9287361343e861b55dc2662e0a1d2095cc1046db77d6ee

…aster fa7f5a4 doc: Update doc/bips.md with recent changes in master (MarcoFalke) Pull request description: Follow-up to bitcoin#17165 ACKs for top commit: jonatack: ACK fa7f5a4. Verified markdown view at https://github.com/MarcoFalke/bitcoin-core/blob/1911-docBips/doc/bips.md and the urls in the links. Some of the PRs are indicated with # and some without, but this is the case over the whole document. laanwj: ACK fa7f5a4 fanquake: ACK fa7f5a4 Tree-SHA512: 31782b5f1f2f10b1189f05f010f908c183dbe723477ca1c46ad1d3bee5ea483335847008a7fe48d72373ccd39b84e0b950d0d1b23e457cb70f34210c5f2dc6aa

e5a0bec doc: add OpenSSL removal to release-notes.md (fanquake) 397dbae ci: remove OpenSSL installation (fanquake) a4eb839 doc: remove OpenSSL from build instructions and licensing info (fanquake) 648b2e3 depends: remove OpenSSL package (fanquake) 8983ee3 build: remove OpenSSL detection and libs (fanquake) b49b6b0 random: Remove remaining OpenSSL calls and locking infrastructure (fanquake) 4fcfcc2 random: stop retrieving random bytes from OpenSSL (fanquake) 5624ab0 random: stop feeding RNG output back into OpenSSL (fanquake) Pull request description: Now that bitcoin#17165 has been merged, removing our remaining OpenSSL usage is possible. That remaining usage was a call to [`RAND_bytes`](https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html) during the ::SLOW path of [ProcRand](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L616). As well as feeding output from our RNG back into OpenSSL via [`RAND_add`](https://www.openssl.org/docs/manmaster/man3/RAND_add.html) during the ::SLOW and ::SLEEP paths. Optimistically tagged for `0.20.0`. Needs discussion, potentially in an upcoming weekly meeting? Closes bitcoin#12530. ACKs for top commit: MarcoFalke: ACK e5a0bec laanwj: ACK e5a0bec Tree-SHA512: 02fce08ec91d20e0da51e9314eec53dcf8699cded02f0a005417d627520c20b826332cb42bdae132af283d4903aa3088a9f613f3aea915d655a51532a4d4796c

244501f depends: disable unused qt networking features (fanquake) 29d56c6 depends: -optimized-qmake is now -optimized-tools (fanquake) ccdda96 depends: skip building qt proxies (fanquake) Pull request description: Somewhat of a followup to removing BIP70 support in bitcoin#17165. This removes networking features from our Qt build. This also removes the need to link against the `CFNetwork` and `SystemConfiguration` libraries on macOS. ```diff src/qt/bitcoin-qt: /usr/lib/libSystem.B.dylib /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation -/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon /usr/lib/libc++.1.dylib -/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO /usr/lib/libobjc.A.dylib ``` > Introduced the -optimized-tools option; supersedes -optimized-qmake. `optimized-qmake` became `optimized-tools` in Qt 5.6.0. While the former still works, we can use the newer flag. A diff of the removed symbols is available [here](https://gist.github.com/fanquake/9c8d5961c91f90a2966191367adfb391). We still need to actually build the network module, because we are using `QLocalServer` & `QLocalSocket` in the payment server. ACKs for top commit: Sjors: Code review ACK 244501f: just a rebase (_updated since I accidentally repeated the previous hash_) practicalswift: ACK 244501f -- diff looks correct promag: Code review ACK 244501f. Tree-SHA512: 79734e3c96c40e7e484c86ac4cd4f738c05fcebe4771aeac443883f618a6c766e667909d5f8f14f9bd82f43206387c952458c5fa765cd0830f8beda6e6ac80ae

6e25d83 Remove remaining references to BIP70 (Fuzzbawls) bcd3ee0 compat: remove bswap_* check on macOS (fanquake) 317f314 build: skip building OpenSSL lib_ssl (fanquake) 752a337 build: remove OpenSSL from Qt build (fanquake) dd471de build: remove SSL lib detection (fanquake) dc843fb build: remove BIP70 entries from macOS Info.plist (Fuzzbawls) 4dc8924 GUI: remove payment request file handling from OpenURI dialog (Fuzzbawls) 1862665 Remove BIP70 support (Fuzzbawls) b047918 docs: remove protobuf from docs (Fuzzbawls) ae47613 build: remove protobuf from depends (Fuzzbawls) Pull request description: Following with upstream's bitcoin#17165, this removes the support for the problematic BIP70, which we weren't fully supporting anyways. This removes the protobuf dependency completely, as well as removes the GUI's dependency on libssl ACKs for top commit: random-zebra: utACK 6e25d83 after rebase furszy: build tested, ACK 6e25d83 and merging.. Tree-SHA512: f3699104e080ed9ea4087b59467e9823d95be6be0b16af930c886b0436ec61eb62725673654f325d0d2b6a61c6795aa84c37f0b1a7f1106f5204ea0f0aa7f2b2

ea9fcfd doc: Drop protobuf stuff (Hennadii Stepanov) Pull request description: This is a follow-up to bitcoin#17165. ACKs for top commit: fanquake: ACK ea9fcfd - clicked the links and they seem to work. Tree-SHA512: 0861bbac3a3ff781a413e15f5ed02c624bc15d572a001a53cd2fb9f7683456175f69e9d666b72f260abbb5114b67cefca9fada4d179c62384c90479534ae63d5

fanquake added GUI Build system Needs gitian build Needs Conceptual Review labels Oct 16, 2019

fanquake force-pushed the remove_bip70 branch from 9b39003 to f1fb1d9 Compare October 17, 2019 15:04

fanquake removed the Needs Conceptual Review label Oct 17, 2019

fanquake added this to the 0.20.0 milestone Oct 17, 2019

DrahtBot removed the Needs gitian build label Oct 18, 2019

fanquake force-pushed the remove_bip70 branch from 39b9994 to 26f58e0 Compare October 18, 2019 17:46

theuni reviewed Oct 18, 2019

View reviewed changes

configure.ac Show resolved Hide resolved

theuni reviewed Oct 18, 2019

View reviewed changes

laanwj mentioned this pull request Oct 19, 2019

random: remove call to RAND_screen() (Windows only) #17191

Merged

fanquake force-pushed the remove_bip70 branch from 26f58e0 to 5cd598f Compare October 19, 2019 16:07

fanquake force-pushed the remove_bip70 branch 2 times, most recently from 1d922d4 to 6bd4a50 Compare October 21, 2019 14:28

10xcryptodev mentioned this pull request May 13, 2020

Paying to a BIP70 invoice shouldn't throw an error dashpay/dash#3481

Closed

hebasto mentioned this pull request May 23, 2020

doc: Drop protobuf stuff #19058

Merged

fanquake removed the Needs release note label May 23, 2020

fanquake mentioned this pull request Jun 12, 2020

build: remove BIP70 configure option #19257

Merged

str4d mentioned this pull request Sep 23, 2020

Remove OpenSSL zcash/zcash#4740

Merged

kwvg mentioned this pull request Mar 2, 2021

merge #17165: Remove BIP70 support dashpay/dash#4023

Merged

hebasto mentioned this pull request Mar 26, 2021

test: remove qt byteswap compattests #21531

Merged

Fuzzbawls mentioned this pull request Apr 2, 2021

[GUI] Remove BIP70 PIVX-Project/PIVX#2286

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Remove BIP70 support #17165

Remove BIP70 support #17165

Uh oh!

Conversation

fanquake commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvengler commented Oct 16, 2019

Uh oh!

maflcko commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 16, 2019

Uh oh!

fanquake commented Oct 17, 2019

Uh oh!

laanwj commented Oct 17, 2019

Uh oh!

practicalswift commented Oct 17, 2019

Uh oh!

hebasto commented Oct 17, 2019

Uh oh!

fanquake commented Oct 17, 2019

Uh oh!

maflcko commented Oct 17, 2019

Uh oh!

maflcko commented Oct 17, 2019

Uh oh!

DrahtBot commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 18, 2019

Uh oh!

Uh oh!

theuni Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

theuni Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

fanquake Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fanquake commented Oct 19, 2019

Uh oh!

fanquake commented May 23, 2020

Uh oh!

Uh oh!

fanquake commented Oct 16, 2019 •

edited

Loading

maflcko commented Oct 16, 2019 •

edited

Loading

DrahtBot commented Oct 17, 2019 •

edited

Loading

laanwj commented Oct 18, 2019 •

edited

Loading

theuni Oct 18, 2019 •

edited

Loading