Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Feb 14, 2019

Conversation

Projects
None yet
10 participants
@luke-jr
Copy link
Member

commented Dec 30, 2018

No description provided.

@fanquake fanquake added the GUI label Dec 30, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

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 luke-jr force-pushed the luke-jr:bip70_fallback_to_bip21 branch Dec 31, 2018

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

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

Show resolved Hide resolved .travis.yml Outdated

@luke-jr luke-jr force-pushed the luke-jr:bip70_fallback_to_bip21 branch to a74d4c0 Dec 31, 2018

@Empact

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

How about a test case for this behavior?

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

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

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

cc @MarcoFalke since this also adds one more Travis machine

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

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.

@luke-jr luke-jr force-pushed the luke-jr:bip70_fallback_to_bip21 branch from a74d4c0 to 84f5315 Feb 11, 2019

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Feb 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?

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

(also, rebased)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

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 DrahtBot removed the Needs rebase label Feb 11, 2019

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 11, 2019

Member

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Feb 11, 2019

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 11, 2019

Member

Maybe L133, L103, or L93?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 11, 2019

Member

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

utACK 84f5315

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 84f5315

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Feb 14, 2019

Merge #15063: GUI: If BIP70 is disabled, attempt to fall back to BIP2…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.