Restore bitcoin: URIs determining network address #4623

rnicoll commented at 6:58 PM on August 2, 2014: contributor

Restored functionality where the network a Bitcoin URI address applies to is used to determine the network the client uses, altered such that invalid addresses do not incorrectly send the user to the test network. This avoids the previous issue where an invalid URI caused the client to open the wrong network, while also ensuring that the wallet opens a network appropriate to the URI where possible.

laanwj commented at 12:32 PM on August 3, 2014: member

~~Can you solve this without changes to the core code?~~

Also: can you provide a clear and easily executed before/after test plan to make sure that this does get it right. This is the so-manyth change to this part of the code in a short time:

509f926 - Payment request parsing on startup now only changes network if a valid network name is specified.
dd49e92 - qt: fix 'opens in testnet mode when presented with a BIP-72 link with no fallback'

laanwj added the label GUI on Aug 3, 2014

in src/qt/paymentserver.cpp:None in 11a1e9d8b4 outdated

 415 | @@ -415,12 +416,22 @@ void PaymentServer::handleURIOrFile(const QString& s)
 416 |              if (GUIUtil::parseBitcoinURI(s, &recipient))
 417 |              {
 418 |                  CBitcoinAddress address(recipient.address.toStdString());
 419 | -                if (!address.IsValid()) {
 420 | +                
 421 | +                if (address.IsValid(Params(CBaseChainParams::MAIN)))
 422 | +                {
 423 | +                    SelectParams(CBaseChainParams::MAIN);

laanwj commented at 9:00 AM on August 29, 2014:

handleUriOrFile is the wrong place to be changing the network parameters. After all it can be called when the application is already running, and switching the parameters while the application is running could result in serious bugs.

laanwj commented at 7:09 AM on August 30, 2014:

You've added SelectParams in ipcParseCommandLine, but not removed it here :)

laanwj commented at 9:00 AM on August 29, 2014: member

Reconsidering my stance on the core changes here, they seem reasonable. You need a way to see if an address matches another network without switching the network.

rnicoll commented at 9:12 AM on August 29, 2014: contributor

Getting back to this is on my list of things for tonight/tomorrow; I'll get the test cases written up, and look for places better suited to put the switching code into.

rnicoll force-pushed on Aug 29, 2014

rnicoll commented at 11:36 PM on August 29, 2014: contributor

Rebased and now rebuilt, and I'd swear that paymentserver.cpp has changed meanwhile. Anyway, network picking code is now in PaymentServer::ipcParseCommandLine() which shouldbe more appropriate.

Test cases:

./src/qt/bitcoin-qt -testnet bitcoin:n4dqmiBSWCTLnJRs4FXRxeHVAhZJhyPGCR ./src/qt/bitcoin-qt bitcoin:n4dqmiBSWCTLnJRs4FXRxeHVAhZJhyPGCR ./src/qt/bitcoin-qt bitcoin:1PfnkaYG7Bamc4zArNRo38LsphPYLgDwwh ./src/qt/bitcoin-qt -testnet bitcoin:1PfnkaYG7Bamc4zArNRo38LsphPYLgDwwh ./src/qt/bitcoin-qt bitcoin:1PfnkaYG7Bamc4zArNRo38LsphPYLgDww

On existing master these launch test, test, main, main, test (and error). The last is incorrect; it should launch to main (as the default) in the last case.

With the patch applied, this launches test, test, main, main, main (and error again). That's correct, as the address is invalid.

Broken addresses on command line no longer trigger testnet.

When passing a bitcoin: URI on the command line, invalid addresses do not incorrectly send the
user to the test network.

e84843c0db

rnicoll force-pushed on Aug 30, 2014

rnicoll commented at 8:57 AM on August 30, 2014: contributor

Once more, with feeling...

BitcoinPullTester commented at 9:20 AM on August 30, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4623_e84843c0dbb9cb853b912c09858b01c5c9302b09/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj merged this on Sep 10, 2014

laanwj closed this on Sep 10, 2014

laanwj referenced this in commit f23869e14b on Sep 10, 2014

rnicoll deleted the branch on Mar 13, 2018

DrahtBot locked this on Sep 8, 2021