Windows URI Support #1437

pull TheBlueMatt wants to merge 3 commits into bitcoin:master from TheBlueMatt:uri changing 7 files +43 −36
  1. TheBlueMatt commented at 12:04 AM on June 10, 2012: member

    This is a redo of #986, but is based on Boost 1.49 which fixed the underlying issue (mostly). It also reverts 7b90edb5a6 to re-enable URI handling itself.

  2. Diapolo commented at 11:27 AM on June 10, 2012: none

    Whenever I proposed to update to Boost 1.49 for the Win-build I got the answer no that is not importand enough ... see #1023, which even hardens the URI handling.

  3. TheBlueMatt commented at 12:28 PM on June 10, 2012: member

    Dunno, maybe gavin didn't quite understand the phrasing there. To be clear, this simply encourages using a path of boost_1_49_0 instead of boost_1_47_0 when building with a MinGW makefile, if you add your own -I/-L you can still gladly use any boost version you want. It also upgrades gitian build to use 1.49 and patches boost according to a request for testing that a boost developer made at https://svn.boost.org/trac/boost/ticket/5392#comment:29 and that has not gotten any negative confirmations and works as far as I've tested it.

  4. Diapolo commented at 12:34 PM on June 10, 2012: none

    I'm with you and I have used boost 1.49 with this mentioned lines enabled for a long time now. I think my pull would perfectly fit on top of what you are doing here, but it seems no one even tried to look at it in detail :-/.

  5. in share/setup.nsi:None in b3b22a1e47 outdated 
     104 | -    #    WriteRegStr HKCR "bitcoin" "" "URL:Bitcoin"

 105 | -    #    WriteRegStr HKCR "bitcoin\DefaultIcon" "" $INSTDIR\bitcoin-qt.exe

 106 | -    #    WriteRegStr HKCR "bitcoin\shell\open\command" "" '"$INSTDIR\bitcoin-qt.exe" "$$1"'

 107 | +    WriteRegStr HKCR "bitcoin" "URL Protocol" ""

 108 | +    WriteRegStr HKCR "bitcoin" "" "URL:Bitcoin"

 109 | +    WriteRegStr HKCR "bitcoin\DefaultIcon" "" $INSTDIR\bitcoin-qt.exe
    Diapolo commented at 3:40 PM on June 10, 2012:

    To ensure we use the first available Icon from the executable you could use $INSTDIR\bitcoin-qt.exe,0.

    TheBlueMatt commented at 4:46 PM on June 10, 2012:

    Do we want that, wont not using that let Windows pick the best icon based on size and color-depth?

    laanwj commented at 7:23 PM on June 12, 2012:

    One windows "icon" embeds multiple sizes/color depths. An executable can embed multiple icons, for example the icons for different file types. However, Bitcoin-qt only has one icon defined in the .rc, so this doesn't matter here.

  6. in share/setup.nsi:None in b3b22a1e47 outdated 
     103 | -    #    WriteRegStr HKCR "bitcoin" "URL Protocol" ""

 104 | -    #    WriteRegStr HKCR "bitcoin" "" "URL:Bitcoin"

 105 | -    #    WriteRegStr HKCR "bitcoin\DefaultIcon" "" $INSTDIR\bitcoin-qt.exe

 106 | -    #    WriteRegStr HKCR "bitcoin\shell\open\command" "" '"$INSTDIR\bitcoin-qt.exe" "$$1"'

 107 | +    WriteRegStr HKCR "bitcoin" "URL Protocol" ""

 108 | +    WriteRegStr HKCR "bitcoin" "" "URL:Bitcoin"
    Diapolo commented at 3:43 PM on June 10, 2012:

    HTTP uses URL:HyperText Transfer Protocol as description, so we could use URL:Bitcoin Protocol.

    We should also add a FriendlyTypeName subkey, which should be set to the same string as the default string (see: http://msdn.microsoft.com/en-us/library/windows/desktop/cc144152%28v=vs.85%29.aspx)

    TheBlueMatt commented at 4:49 PM on June 10, 2012:

    AFAIK, its never really displayed anywhere, and if it is, Id guess its clear ;). Anyway, Id kinda like to leave the Revert commit to just revert and nothing more...feel free to add more useful stuff in a separate pull, Id say.

  7. gavinandresen commented at 1:30 PM on June 12, 2012: contributor

    I think this feature is big enough it needs a test plan (see https://secure.bettermeans.com/projects/4180/wiki/Raw_Transaction_RPC_Test_Plan for an example of what I'm thinking). Also, since it opens up potential security holes, writing a tool to throw "fuzzed" input at it and verifying that nothing bad happens would make me happy.

  8. TheBlueMatt commented at 4:03 PM on June 12, 2012: member

    To test:

    1. launch Bitcoin-Qt with a bitcoin: URI to make sure it opens URIs provided at initial start. Result should be opening directly to the send dialog with the content of the provided URI.
    2. while [ true ]; do ./release/bitcoin-qt.exe bitcoin:1JBMattRztKDF2KRS3vhjJXA7h47NEsn2c; done Ensure only one URI is added per second to the send dialog (note that any more sent to Bitcoin will result in an error locking datadir as the new client attempts to load normally, this is an unavoidable result of the IPC mechanism), Bitcoin-Qt isn't using an unreasonable amount of CPU, and switching to the send dialog and scrolling to the bottom each time a new URI is added.
    3. With the listening bitcoin-Qt running, kill Windows without shutting it down, start again and ensure that the same results.
    4. Click the Send button and with the Send Dialog box up, attempt to open new URIs. Those URIs should be ignored until the send has finished.

    To Fuzz with printable characters and URIs up to size 300: (note that anything larger than 256 will attempt to launch Bitcoin and give an error that it cannot obtain a datadir lock). while [ true ]; do ./release/bitcoin-qt.exe bitcoin:tr -dc "[:print:]" < /dev/urandom | head -c $[ $RANDOM % 300 ]; sleep 1; done To Fuzz with control characters and URIs up to size 300: (note that anything larger than 256 will attempt to launch Bitcoin and give an error that it cannot obtain a datadir lock). while [ true ]; do ./release/bitcoin-qt.exe bitcoin:tr -dc "[:cntrl:]" < /dev/urandom | head -c $[ $RANDOM % 300 ]; sleep 1; done

  9. laanwj commented at 7:11 AM on June 13, 2012: member

    It would be nice to get URL support back in once and for all. If that means requiring boost 1.49 on windows that's fine with me. But it's too bad it still needs the monkey patch to be stable :/ Won't the upstream boost developers integrate it?

    Anything larger than 256 will attempt to launch bitcoin

    Is that desired behavior? I'd prefer showing an error message about a too-long URL, then quitting. Maybe for a later pull.

  10. Diapolo commented at 12:59 PM on June 13, 2012: none

    @laanwj I consider this pull as a base for further updates to the URI handling code, even if not all parts of my re-work pull #1023 are considered good or mergable, I'm fine with a cherry picking or splitting them. But I think this pull needs to get in ASAP and if we need a patched boost 1.49 on Windows that's ok.

  11. laanwj commented at 1:22 PM on June 13, 2012: member

    I agree with merging this ASAP.

    Feels like a deja-vu, but can we detect the patched boost 1.49 somehow, compile time? It would be nice if it failed if one tried to build on windows with URL support without the patched boost. Otherwise, someone building bitcoin manually on windows might run into the mysterious crashes again.

  12. in src/qt/bitcoin.cpp:None in b3b22a1e47 outdated 
     115 | @@ -116,9 +116,6 @@ static void handleRunawayException(std::exception *e)
 116 |  #ifndef BITCOIN_QT_TEST
 117 |  int main(int argc, char *argv[])
 118 |  {
 119 | -#if !defined(MAC_OSX) && !defined(WIN32)
    laanwj commented at 1:24 PM on June 13, 2012:

    This enables it for MAC_OSX too, is that intended? I remember boost interprocess had/has another problem on mac: it took 100% CPU time.

    TheBlueMatt commented at 2:31 PM on June 13, 2012:

    This simply attempts to open the message_queue, which will fail as we never set up the message_queue listener in ipcInit on OSX. There is no point having an ugly ifdef here, especially since we are never gonna get called with a bitcoin: URI on osx on the command line unless a user does it manually.

  13. Diapolo commented at 1:39 PM on June 13, 2012: none

    @laanwj I guess we need no detection for the Windows users, we just need an official dependency package, which includes the used version of Boost and its libs.

  14. laanwj commented at 1:43 PM on June 13, 2012: member

    Agreed, but not everyone will be using that, if you build from source you could insist to build everything from source. It's easy to miss.

  15. Diapolo commented at 2:19 PM on June 13, 2012: none

    Why not #if !defined(BOOST_INTERPROCESS_HAS_WINDOWS_KERNEL_BOOTTIME) || !defined(BOOST_INTERPROCESS_HAS_KERNEL_BOOTTIME) and throw an error. It should be insinde an #ifdef WIN32, too or use defined(WIN32) as addition argument.

  16. TheBlueMatt commented at 2:39 PM on June 13, 2012: member

    In terms of erroring if we dont detect BOOST_... flags specified, Id really prefer not to. I dont want to follow boost and remove the checks if boost upgrades and complicate them with complicated boost version checks. Anyone who is building bitcoin for Windows should be reading and creating executables by closely following the gitian commands. Especially note that we add the mthreads/-fexceptions flags in the qt gitian descriptor, not in Bitcoin itself.

  17. laanwj commented at 2:52 PM on June 13, 2012: member

    -mthreads/-fexceptions doesn't apply to people building on windows. It's a cross-compile specific issue.

    Another idea (better than failing) would be to make URL support a build option in bitcoin-qt.pro, which is disabled by default on Windows. Then document it as "Enable this only with a patched Boost >=1.49 (link to patch), otherwise you may experience random crashes at startup".

  18. TheBlueMatt commented at 2:56 PM on June 13, 2012: member

    And AFAIK, most people who compile bitcoin for windows do cross compile. I know several people recently have been on #bitcoin-dev asking how to build it, and ended up giving up and cross compiling in an Ubuntu VM.

  19. Diapolo commented at 3:55 PM on June 13, 2012: none

    At least I build it native on Windows ;).

  20. Upgrade to Boost 1.49 on Win32 39471861d5
  21. Fix #956 the Boost 1.49 way. 1d42878adb
  22. Revert "Disable bitcoin: URI handling on Windows for the 0.6 release"
    This reverts commit 7b90edb5a6cada7176012d09d748847b5f966585.
    ad5f29b743
  23. TheBlueMatt commented at 3:19 PM on June 15, 2012: member

    Added a #if defined(WIN32) && (!defined(BOOST_INTERPROCESS_HAS_WINDOWS_KERNEL_BOOTTIME) || !defined(BOOST_INTERPROCESS_HAS_KERNEL_BOOTTIME) || BOOST_VERSION < 104900) #warning

  24. Diapolo commented at 5:52 PM on June 15, 2012: none

    That's a good thing, even if it does not look very nice ;).

  25. laanwj commented at 6:36 AM on June 16, 2012: member

    I don't think looking nice is important in any way here. It is a complex logical expression because we need a complex logical expression.

    ACK

  26. sipa commented at 11:38 AM on June 16, 2012: member

    I think it is important that this functionality gets added soon. I haven't checked or tested the code though.

  27. gavinandresen merged this on Jul 5, 2012
  28. gavinandresen closed this on Jul 5, 2012
  29. suprnurd referenced this in commit 7242e29228 on Dec 5, 2017
  30. lateminer referenced this in commit 3d2e7570b4 on Jan 22, 2019
  31. lateminer referenced this in commit 8a6f5147c9 on May 6, 2020
  32. lateminer referenced this in commit eb1ed66c2e on May 6, 2020
  33. lateminer referenced this in commit cb6d15a6ac on May 6, 2020
  34. lateminer referenced this in commit 1fa5156a97 on May 6, 2020
  35. lateminer referenced this in commit b26dbc4e55 on May 6, 2020
  36. lateminer referenced this in commit c94f2412af on May 6, 2020
  37. DrahtBot locked this on Sep 8, 2021
Contributors
TheBlueMattDiapololaanwjgavinandresensipa
Linked (view graph)
#986 Fix boost::interprocess::detail::winapi::get_last_bootup_time
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me