Remove BIP70 support #17165

pull fanquake wants to merge 11 commits into bitcoin:master from fanquake:remove_bip70 changing 48 files +38 −2114
  1. fanquake commented at 8:15 pm on October 16, 2019: member
    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.
  2. fanquake added the label GUI on Oct 16, 2019
  3. fanquake added the label Build system on Oct 16, 2019
  4. fanquake added the label Needs gitian build on Oct 16, 2019
  5. fanquake added the label Needs Conceptual Review on Oct 16, 2019
  6. emilengler commented at 8:26 pm on October 16, 2019: contributor
    Not tested yet but a grep -rnI bip70 only finds matches in release notes so it looks good to me
  7. MarcoFalke commented at 9:24 pm on October 16, 2019: member

    Concept ACK

    444 gif

  8. MarcoFalke commented at 9:29 pm on October 16, 2019: member
    Can the ssl libs be removed from the msvc build in build_msvc/README.md as well?
  9. fanquake commented at 0:55 am on October 17, 2019: member

    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?

  10. laanwj commented at 8:25 am on October 17, 2019: member

    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?

  11. practicalswift commented at 11:23 am on October 17, 2019: contributor

    Concept ACK

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

  12. hebasto commented at 1:33 pm on October 17, 2019: member
    Concept ACK
  13. fanquake commented at 1:55 pm on October 17, 2019: member

    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

  14. MarcoFalke commented at 2:53 pm on October 17, 2019: member
    (needs rebase for the macOS bip70 run in ./ci/)
  15. fanquake force-pushed on Oct 17, 2019
  16. fanquake removed the label Needs Conceptual Review on Oct 17, 2019
  17. fanquake added this to the milestone 0.20.0 on Oct 17, 2019
  18. MarcoFalke commented at 5:13 pm on October 17, 2019: member

    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?

  19. DrahtBot commented at 6:52 pm on October 17, 2019: member

    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.

  20. laanwj commented at 9:43 am on October 18, 2019: member

    @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?

  21. DrahtBot removed the label Needs gitian build on Oct 18, 2019
  22. fanquake commented at 5:42 pm on October 18, 2019: member

    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.

  23. fanquake force-pushed on Oct 18, 2019
  24. in configure.ac:229 in 87a81ddc31 outdated 
    225@@ -226,13 +226,6 @@ AC_ARG_ENABLE([zmq],
226   [disable ZMQ notifications])],
227   [use_zmq=$enableval],
228   [use_zmq=yes])
229-AC_ARG_ENABLE([bip70],
    theuni commented at 6:11 pm on October 18, 2019:
    I think we probably want to leave this here, but turn --enable-bip70 into an error. That way it’s obvious to those who are currently manually overriding.
    fanquake commented at 4:09 pm on October 19, 2019:
    Have reverted this removal, and now throw an error for --enable-bip70.
  25. in src/qt/optionsmodel.cpp:486 in 87a81ddc31 outdated 
    482@@ -483,24 +483,6 @@ void OptionsModel::setDisplayUnit(const QVariant &value)
483     }
484 }
485 
486-bool OptionsModel::getProxySettings(QNetworkProxy& proxy) const
    theuni commented at 6:17 pm on October 18, 2019:
    I guess the paymentprotocol was the only user of QNetwork* ?
  26. in src/qt/paymentserver.h:53 in 87a81ddc31 outdated 
    50 QT_END_NAMESPACE
51 
52-// BIP70 max payment request size in bytes (DoS protection)
53-static const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE = 50000;
54-
55 class PaymentServer : public QObject
    theuni commented at 6:21 pm on October 18, 2019:
    I imagine the necessary scraps can probably move out of here and we can nuke PaymentServer?
    fanquake commented at 2:30 pm on October 21, 2019:
    Yes, that, and removing the network module from the Qt build can all be done as followups.
  27. in src/qt/sendcoinsentry.cpp:5 in 87a81ddc31 outdated 
    1@@ -2,10 +2,6 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5-#if defined(HAVE_CONFIG_H)
    theuni commented at 6:22 pm on October 18, 2019:
    Please leave this in case someone uses another define down the road.
  28. in src/qt/transactiondesc.cpp:5 in 87a81ddc31 outdated 
    1@@ -2,10 +2,6 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5-#ifdef HAVE_CONFIG_H
    theuni commented at 6:23 pm on October 18, 2019:
    Same, please leave these.
  29. theuni commented at 6:29 pm on October 18, 2019: member
    Concept ACK. Only reviewed very briefly.
  30. fanquake force-pushed on Oct 19, 2019
  31. fanquake commented at 4:09 pm on October 19, 2019: member

    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).

  32. fanquake force-pushed on Oct 19, 2019
  33. fanquake force-pushed on Oct 21, 2019
  34. fanquake renamed this:
    [WIP] Remove BIP70 support
    Remove BIP70 support
    on Oct 21, 2019
  35. fanquake marked this as ready for review on Oct 21, 2019
  36. fanquake force-pushed on Oct 21, 2019
  37. laanwj added the label Needs gitian build on Oct 22, 2019
  38. laanwj commented at 11:33 am on October 22, 2019: member
    Code review ACK 609042a6bbd6a4526cd9713a221f827035db9935 Code review ACK 8c6081a884cd0969160955ce8687d4d4ed074db3
  39. MarcoFalke deleted a comment on Oct 22, 2019
  40. DrahtBot commented at 6:14 am on October 23, 2019: member

    Gitian builds for commit a22b62481aae95747830bd3c0db3227860b12d8e (master):

    Gitian builds for commit 75a3c6727ff2dcc6bf542030fa9b0ee94233e605 (master and this pull):

  41. DrahtBot removed the label Needs gitian build on Oct 23, 2019
  42. laanwj commented at 8:38 am on October 23, 2019: member
    Good, this time the OSX binaries are included in the gitian build.
  43. laanwj added this to the "Blockers" column in a project
  44. jtimon commented at 7:23 pm on October 24, 2019: contributor
    Oh, wow, I hadn’t seen this. Yes, please. Concept ACK
  45. DrahtBot commented at 8:00 pm on October 24, 2019: member
  46. DrahtBot added the label Needs rebase on Oct 24, 2019
  47. build: remove protobuf from depends 67328bb7ca
  48. docs: remove protobuf from docs 1cb9a4e28c
  49. Remove BIP70 Support 3548e4aac7
  50. gui: remove payment request file handling from OpenURI dialog 72fe13a58d
  51. build: remove BIP70 entries from macOS Info.plist a3e810326d
  52. gui: Update BIP70 support message c7f30dbca8
  53. build: remove SSL lib detection fcee10c2d0
  54. build: remove EVP_MD_CTX_new detection 
    This was added in #9475 to fix LibreSSL compatibility for
BIP70, so is no longer required.
    befbc40eb5
  55. build: remove OpenSSL from Qt build 
    More info available from:
https://doc.qt.io/qt-5/ssl.html#enabling-and-disabling-ssl-support
    45a2d3c552
  56. build: skip building OpenSSL lib_ssl 2cba35ab38
  57. compat: remove bswap_* check on macOS 
    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.
    8c6081a884
  58. fanquake force-pushed on Oct 24, 2019
  59. fanquake removed the label Needs rebase on Oct 24, 2019
  60. in src/qt/walletmodel.h:67 in 8c6081a884 
    63@@ -67,15 +64,9 @@ class SendCoinsRecipient
64     CAmount amount;
65     // If from a payment request, this is used for storing the memo
66     QString message;
67-
68-#ifdef ENABLE_BIP70
69-    // If from a payment request, paymentRequest.IsInitialized() will be true
70-    PaymentRequestPlus paymentRequest;
71-#else
72     // If building with BIP70 is disabled, keep the payment request around as
    fjahr commented at 4:25 pm on October 25, 2019:
    BIP70 reference should be removed from comment
  61. in src/qt/transactiondesc.cpp:53 in 8c6081a884 
    47@@ -48,7 +48,6 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTx& wtx, const i
48     }
49 }
50 
51-#ifndef ENABLE_BIP70
52 // Takes an encoded PaymentRequest as a string and tries to find the Common Name of the X.509 certificate
53 // used to sign the PaymentRequest.
54 bool GetPaymentRequestMerchant(const std::string& pr, QString& merchant)
    fjahr commented at 5:20 pm on October 25, 2019:
    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.
    MarcoFalke commented at 9:02 pm on October 25, 2019:
    I think this is still needed to display the merchant name for existing txs made with bip 70?
    laanwj commented at 9:29 am on October 26, 2019:
    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.
    fjahr commented at 11:36 am on October 26, 2019:
    Ok, thanks for clarifying!
  62. fjahr approved
  63. fjahr commented at 5:24 pm on October 25, 2019: member

    ACK 8c6081a

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

  64. MarcoFalke commented at 9:00 pm on October 25, 2019: member

    ACK 8c6081a884cd0969160955ce8687d4d4ed074db3

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 8c6081a884cd0969160955ce8687d4d4ed074db3
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjKSgv/exTwf/JSsLIdKgCml6kmBoF0kadbbWndiUR7azT5wlTQcWV/+c9McOxO
 8clD0f1Sci5Fks1Dy9N5SIEoVpjh29CYSInYnbu3rQTagb4+DYi1kVtVgp0j7E1lD
 9eHxTD4VJIbz1rdnseGs6xJA5KU8bkZekDrwIO4u7qRZWP4ikK2dwwf96MoMt42KI
10x7w6apQaSkyBjAxTVa665xOpV+PugO0CcdbMudVq3v8awC1ITrcRUH3Fp2egnuJn
11ithoI7FONpQH9+ipFxQ3KO0shJWYk45nw0EMATjBZO+Amhzif1gTJacbeozC6sZs
12u4jplY3sHH2pYlPdIhs3FAaHO1Cia/lwfV+k2GvXD/2Ym5qGX63ZhPYo483wghtO
133av1EsgdHdxx62TWgQChPi/5lC0DU/h5M+vnHQYE/bhzvViUrqPzyODG1NoBhKIE
14JoX5SWQE7XmvCrsjhbMS/91YevGUe61AFjUwx4ITyr3zh8VGH43YOc9BZRDOyvSC
15ZUO0vK0l
16=K4mK
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 962930a6f6f662ed694e7153800aef1dfae989ffe2d302c9f2c6c70952527e09 -

  65. laanwj referenced this in commit d91af4768c on Oct 26, 2019
  66. laanwj merged this on Oct 26, 2019
  67. laanwj closed this on Oct 26, 2019
  68. fanquake deleted the branch on Oct 26, 2019
  69. fanquake removed this from the "Blockers" column in a project
  70. fanquake added the label Needs release note on Oct 26, 2019
  71. MarcoFalke referenced this in commit c737839471 on Nov 1, 2019
  72. sidhujag referenced this in commit a1cc49265c on Nov 2, 2019
  73. laanwj referenced this in commit 463eab5e14 on Nov 2, 2019
  74. sidhujag referenced this in commit 902791dd2a on Nov 3, 2019
  75. fanquake referenced this in commit 4e21f72980 on Nov 6, 2019
  76. sidhujag referenced this in commit f5be3f374b on Nov 7, 2019
  77. MarkLTZ referenced this in commit 18a6d41cb0 on Nov 17, 2019
  78. laanwj referenced this in commit 2065ef66ee on Nov 19, 2019
  79. sidhujag referenced this in commit 517bf47683 on Nov 19, 2019
  80. laanwj referenced this in commit 5e4912f990 on Dec 16, 2019
  81. sidhujag referenced this in commit d2ef00b117 on Dec 16, 2019
  82. fanquake removed the label Needs release note on May 23, 2020
  83. fanquake commented at 7:28 am on May 23, 2020: member
    This has a release note in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft.
  84. fanquake referenced this in commit 492cdc56e0 on May 23, 2020
  85. fanquake referenced this in commit c4ffcf07af on Jun 12, 2020
  86. laanwj referenced this in commit 9d92ee12fd on Jul 1, 2020
  87. sidhujag referenced this in commit 2790fd2b3b on Jul 8, 2020
  88. zkbot referenced this in commit 5bea6d806f on Sep 23, 2020
  89. zkbot referenced this in commit 8777d2884e on Sep 29, 2020
  90. zkbot referenced this in commit b5fa52b701 on Oct 1, 2020
  91. sidhujag referenced this in commit 8ce33973c3 on Nov 10, 2020
  92. sidhujag referenced this in commit 50ec6bafea on Nov 10, 2020
  93. sidhujag referenced this in commit 98c9ba6d69 on Nov 10, 2020
  94. sidhujag referenced this in commit 8d83aefe28 on Nov 10, 2020
  95. sidhujag referenced this in commit 207d5ff5f3 on Nov 10, 2020
  96. furszy referenced this in commit e9807864f3 on Apr 18, 2021
  97. PastaPastaPasta referenced this in commit 2c81cbc6c9 on Jun 27, 2021
  98. PastaPastaPasta referenced this in commit 8f92d6dc50 on Jun 28, 2021
  99. PastaPastaPasta referenced this in commit a90dde110b on Jun 29, 2021
  100. PastaPastaPasta referenced this in commit 282f6492ea on Jul 1, 2021
  101. PastaPastaPasta referenced this in commit 6da725c555 on Jul 1, 2021
  102. PastaPastaPasta referenced this in commit 51813dd249 on Jul 14, 2021
  103. PastaPastaPasta referenced this in commit 39f7ef74d2 on Sep 17, 2021
  104. DrahtBot locked this on Feb 15, 2022


fanquake emilengler MarcoFalke laanwj practicalswift hebasto DrahtBot theuni jtimon fjahr

Labels
GUI Build system

Milestone
0.20.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me