Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING7144c5d95cce: Merge #14451: Allow building GUI without BIP70 support
rABC7144c5d95cce: Merge #14451: Allow building GUI without BIP70 support

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Dec 2 2019, 23:57

Herald added a reviewer: deadalnix. · View Herald TranscriptDec 2 2019, 23:57

jasonbcox planned changes to this revision.Dec 2 2019, 23:57

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

jasonbcox added a child revision: D4620: Merge #15101: gui: Add WalletController.Dec 3 2019, 00:01

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8380: Diff 14583.Dec 3 2019, 00:14

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

jasonbcox requested review of this revision.Dec 3 2019, 23:01

jasonbcox edited the test plan for this revision. (Show Details)

Do not deprecate BIP70.

This revision now requires changes to proceed.Dec 4 2019, 01:47

In D4619#111500, @deadalnix wrote:

Do not deprecate BIP70.

I also agree, we should not be deprecating BIP70!

Remove BIP70 deprecation message

Build Bitcoin-ABC / Diffs / Diff Testing started.

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)

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8414: Diff 14625.Dec 4 2019, 21:48

Fixed issue building with ninja and wallet disabled

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox edited the test plan for this revision. (Show Details)Dec 5 2019, 19:55

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8435: Diff 14659.Dec 5 2019, 20:06

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

jasonbcox mentioned this in D4668: [move only] Move BIP70 code together in preparation to backport PR14451 BIP70 changes.Dec 9 2019, 17:19

jasonbcox mentioned this in rABC4e65d88e3a16: [move only] Move BIP70 code together in preparation to backport PR14451 BIP70….Dec 9 2019, 18:04

Rebase

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox edited the summary of this revision. (Show Details)Dec 9 2019, 18:12

jasonbcox added a parent revision: D4668: [move only] Move BIP70 code together in preparation to backport PR14451 BIP70 changes.

jasonbcox edited the summary of this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8457: Diff 14700.Dec 9 2019, 18:28

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

Address feedback

Build Bitcoin-ABC / Diffs / Diff Testing started.

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.