Page MenuHomePhabricator

Merge #14451: Allow building GUI without BIP70 support
ClosedPublic

Authored by jasonbcox on Dec 2 2019, 23:57.

Details

Summary

48439b3c10391e5f5555c7d98e1a99706b77eaf7 Don't link SSL_LIBS with GUI unless BIP70 is enabled (James Hilliard)
9dcf6c0dfec51f2a49edef537f377422d6dbdceb build: Add --disable-bip70 configure option (Wladimir J. van der Laan)

Pull request description:

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

Tree-SHA512: 1e16ee8d2cdac9499f751ee7b50d058278150f9e38a87a47ddb5105dd0353cdedabe462903f54ead6209b249b249fe5e6a10d29631531be27400f2f69c25b9b9

Backport of Core PR14451, but without the deprecation message commit
https://github.com/bitcoin/bitcoin/pull/14451/files
Completes T700
Depends on D4668

Test Plan
cmake -GNinja -DENABLE_BIP70=ON ..
ninja check

cmake -GNinja -DENABLE_BIP70=OFF ..
ninja check

cmake -GNinja -DBUILD_BITCOIN_WALLET=OFF ..
ninja check

../configure
make check

../configure --disable-bip70
make check

../configure --disable-wallet
make check

Sanity check to make sure bitcoin-qt runs without errors.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Dec 2 2019, 23:57
jasonbcox planned changes to this revision.Dec 2 2019, 23:57
jasonbcox requested review of this revision.Dec 3 2019, 23:01
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Dec 4 2019, 01:47

Do not deprecate BIP70.

This revision now requires changes to proceed.Dec 4 2019, 01:47
zquestz added a subscriber: zquestz.Dec 4 2019, 18:37

Do not deprecate BIP70.

I also agree, we should not be deprecating BIP70!

jasonbcox updated this revision to Diff 14625.Dec 4 2019, 21:36

Remove BIP70 deprecation message

jasonbcox retitled this revision from Merge #14451: Add BIP70 deprecation warning and allow building GUI without BIP70 support to Merge #14451: Allow building GUI without BIP70 support.Dec 4 2019, 21:37
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox updated this revision to Diff 14659.Dec 5 2019, 19:54

Fixed issue building with ninja and wallet disabled

jasonbcox edited the test plan for this revision. (Show Details)Dec 5 2019, 19:55
Fabien requested changes to this revision.Dec 6 2019, 15:37
Fabien added a subscriber: Fabien.

This is really large, and hard enough to figure out if something is missing. Can you try to split this ?

configure.ac
1063 ↗(On Diff #14659)

There is a missing indentation (also in PR)

src/qt/paymentserver.h
85 ↗(On Diff #14659)

Previous layout was better.

This revision now requires changes to proceed.Dec 6 2019, 15:37
Fabien requested changes to this revision.Dec 9 2019, 20:02
Fabien added inline comments.
src/qt/paymentserver.cpp
164 ↗(On Diff #14700)

I'm not sure this check should be excluded. For example if someone opens a bitcoin core uri (bitcoin://) this will fail silently.

231 ↗(On Diff #14700)

I suppose this is from clang-format, any chance this can be made better ?

This revision now requires changes to proceed.Dec 9 2019, 20:02
jasonbcox updated this revision to Diff 14737.Dec 10 2019, 20:19

Address feedback

jasonbcox added inline comments.Dec 10 2019, 20:21
src/qt/paymentserver.cpp
231 ↗(On Diff #14700)

yes, clang-format did this. I've turned off formatting for these few lines. it's a little easier to read this way. Although it doesn't strictly match Core better, it looks to be more inline with Core's code, so should make merge conflicts easier to resolve. Let me know what you think.

Fabien accepted this revision.Dec 10 2019, 22:00
deadalnix requested changes to this revision.Dec 18 2019, 00:40
deadalnix added inline comments.
src/qt/CMakeLists.txt
246 ↗(On Diff #14737)

This should be moved in the other if.

This revision now requires changes to proceed.Dec 18 2019, 00:40
jasonbcox updated this revision to Diff 15017.Dec 20 2019, 01:21
  • Merged if statements per feedback
  • Rebased
deadalnix accepted this revision.Dec 22 2019, 03:25
This revision is now accepted and ready to land.Dec 22 2019, 03:25