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

luke-jr commented at 10:58 AM on December 30, 2018: member

No description provided.

fanquake added the label GUI on Dec 30, 2018

in src/qt/paymentserver.cpp:321 in 381e22e658 outdated

 320 | -                tr("Cannot process payment request because BIP70 support was not compiled in."),
 321 | -                CClientUIInterface::ICON_WARNING);
 322 | -#endif
 323 |              return;
 324 |          }
 325 | +#endif

Empact commented at 11:07 PM on December 30, 2018:

Just from reading this, seems this is invalid when ENABLE_BIP70 is disabled, due to the unpaired else block. Maybe we should disable BIP70 in one of the CI runs?

fanquake commented at 12:19 AM on December 31, 2018: member

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 force-pushed on Dec 31, 2018

luke-jr commented at 2:53 AM on December 31, 2018: member

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

in .travis.yml:117 in 2c5e0c5a2a outdated

 109 | @@ -110,6 +110,15 @@ jobs:
 110 |          FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" # Temporarily suppress ASan heap-use-after-free (see issue #14163)
 111 |          GOAL="install"
 112 |          BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"
 113 | +# x86_64 Linux, no BIP70
 114 | +    - stage: test
 115 | +      env: >-
 116 | +        HOST=x86_64-unknown-linux-gnu
 117 | +        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 libprotobuf-dev protobuf-compiler libqrencode-dev"

fanquake commented at 3:03 AM on December 31, 2018:

You should be able to skip installing the libprotobuf-dev & protobuf-compiler packages.

luke-jr commented at 3:13 AM on December 31, 2018:

Good point, fixed

luke-jr force-pushed on Dec 31, 2018

Empact commented at 7:09 PM on December 31, 2018: member

How about a test case for this behavior?

DrahtBot added the label Needs rebase on Jan 3, 2019

jameshilliard commented at 9:32 PM on January 22, 2019: contributor

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

laanwj added this to the milestone 0.18.0 on Jan 22, 2019

Sjors commented at 12:34 PM on February 11, 2019: member

cc @MarcoFalke since this also adds one more Travis machine

MarcoFalke commented at 1:11 PM on February 11, 2019: member

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.

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

GUI: If BIP70 is disabled, give a proper error when trying to open a payment request file 113f0004be

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

luke-jr force-pushed on Feb 11, 2019

luke-jr commented at 3:17 PM on February 11, 2019: member

@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 commented at 3:17 PM on February 11, 2019: member

(also, rebased)

DrahtBot commented at 4:04 PM on February 11, 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:

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

DrahtBot removed the label Needs rebase on Feb 11, 2019

in .travis.yml:142 in 84f53154e1

 137 | +      env: >-
 138 | +        HOST=x86_64-unknown-linux-gnu
 139 | +        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"
 140 | +        NO_DEPENDS=1
 141 | +        GOAL="install"
 142 | +        BITCOIN_CONFIG="--enable-zmq --disable-bip70 --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"

MarcoFalke commented at 4:47 PM on February 11, 2019:

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

luke-jr commented at 11:02 PM on February 11, 2019:

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

MarcoFalke commented at 11:23 PM on February 11, 2019:

Maybe L133, L103, or L93?

MarcoFalke commented at 11:24 PM on February 11, 2019:

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

laanwj commented at 9:28 AM on February 12, 2019: member

utACK 84f53154e1a0309ef582443476451748eb982805

Sjors commented at 10:14 AM on February 12, 2019: member

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 commented at 7:09 PM on February 14, 2019: contributor

utACK

jonasschnelli commented at 7:18 PM on February 14, 2019: contributor

utACK 84f53154e1a0309ef582443476451748eb982805

jonasschnelli merged this on Feb 14, 2019

jonasschnelli closed this on Feb 14, 2019

jonasschnelli referenced this in commit 758c6d784d on Feb 14, 2019

dzutto referenced this in commit 95a0b223eb on Aug 18, 2021

dzutto referenced this in commit 2e5646378b on Aug 19, 2021

dzutto referenced this in commit 3a21fc1d8c on Aug 19, 2021

dzutto referenced this in commit 748b227455 on Aug 23, 2021

deadalnix referenced this in commit d3b57ee3b2 on Aug 24, 2021

dzutto referenced this in commit bb99770889 on Aug 26, 2021

dzutto referenced this in commit a70a4fbb19 on Aug 26, 2021

dzutto referenced this in commit 73e4ccc188 on Aug 27, 2021

PastaPastaPasta referenced this in commit b9df9815ba on Aug 29, 2021

MarcoFalke locked this on Dec 16, 2021