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

Remove BIP70 support #17165

Merged
merged 11 commits into from Oct 26, 2019
Merged

Remove BIP70 support #17165

merged 11 commits into from Oct 26, 2019

Conversation

@fanquake
Copy link
Member

fanquake commented Oct 16, 2019

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.

@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Oct 16, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 16, 2019

Concept ACK

444 gif

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 16, 2019

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

@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Oct 17, 2019

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

This comment has been minimized.

Copy link
Member

laanwj commented Oct 17, 2019

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

This comment has been minimized.

Copy link
Member

practicalswift commented Oct 17, 2019

Concept ACK

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

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Oct 17, 2019

Concept ACK

@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Oct 17, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 17, 2019

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

@fanquake fanquake force-pushed the fanquake:remove_bip70 branch from 9b39003 to f1fb1d9 Oct 17, 2019
@fanquake fanquake added this to the 0.20.0 milestone Oct 17, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 17, 2019

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

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 17, 2019

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

This comment has been minimized.

Copy link
Member

laanwj commented Oct 18, 2019

@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

This comment has been minimized.

Copy link
Member Author

fanquake commented Oct 18, 2019

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.

@fanquake fanquake force-pushed the fanquake:remove_bip70 branch from 39b9994 to 26f58e0 Oct 18, 2019
configure.ac Show resolved Hide resolved
@@ -483,24 +483,6 @@ void OptionsModel::setDisplayUnit(const QVariant &value)
}
}

bool OptionsModel::getProxySettings(QNetworkProxy& proxy) const

This comment has been minimized.

Copy link
@theuni

theuni Oct 18, 2019

Member

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

Copy link
Member

theuni left a comment

Concept ACK. Only reviewed very briefly.

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

This comment has been minimized.

Copy link
@theuni

theuni Oct 18, 2019

Member

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

This comment has been minimized.

Copy link
@fanquake

fanquake Oct 21, 2019

Author Member

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

src/qt/sendcoinsentry.cpp Outdated Show resolved Hide resolved
src/qt/transactiondesc.cpp Outdated Show resolved Hide resolved
@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Oct 19, 2019

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 fanquake force-pushed the fanquake:remove_bip70 branch 2 times, most recently from 1d922d4 to 6bd4a50 Oct 19, 2019
fanquake added 6 commits Oct 16, 2019
This was originally added in #9366 to fix the gui build, as
Protobuf would also define these macros. Now that we're no-longer
using Protobuf, remove the additional check.
@fanquake fanquake force-pushed the fanquake:remove_bip70 branch from 609042a to 8c6081a Oct 24, 2019
@fanquake fanquake removed the Needs rebase label Oct 24, 2019
@fjahr
fjahr approved these changes Oct 25, 2019
Copy link
Contributor

fjahr left a comment

ACK 8c6081a

Reviewed code, built and ran tests. Two nits but this works.

#ifdef ENABLE_BIP70
// If from a payment request, paymentRequest.IsInitialized() will be true
PaymentRequestPlus paymentRequest;
#else
// If building with BIP70 is disabled, keep the payment request around as

This comment has been minimized.

Copy link
@fjahr

fjahr Oct 25, 2019

Contributor

BIP70 reference should be removed from comment

@@ -48,7 +48,6 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTx& wtx, const i
}
}

#ifndef ENABLE_BIP70
// Takes an encoded PaymentRequest as a string and tries to find the Common Name of the X.509 certificate
// used to sign the PaymentRequest.
bool GetPaymentRequestMerchant(const std::string& pr, QString& merchant)

This comment has been minimized.

Copy link
@fjahr

fjahr Oct 25, 2019

Contributor

Could this method not rather be removed as well, along with the one place where it is used (line 295, same file)? I guess you try to keep the PR small but I would probably still remove this.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 25, 2019

Member

I think this is still needed to display the merchant name for existing txs made with bip 70?

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 26, 2019

Member

It's not the intent to remove this any time soon. Maybe in the far future on a wallet format change. This was added as fallback for removing of BIP70 in #16852, so that displaying of old transactions doesn't suffer.

This comment has been minimized.

Copy link
@fjahr

fjahr Oct 26, 2019

Contributor

Ok, thanks for clarifying!

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 25, 2019

ACK 8c6081a

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 8c6081a884cd0969160955ce8687d4d4ed074db3
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjKSgv/exTwf/JSsLIdKgCml6kmBoF0kadbbWndiUR7azT5wlTQcWV/+c9McOxO
clD0f1Sci5Fks1Dy9N5SIEoVpjh29CYSInYnbu3rQTagb4+DYi1kVtVgp0j7E1lD
eHxTD4VJIbz1rdnseGs6xJA5KU8bkZekDrwIO4u7qRZWP4ikK2dwwf96MoMt42KI
x7w6apQaSkyBjAxTVa665xOpV+PugO0CcdbMudVq3v8awC1ITrcRUH3Fp2egnuJn
ithoI7FONpQH9+ipFxQ3KO0shJWYk45nw0EMATjBZO+Amhzif1gTJacbeozC6sZs
u4jplY3sHH2pYlPdIhs3FAaHO1Cia/lwfV+k2GvXD/2Ym5qGX63ZhPYo483wghtO
3av1EsgdHdxx62TWgQChPi/5lC0DU/h5M+vnHQYE/bhzvViUrqPzyODG1NoBhKIE
JoX5SWQE7XmvCrsjhbMS/91YevGUe61AFjUwx4ITyr3zh8VGH43YOc9BZRDOyvSC
ZUO0vK0l
=K4mK
-----END PGP SIGNATURE-----

Timestamp of file with hash 962930a6f6f662ed694e7153800aef1dfae989ffe2d302c9f2c6c70952527e09 -

laanwj added a commit that referenced this pull request Oct 26, 2019
8c6081a compat: remove bswap_* check on macOS (fanquake)
2cba35a build: skip building OpenSSL lib_ssl (fanquake)
45a2d3c build: remove OpenSSL from Qt build (fanquake)
befbc40 build: remove EVP_MD_CTX_new detection (fanquake)
fcee10c build: remove SSL lib detection (fanquake)
c7f30db gui: Update BIP70 support message (fanquake)
a3e8103 build: remove BIP70 entries from macOS Info.plist (fanquake)
72fe13a gui: remove payment request file handling from OpenURI dialog (fanquake)
3548e4a Remove BIP70 Support (fanquake)
1cb9a4e docs: remove protobuf from docs (fanquake)
67328bb build: remove protobuf from depends (fanquake)

Pull request description:

  This removes [BIP70](https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki) 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`.

ACKs for top commit:
  laanwj:
    Code review ACK 8c6081a
  MarcoFalke:
    ACK 8c6081a
  fjahr:
    ACK 8c6081a

Tree-SHA512: 9dd9153afa4eca1a795f983e5b31f5fee9fa9a064c2a95d2f98810689add3ad0bf221c4608282299e66e4d1ec31cd556d4b16eea55de7912c3b9931f64735883
@laanwj laanwj merged commit 8c6081a into bitcoin:master Oct 26, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake deleted the fanquake:remove_bip70 branch Oct 26, 2019
@fanquake fanquake removed this from Blockers in High-priority for review Oct 26, 2019
@fanquake fanquake mentioned this pull request Oct 26, 2019
MarcoFalke added a commit that referenced this pull request Nov 1, 2019
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 #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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 2, 2019
…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
laanwj added a commit that referenced this pull request Nov 2, 2019
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 #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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 3, 2019
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
fanquake added a commit that referenced this pull request Nov 6, 2019
fa7f5a4 doc: Update doc/bips.md with recent changes in master (MarcoFalke)

Pull request description:

  Follow-up to #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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 7, 2019
…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
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019
laanwj added a commit that referenced this pull request Nov 19, 2019
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 #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 #12530.

ACKs for top commit:
  MarcoFalke:
    ACK e5a0bec
  laanwj:
    ACK e5a0bec

Tree-SHA512: 02fce08ec91d20e0da51e9314eec53dcf8699cded02f0a005417d627520c20b826332cb42bdae132af283d4903aa3088a9f613f3aea915d655a51532a4d4796c
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 19, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.