Page MenuHomePhabricator

Merge #14451: Allow building GUI without BIP70 support
Needs RevisionPublic

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

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
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
Branch
pr14451-bip70
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8457
Build 14925: Bitcoin ABC Buildbot
Build 14924: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Mon, Dec 2, 23:57
jasonbcox planned changes to this revision.Mon, Dec 2, 23:57
jasonbcox requested review of this revision.Tue, Dec 3, 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.Wed, Dec 4, 01:47

Do not deprecate BIP70.

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

Do not deprecate BIP70.

I also agree, we should not be deprecating BIP70!

jasonbcox updated this revision to Diff 14625.Wed, Dec 4, 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.Wed, Dec 4, 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.Thu, Dec 5, 19:54

Fixed issue building with ninja and wallet disabled

jasonbcox edited the test plan for this revision. (Show Details)Thu, Dec 5, 19:55
Fabien requested changes to this revision.Fri, Dec 6, 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.Fri, Dec 6, 15:37
Fabien requested changes to this revision.Mon, Dec 9, 20:02
Fabien added inline comments.
src/qt/paymentserver.cpp
164

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

231

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

This revision now requires changes to proceed.Mon, Dec 9, 20:02