GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing by luke-jr · Pull Request #15063 · bitcoin/bitcoin

luke-jr · 2018-12-30T10:58:53Z

No description provided.

src/qt/paymentserver.cpp

fanquake · 2018-12-31T00:19:32Z

When I try compiling with ./configure --disable-bip70:

  CXX      qt/libbitcoinqt_a-sendcoinsdialog.o
  CXX      qt/libbitcoinqt_a-sendcoinsentry.o
  CXX      qt/libbitcoinqt_a-signverifymessagedialog.o
qt/paymentserver.cpp:321:9: error: expected expression
        else // normal URI
        ^
1 error generated.
make[2]: *** [qt/libbitcoinqt_a-paymentserver.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1

luke-jr · 2018-12-31T02:53:24Z

Fixed, added another fix, and a Travis test for no-BIP70

.travis.yml

Empact · 2018-12-31T19:09:12Z

How about a test case for this behavior?

jameshilliard · 2019-01-22T21:32:30Z

@luke-jr can you rebase this? This should probably be a 0.18 milestone requirement.

Sjors · 2019-02-11T12:34:04Z

cc @MarcoFalke since this also adds one more Travis machine

maflcko · 2019-02-11T13:11:13Z

Don't we already have at least 3 jobs with a gui? I'd prefer to just set the configure flag on one of them.

…payment request file

luke-jr · 2019-02-11T15:17:36Z

@MarcoFalke It doesn't make sense to test this without the wallet. Is there a reason x86_64 Linux [GOAL: install] [xenial] [no depends, only system libs, sanitizers: thread (TSan), no wallet] needs to be wallet-less? Or where else would you want to add it?

luke-jr · 2019-02-11T15:17:48Z

(also, rebased)

DrahtBot · 2019-02-11T16:04:42Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15295 (fuzz: Add test/fuzz/test_runner.py and run it in travis by MarcoFalke)
#15134 (tests: Add a Travis ASan/LSan/UBSan job testing in a unsigned char environment (-funsigned-char) by practicalswift)

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.

maflcko · 2019-02-11T16:47:09Z

.travis.yml

+        PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev"
+        NO_DEPENDS=1
+        GOAL="install"
+        BITCOIN_CONFIG="--enable-zmq --disable-bip70 --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"


I'd prefer to set --disable-bip70 on one of the other bionic jobs that have wallet enabled.

@MarcoFalke It doesn't make sense to test this without the wallet. Is there a reason x86_64 Linux [GOAL: install] [xenial] [no depends, only system libs, sanitizers: thread (TSan), no wallet] needs to be wallet-less? Or where else would you want to add it?

Maybe L133, L103, or L93?

I think xenial has wallet disabled because the thread sanitizer yells at bdb

laanwj · 2019-02-12T09:28:08Z

utACK 84f5315

Sjors · 2019-02-12T10:14:57Z

Compiles and src/qt/test/test_bitcoin-qt passes on macOS 10.14.3 (--disable-bip70 & --enable-bip70). These tests don't cover much though.

84f5315 looks good modulo Travis, but it seems like a good idea to actually test this. Is there a (testnet or otherwise) site that supports BIP21 fallback for BIP71?

Also can someone upload a BIP70 file with and without such fallback?

gmaxwell · 2019-02-14T19:09:30Z

utACK

jonasschnelli · 2019-02-14T19:18:30Z

utACK 84f5315

…1 parsing 84f5315 Travis: Add test without BIP70 (but still full wallet + tests) (Luke Dashjr) 113f000 GUI: If BIP70 is disabled, give a proper error when trying to open a payment request file (Luke Dashjr) 9975282 GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing (Luke Dashjr) Pull request description: Tree-SHA512: 66a684ce4336d0eac8b0107b405ff3a2cf312258a967f3e1b14734cd39db11e2db3e9b03492755583170d94d54754ef536b0776e5f19a0cc2caca8379eeb4495

…to BIP21 parsing 84f5315 Travis: Add test without BIP70 (but still full wallet + tests) (Luke Dashjr) 113f000 GUI: If BIP70 is disabled, give a proper error when trying to open a payment request file (Luke Dashjr) 9975282 GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing (Luke Dashjr) Pull request description: Tree-SHA512: 66a684ce4336d0eac8b0107b405ff3a2cf312258a967f3e1b14734cd39db11e2db3e9b03492755583170d94d54754ef536b0776e5f19a0cc2caca8379eeb4495

Summary: Commit 1: > GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing Commit 2: > GUI: If BIP70 is disabled, give a proper error when trying to open a payment request file bitcoin/bitcoin@113f000 This is a backport of [[bitcoin/bitcoin#15063 | core#15063]] Test Plan: ``` cmake .. -GNinja -DENABLE_BIP70=OFF ninja all check-all` ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9897

…to BIP21 parsing 84f5315 Travis: Add test without BIP70 (but still full wallet + tests) (Luke Dashjr) 113f000 GUI: If BIP70 is disabled, give a proper error when trying to open a payment request file (Luke Dashjr) 9975282 GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing (Luke Dashjr) Pull request description: Tree-SHA512: 66a684ce4336d0eac8b0107b405ff3a2cf312258a967f3e1b14734cd39db11e2db3e9b03492755583170d94d54754ef536b0776e5f19a0cc2caca8379eeb4495

…5063_15411 Backport 0.18 PRs bitcoin#14451, bitcoin#14686, bitcoin#14564, bitcoin#14568,bitcoin#15063, bitcoin#15411: deprecate BIP70 and build GUI without BIP70 support

fanquake added the GUI label Dec 30, 2018

Empact reviewed Dec 30, 2018

View reviewed changes

src/qt/paymentserver.cpp Outdated Show resolved Hide resolved

luke-jr force-pushed the bip70_fallback_to_bip21 branch from 381e22e to 2c5e0c5 Compare December 31, 2018 02:52

fanquake reviewed Dec 31, 2018

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

luke-jr force-pushed the bip70_fallback_to_bip21 branch from 2c5e0c5 to a74d4c0 Compare December 31, 2018 03:13

Empact mentioned this pull request Dec 31, 2018

Build: add names to Travis jobs #15020

Merged

DrahtBot added the Needs rebase label Jan 3, 2019

laanwj added this to the 0.18.0 milestone Jan 22, 2019

luke-jr added 3 commits February 11, 2019 15:08

GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing

9975282

GUI: If BIP70 is disabled, give a proper error when trying to open a …

113f000

…payment request file

Travis: Add test without BIP70 (but still full wallet + tests)

84f5315

luke-jr force-pushed the bip70_fallback_to_bip21 branch from a74d4c0 to 84f5315 Compare February 11, 2019 15:15

DrahtBot removed the Needs rebase label Feb 11, 2019

maflcko reviewed Feb 11, 2019

View reviewed changes

jonasschnelli merged commit 84f5315 into bitcoin:master Feb 14, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

luke-jr commented Dec 30, 2018

Uh oh!

Uh oh!

fanquake commented Dec 31, 2018

Uh oh!

luke-jr commented Dec 31, 2018

Uh oh!

Uh oh!

Empact commented Dec 31, 2018

Uh oh!

jameshilliard commented Jan 22, 2019

Uh oh!

Sjors commented Feb 11, 2019

Uh oh!

maflcko commented Feb 11, 2019

Uh oh!

luke-jr commented Feb 11, 2019

Uh oh!

luke-jr commented Feb 11, 2019

Uh oh!

DrahtBot commented Feb 11, 2019

Conflicts

Uh oh!

maflcko Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

luke-jr Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 12, 2019

Uh oh!

Sjors commented Feb 12, 2019

Uh oh!

gmaxwell commented Feb 14, 2019

Uh oh!

jonasschnelli commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants