extend ParseParameters() with bitcoin: URI check #2932

Diapolo commented at 1:31 PM on August 24, 2013: none

this allows to remove some URI related double processing of passed command-line arguments for Bitcoin-Qt and bitcoind by collecting a vector of URIs in ParseParameters()
rename ipcSendCommandLine() to ipcSendUris() to reflect we do not send the whole command-line, but just pre-filtered URIs
also just install an OSX-specific event filter in paymentserver on Mac
fCommandLine is also no longer a global in util, but a local variable in bitcoind.cpp

Diapolo commented at 8:51 AM on August 29, 2013: none

Any comments?

laanwj commented at 11:13 AM on August 29, 2013: member

Sounds sensible to do all the parsing of parameters in ParseParameters. Maybe store the URL as well, and not only signal a boolean? This avoids having to scan the arguments again to find out what exactly the URL was.

Diapolo commented at 1:45 PM on August 29, 2013: none

Nice suggestion, but what happens, if the parameters would contain multiple bitcoin: URIs or is that even a possible condition?

laanwj commented at 3:06 PM on August 29, 2013: member

Yes, I think that's a possible condition. You could make it store a vector of strings instead of one string. A length of zero would then mean "no URL".

Diapolo commented at 10:18 AM on September 20, 2013: none

@laanwj Changed to a vector of strings and modified paymentserver to benefit of the changes. Can you take another look?

in src/bitcoind.cpp:None in 00b94d64ad outdated

  67 | @@ -67,7 +68,7 @@ bool AppInit(int argc, char* argv[])
  68 |  
  69 |          // Command-line RPC
  70 |          for (int i = 1; i < argc; i++)
  71 | -            if (!IsSwitchChar(argv[i][0]) && !boost::algorithm::istarts_with(argv[i], "bitcoin:"))
  72 | +            if (!IsSwitchChar(argv[i][0]) && vUris.empty())

Diapolo commented at 10:24 AM on September 20, 2013:

I could need some help with this clause, currently it seems "wrong" after my change...

laanwj commented at 8:28 AM on September 28, 2013:

You can factor the vUris.empty() out from the loop, even skip the loop if there are any URLs. You still need the loop to check for switch characters, but the Uris check has already been done, so

if(vUris.empty())
{
    bool fCommandLine = false;
    for (int i = 1; i < argc; ++i)
        if (!IsSwitchChar(argv[i][0])
            fCommandLine = true;
    if(fCommandLine) 
    {
        ...
    }
}

Diapolo commented at 9:56 AM on September 21, 2013: none

@TheBlueMatt Can you restart a build if the pulltester errors are fixed?

Diapolo commented at 5:53 AM on September 25, 2013: none

I'm still rather sure this is a @BitcoinPullTester related error, can some core-dev take a look at http://jenkins.bluematt.me/pull-tester/8f3cb255f195b4808ca5e64c01e6bc6c58cbf210/test.log

extend ParseParameters() with bitcoin: URI check

- this allows to remove some URI related double processing of passed
  command-line arguments for Bitcoin-Qt and bitcoind by collecting a
  vector of URIs in ParseParameters()
- rename ipcSendCommandLine() to ipcSendUris() to reflect we do not send
  the whole command-line, but just pre-filtered URIs
- also just install an OSX-specific event filter in paymentserver on Mac
- fCommandLine is also no longer a global in util, but a local variable
  in bitcoind.cpp

4e5d555f05

Diapolo commented at 1:20 PM on September 28, 2013: none

@laanwj Can you have another look, I updated the code in bitcoind.cpp to what you had suggested :). Also fCommandLine is no longer a global now.

laanwj commented at 7:28 AM on September 29, 2013: member

Yep, haven't tested it though!

In time it'd be better to split off the RPC client to a seperate executable and remove the hack. Likely we'd need to do this in two steps 0.9.x) add rpc client executable (I leave it to other people to bikeshed the name), support rpc client in bitcoind for backwards compatibility but deprecate it 0.10.x) remove backwards compatibility in bitcoind

sipa commented at 2:13 PM on September 29, 2013: member

@laanwj I'm very much in favor of splitting off the RPC client into a separate binary.

Diapolo commented at 8:40 AM on October 1, 2013: none

Your suggestions are good but far beyond the scope of this pull :). Any final comments/nits/ACKs?

laanwj commented at 4:41 PM on October 1, 2013: member

Code changes are ACK, but anything that affects command line parsing needs to be extensively tested. So I'd like to see test reports from a few people (that bitcoin URLs still work, and other options still work, that the rpc client still works, and combinations...) before merging.

BitcoinPullTester commented at 5:12 AM on October 4, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4e5d555f05ccd25ddf0efb1c993b2d29de605e3e 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.

gavinandresen commented at 5:29 AM on October 4, 2013: contributor

util.cpp ParseParameters is the wrong place to look for bitcoin: URIs, in my opinion. It should just return an array of strings that it could not parse. The payment server code can then look through that array and pull out any bitcoin: URIs.

The payment server code currently looks for two things: bitcoin: URIs, but also filenames to files that contain payment requests. It is important to keep the filename functionality, because with the right registry entries that will let users drag and drop payment requests sent as (for example) email attachments.

laanwj commented at 11:38 AM on October 17, 2013: member

Agreed, checking for bitcoin: in the core makes little sense as it has no knowledge of bitcoin URIs. Returning all non-parseable arguments is more flexible too.

Diapolo commented at 12:24 PM on October 17, 2013: none

Indeed, I'll close this and return to the garage ^^.

Diapolo closed this on Oct 17, 2013

Diapolo deleted the branch on Oct 17, 2013

Bushstar referenced this in commit 7c05aa8213 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021