Drop miniupnp dependency #31130

pull darosior wants to merge 10 commits into bitcoin:master from darosior:2410_drop_upnp changing 40 files +45 −428
  1. darosior commented at 1:31 pm on October 22, 2024: member

    This PR removes UPnP IGD support and drops our miniupnp dependency.

    Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed here), some of which directly affected our software (RCE in 2015, OOM in 2020).

    The main purpose of this functionality is to have more (non-data-center) reachable nodes on the network. For a non-technical user running Bitcoin Core at home, the software would automatically open a port on their router to receive incoming connections. This way, users not able to manually open a port on their router would still provide the network with more resources and enhance its diversity.

    However, due to past vulnerabilities (and a worry about unknown future ones) in miniupnpc this feature was disabled by default in #6795. Having it disabled by default kills (most of?) the purpose of having this functionality in the first place: someone technical enough to understand the -upnp startup option or the “enable UPnP” setting is most likely able to open a port on his box in the first place.

    In addition, laanwj implemented PCP with a NAT-PMP fallback directly in Bitcoin Core in #30043. If we ever want to re-enable automatic NAT traversal by default in Bitcoin Core, this is the best option (and in my opinion the only sane one). The NAT-PMP fallback makes it so compatibility shouldn’t be (much of) an issue.

    On balance, i believe that keeping this functionality and this barely maintained C dependency has higher costs than benefits. Therefore i propose that we get rid of it.

  2. DrahtBot commented at 1:31 pm on October 22, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK 1440000bytes, laanwj, i-am-yuvi, jarolrod
    Concept ACK TheCharlatan, theStack, stickies-v, kristapsk, kevkevinpal, fjahr, Christewart, pythcoiner, theuni, sipa
    Stale ACK maflcko, edilmedeiros

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)
    • #31073 (ci: Split out native fuzz jobs for macOS and windows by dergoegge)
    • #31042 (build: Rename PACKAGE_* variables to CLIENT_* by hebasto)
    • #30997 (build: Switch to Qt 6 by hebasto)
    • #30634 (ci: Use clang-19 from apt.llvm.org by maflcko)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)

    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.

  3. TheCharlatan commented at 1:34 pm on October 22, 2024: contributor
    Concept ACK
  4. laanwj added the label P2P on Oct 22, 2024
  5. laanwj added the label Build system on Oct 22, 2024
  6. hebasto commented at 1:39 pm on October 22, 2024: member

    To fix CI failure:

     0--- a/src/mapport.cpp
 1+++ b/src/mapport.cpp
 2@@ -2,8 +2,6 @@
 3 // Distributed under the MIT software license, see the accompanying
 4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 5 
 6-#include <bitcoin-build-config.h> // IWYU pragma: keep
 7-
 8 #include <mapport.h>
 9 
10 #include <clientversion.h>
  7. laanwj commented at 1:48 pm on October 22, 2024: member

    Big concept ACK on this. As mentioned, MiniUPnP is some pretty dangerous code that resulted in the only known exploitable RCE in bitcoin core in history. It does brittle C-based string manipulation to generate and parse XML and HTTP which still has a high risk of having further bugs.

    For that reason it’s been disabled by default for years, there is no path toward enabling it, and meanwhile i doubt many people have been going through the hassle of enabling UPnP in the settings to open a port.

    #30043 and follow-ups provide a reasonable replacement, which might be enabled by default at some point, when we are confident enough about it. #31022 adds testing of its network interaction and may help there.

    Besides that, in general it’s good to no longer have miniupnp be part of the GUIX build, as it’s a sparsely maintained library which could make it target for xz-like backdooring.

  8. darosior force-pushed on Oct 22, 2024
  9. DrahtBot added the label CI failed on Oct 22, 2024
  10. DrahtBot commented at 1:55 pm on October 22, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31889998343

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  11. DrahtBot removed the label CI failed on Oct 22, 2024
  12. theStack commented at 11:37 pm on October 22, 2024: contributor
    Concept ACK
  13. stickies-v commented at 11:13 am on October 23, 2024: contributor
    Concept ACK
  14. kristapsk commented at 12:22 pm on October 23, 2024: contributor
    Concept ACK
  15. kevkevinpal commented at 12:34 pm on October 23, 2024: contributor

    Concept ACK

    There have been instances where there have been breakage when using newer minor versions of miniupnp aswell to the other mentioned reasons #30266 -> #30283 -> #30301

  16. kevkevinpal commented at 12:38 pm on October 23, 2024: contributor
    Might be worth adding a release note?
  17. fjahr commented at 1:41 pm on October 23, 2024: contributor
    Concept ACK but shouldn’t this go through a deprecation cycle?
  18. in src/init.cpp:566 in 48e291acb2 outdated 
    560@@ -561,11 +561,6 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
561     argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
562     argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port of %i will be used.", DEFAULT_TOR_CONTROL, DEFAULT_TOR_CONTROL_PORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
563     argsman.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION);
564-#ifdef USE_UPNP
565-    argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", DEFAULT_UPNP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
566-#else
567-    hidden_args.emplace_back("-upnp");
568-#endif
569     argsman.AddArg("-natpmp", strprintf("Use PCP or NAT-PMP to map the listening port (default: %u)", DEFAULT_NATPMP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    laanwj commented at 2:09 pm on October 23, 2024:
    Might want to keep this arg around (hidden) for one major version, but make it display an error that the functionality has been removed. This gives a clearer error to anyone who has it in their configuration file than “argument doesn’t exist”.
    darosior commented at 8:56 pm on October 23, 2024:
    Done.
  19. laanwj commented at 2:10 pm on October 23, 2024: member

    Concept ACK but shouldn’t this go through a deprecation cycle?

    imo a real deprecation cycle is not needed here. This has teetered on the brink of being removed for so long already. Also, deprecation cycles are generally for APIs, and this doesn’t add or remove any RPC or REST functionality.

  20. maflcko added the label Needs release note on Oct 23, 2024
  21. maflcko added the label DrahtBot Guix build requested on Oct 23, 2024
  22. Christewart commented at 4:09 pm on October 23, 2024: contributor
    concept ACK
  23. edilmedeiros commented at 5:04 pm on October 23, 2024: contributor
    Concept ACK
  24. darosior referenced this in commit 625ec41c38 on Oct 23, 2024
  25. darosior force-pushed on Oct 23, 2024
  26. darosior commented at 8:57 pm on October 23, 2024: member

    Kept the option as a hidden arg which will return an error explaining UPnP support was dropped and encouraging users to set -natpmp instead.

    Added a release note.

  27. in doc/release-notes-31130.md:4 in 625ec41c38 outdated 
    0@@ -0,0 +1,9 @@
1+P2P and network changes
2+-----------------------
3+
4+Support for UPnP was dropped. Consider using PCP or NAT-PMP instead.
    sipa commented at 9:05 pm on October 23, 2024:
    This is perhaps a bit confusing, given that from the user’s perspective these two are the same thing (-natpmp controls both).
    laanwj commented at 1:52 pm on October 24, 2024:
    maybe “If you want to open a port automatically, consider using the -natpmp option instead, which uses PCP or NAT-PMP depending on router support.” (in the GUI, the option is still called NATPMP as well, so no need to mention that seperately i think)
    darosior commented at 3:01 pm on October 24, 2024:
    Took the suggestion, thanks.
  28. DrahtBot added the label CI failed on Oct 23, 2024
  29. DrahtBot commented at 10:21 pm on October 23, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31973806317

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  30. jarolrod commented at 11:58 pm on October 23, 2024: member
    concept ack
  31. DrahtBot commented at 1:23 am on October 24, 2024: contributor

    Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

    File commit ffe4261cb0669b1e1a926638e0498ae5b63f3599(master) commit be52a2d5fa3e21a2c057513573b580dd4b483d66(master and this pull)
    SHA256SUMS.part 22cfd3f93c3a9ac6... 9b34cb07d86db2c5...
    *-aarch64-linux-gnu-debug.tar.gz 0a4dd54c1c7b3776... fc39b5581b9368ae...
    *-aarch64-linux-gnu.tar.gz 63571d5b306b47e4... 2eb580f165bfdfd7...
    *-arm-linux-gnueabihf-debug.tar.gz 3f29cd1363fca626... cffd2a8c5b392cdd...
    *-arm-linux-gnueabihf.tar.gz 626f2f15d407a6ef... f0bd0ca8c0512c86...
    *-arm64-apple-darwin-unsigned.tar.gz 5ca372a10f531b39... 803d49613fcc674a...
    *-arm64-apple-darwin-unsigned.zip 6aa68590bcc04b54... 40626c8e9f4a3453...
    *-arm64-apple-darwin.tar.gz 38cd70e5e6382266... 0c61bdae520e5bc2...
    *-powerpc64-linux-gnu-debug.tar.gz 05c45d0ec2722dc6... b74336f5da0854f7...
    *-powerpc64-linux-gnu.tar.gz 1cc79270c641813e... ed7a5c9c5a225ff1...
    *-riscv64-linux-gnu-debug.tar.gz 6e49db2174ea3135... 9c37c9feafc6739a...
    *-riscv64-linux-gnu.tar.gz c4cc512b96a85655... 9300e7deece6b304...
    *-x86_64-apple-darwin-unsigned.tar.gz 86cb7a97014f0373... ab623c9fe88bc416...
    *-x86_64-apple-darwin-unsigned.zip f2af96ebabbc52a3... 99a1373e872ac19d...
    *-x86_64-apple-darwin.tar.gz d1203f85e2b61c2d... e8205a08865fd8f3...
    *-x86_64-linux-gnu-debug.tar.gz c7ccbb4103a4f91e... 9d923ec270cb288b...
    *-x86_64-linux-gnu.tar.gz ca08639c38d73d68... e568b9045de5ac21...
    *.tar.gz 7ed420fdc4406061... 8ed122ad1a9223a9...
    guix_build.log 51b17f75f160f6cc... 36c2112233895d1a...
    guix_build.log.diff c280d4073d4d1513...
  32. DrahtBot removed the label DrahtBot Guix build requested on Oct 24, 2024
  33. fanquake removed the label Needs release note on Oct 24, 2024
  34. 1440000bytes commented at 1:42 pm on October 24, 2024: none

    Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed here), some of which directly affected our software (RCE in 2015, OOM in 2020).

    and CVE-2017-1000494 in 2018?

    Concept ACK

    Lint error: https://github.com/bitcoin/bitcoin/pull/31130/checks?check_run_id=31973806317

  35. darosior referenced this in commit 29bd2f1606 on Oct 24, 2024
  36. darosior force-pushed on Oct 24, 2024
  37. darosior commented at 3:04 pm on October 24, 2024: member
    Updated the release note to something clearer and fixed the check-doc.py lint error by using AddArg() directly for the hidden -upnp arg.
  38. qt: remove UPnP settings 844770b05e
  39. daemon: remove UPnP support 
    Keep the "-upnp" option as a hidden arg for one major version in order
to show a more user friendly error to people who had this option set in
their config file.
    038bbe7b20
  40. interfaces: remove now unused 'use_upnp' arg from 'mapPort' a5fcfb7385
  41. build: drop miniupnpc dependency a9598e5eaa
  42. darosior force-pushed on Oct 24, 2024
  43. darosior referenced this in commit 2a541e93cf on Oct 24, 2024
  44. darosior commented at 4:26 pm on October 24, 2024: member
    Rebased after #31148 merge.
  45. DrahtBot removed the label CI failed on Oct 24, 2024
  46. pythcoiner commented at 3:55 am on October 25, 2024: none
    concept ACK
  47. i-am-yuvi commented at 7:53 am on October 25, 2024: none
    Concept ACK!!
  48. in .github/workflows/ci.yml:75 in 3c0fe09cc2 outdated 
    72       - name: Compile and run tests
73         run: |
74           # Run tests on commits after the last merge commit and before the PR head commit
75           # Use clang++, because it is a bit faster and uses less memory than g++
76-          git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_MINIUPNPC=ON -DWITH_USDT=ON && cmake --build build -j $(nproc) && ctest --output-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }}
77+          git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON && cmake --build build -j $(nproc) && ctest --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }}
    maflcko commented at 8:21 am on October 25, 2024:
    3c0fe09cc2fac18175aac35a54d64c9f6cd4482b: This merge conflict was solved incorrectly. Please be careful when rebasing. See also diff3, which should make rebasing easier, if used correctly: https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#rebasingmerging-code
    darosior commented at 1:29 pm on October 25, 2024:
    My bad i did this one in a hurry. Fixed now, thanks for catching this.
  49. maflcko approved
  50. maflcko commented at 8:26 am on October 25, 2024: member

    Almost lgtm. Didn’t review the gui changes and the rebase was borked.

    ACK 2a541e93cf13362e52c62e4d01d34a602843471e 📏

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 2a541e93cf13362e52c62e4d01d34a602843471e 📏
35Gd8ObbrtS4VtkpC0GlzanymPWZ6n1+iRsigBUfaI+d+RC5UmLKR6iPjc6MOit/UP1GhKd52edRMDiWGb3rsAg==
  51. DrahtBot requested review from fjahr on Oct 25, 2024
  52. DrahtBot requested review from theStack on Oct 25, 2024
  53. DrahtBot requested review from laanwj on Oct 25, 2024
  54. DrahtBot requested review from stickies-v on Oct 25, 2024
  55. DrahtBot requested review from TheCharlatan on Oct 25, 2024
  56. laanwj commented at 9:13 am on October 25, 2024: member

    i’ve checked that no references no miniupnp as dependency remain after this change (except in historical release notes ofc).

    Also, successful guix build of 2a541e93cf13:

     0d0674bc3aa12577fb8d0319d51b34ab8650b810f56868a701b388f4f91cfc9b2  guix-build-2a541e93cf13/output/aarch64-linux-gnu/SHA256SUMS.part
 16be1f0a8fa6596bd4e6e68996b9a53cb438fb727e55755f6bf101656e1517c47  guix-build-2a541e93cf13/output/aarch64-linux-gnu/bitcoin-2a541e93cf13-aarch64-linux-gnu-debug.tar.gz
 2a9386bc64b0222ce03a3aa98edb2314b4c890de029e4e8aacb355505847ff024  guix-build-2a541e93cf13/output/aarch64-linux-gnu/bitcoin-2a541e93cf13-aarch64-linux-gnu.tar.gz
 3149b958ddc9c6ee942fb697814f1f0a85838b9ecaf3c86b62d40a3148dd0a830  guix-build-2a541e93cf13/output/arm-linux-gnueabihf/SHA256SUMS.part
 4bf885d642a8e0510ae250579302d1360fc64489a69f63edb80340f7d2cc98410  guix-build-2a541e93cf13/output/arm-linux-gnueabihf/bitcoin-2a541e93cf13-arm-linux-gnueabihf-debug.tar.gz
 5d947d31e240fda2ff62caa165ca543ed82b8bc8e8b1257631e00cc89b79fc2eb  guix-build-2a541e93cf13/output/arm-linux-gnueabihf/bitcoin-2a541e93cf13-arm-linux-gnueabihf.tar.gz
 60af5d5bb866c06593df194e574730ec2acb574f0a7df5f1200df29a4355e457e  guix-build-2a541e93cf13/output/arm64-apple-darwin/SHA256SUMS.part
 74c50e60e5b26e810ca27e0de48a5f2520eacc242906eea01620bd66df5df2d16  guix-build-2a541e93cf13/output/arm64-apple-darwin/bitcoin-2a541e93cf13-arm64-apple-darwin-unsigned.tar.gz
 8bc6bd0dbf76ee556f4034976f07fd393779fb1481d7eb5f29e21030295f3d89d  guix-build-2a541e93cf13/output/arm64-apple-darwin/bitcoin-2a541e93cf13-arm64-apple-darwin-unsigned.zip
 9e570600520646a4adefac087fbff07dcc69a859ec71bc3384da5fc0f63aaa0e9  guix-build-2a541e93cf13/output/arm64-apple-darwin/bitcoin-2a541e93cf13-arm64-apple-darwin.tar.gz
10bed7cbbc3c98579ef5265c3caf8cea7f6c1ff6c8b910754d18341ec04e301a59  guix-build-2a541e93cf13/output/dist-archive/bitcoin-2a541e93cf13.tar.gz
1142dab4f9404b79f5f92c930cfc08af0929d63701437f7c50e77018857ea74d88  guix-build-2a541e93cf13/output/powerpc64-linux-gnu/SHA256SUMS.part
1281516ccf5215b77171fe19026fa123dbd9873eabd9114c97045f27dfd361fbd1  guix-build-2a541e93cf13/output/powerpc64-linux-gnu/bitcoin-2a541e93cf13-powerpc64-linux-gnu-debug.tar.gz
13c86ca9763fc8cb7e58594cc22953c4909e8db672b71a53e74e33ea390475a57f  guix-build-2a541e93cf13/output/powerpc64-linux-gnu/bitcoin-2a541e93cf13-powerpc64-linux-gnu.tar.gz
142911f40bed7bb8ac9a9ee5ba390074b76b3932b16c999f1a183d4a2c81c9e143  guix-build-2a541e93cf13/output/riscv64-linux-gnu/SHA256SUMS.part
15758e57addf4d85be5ac4516e109604cb72406750d4ae6578c6138fcbb4e6cccb  guix-build-2a541e93cf13/output/riscv64-linux-gnu/bitcoin-2a541e93cf13-riscv64-linux-gnu-debug.tar.gz
16b04aab9c365fcf0a327a5d6f9bcc26773e350c2f149e1b49cedee3537bd44427  guix-build-2a541e93cf13/output/riscv64-linux-gnu/bitcoin-2a541e93cf13-riscv64-linux-gnu.tar.gz
1770b9f6e6f28223807ead560ab20f7cece7bb9d4535bedf76f21b77007d86c2f8  guix-build-2a541e93cf13/output/x86_64-apple-darwin/SHA256SUMS.part
184ea48c1a2e538f693fc25c41428c0c5df6cf440cc5530c9a6b877b6d0d7c3963  guix-build-2a541e93cf13/output/x86_64-apple-darwin/bitcoin-2a541e93cf13-x86_64-apple-darwin-unsigned.tar.gz
19bb712663bd97382067e4e66c3410dbf9d34813c95693b15c1517447ff38a31a9  guix-build-2a541e93cf13/output/x86_64-apple-darwin/bitcoin-2a541e93cf13-x86_64-apple-darwin-unsigned.zip
204a0c2f0f1143d0f3009923f93a6d2b6d06191badfb745cb6c94690a89f2d3e18  guix-build-2a541e93cf13/output/x86_64-apple-darwin/bitcoin-2a541e93cf13-x86_64-apple-darwin.tar.gz
217c9567d96915ddb2d8e8ba690def67c9bec00d9c92a49c170d4d19ac9144dd75  guix-build-2a541e93cf13/output/x86_64-linux-gnu/SHA256SUMS.part
22a90c3107181260340ad87725693d93e651af711287ea635a969c4d286f62de08  guix-build-2a541e93cf13/output/x86_64-linux-gnu/bitcoin-2a541e93cf13-x86_64-linux-gnu-debug.tar.gz
23987643c5065409046eb5897dd53c39aef7e6e34f94db92b2fef4b0fcc6e30823  guix-build-2a541e93cf13/output/x86_64-linux-gnu/bitcoin-2a541e93cf13-x86_64-linux-gnu.tar.gz
246c68dea92932f236291063e94291781f056008fcb1d57dc7a364e4da025e5246  guix-build-2a541e93cf13/output/x86_64-w64-mingw32/SHA256SUMS.part
25a0a0872d90a9fa1bb2f39128d1cc7631e6f8fe74876cdd5e3246da845903113c  guix-build-2a541e93cf13/output/x86_64-w64-mingw32/bitcoin-2a541e93cf13-win64-debug.zip
262a2b98aa2db456da92d9594b142e843abfad63922deb5ea717154b2cc23725be  guix-build-2a541e93cf13/output/x86_64-w64-mingw32/bitcoin-2a541e93cf13-win64-setup-unsigned.exe
27be444f76fada12931e4b41113e528b284a37ae4948e0bfa17e79062783189796  guix-build-2a541e93cf13/output/x86_64-w64-mingw32/bitcoin-2a541e93cf13-win64-unsigned.tar.gz
28b8b67aca5ef4eeceeed11f0f401ddbb2df3d47989a424798a545778250e4ef4f  guix-build-2a541e93cf13/output/x86_64-w64-mingw32/bitcoin-2a541e93cf13-win64.zip
  57. in src/mapport.cpp:271 in 2a541e93cf outdated 
    175@@ -268,7 +176,7 @@ static void DispatchMapPort()
176 
177     assert(g_mapport_thread.joinable());
178     assert(!g_mapport_interrupt);
179-    // Interrupt a protocol-specific loop in the ThreadUpnp() or in the ThreadPCP()
    fanquake commented at 10:50 am on October 25, 2024:
    I think you can just drop this comment entirely, as there is no-longer a “next protocol” in the loop.
    laanwj commented at 1:18 pm on October 25, 2024:
    i think cleaning up a bit more makes sense here, though let’s keep it easy to review refactor-wise
    darosior commented at 7:18 pm on October 25, 2024:
    Done.
  58. in src/mapport.cpp:222 in 2a541e93cf outdated 
    135@@ -219,15 +136,6 @@ static void ThreadMapPort()
136             if (ok) continue;
137         }
138 
139-#ifdef USE_UPNP
    fanquake commented at 10:50 am on October 25, 2024:
    Just up from here, could drop // High priority protocol..
  59. in src/mapport.cpp:176 in 2a541e93cf outdated 
    175@@ -268,7 +176,7 @@ static void DispatchMapPort()
176
    fanquake commented at 10:55 am on October 25, 2024:

    Above here:

    0    if (g_mapport_enabled_protos & g_mapport_current_proto) {
1        // Enabling another protocol does not cause switching from the currently used one.
2        return;
3    }

    This code is either dead, or the comment is outdated, as we can no-longer “enable another protocol”.

    darosior commented at 7:19 pm on October 25, 2024:
    It was both. Removed outdated comments and dead code in the latest push. I cleaned up this function in the follow-up.
  60. fanquake commented at 10:56 am on October 25, 2024: member
    Not sure how much you want to do here, but I think this code can be cleaned up further, given it was written to cater for using two protocols, but after this change, we’ve got 1 protocol (with a fallback).
  61. fanquake added this to the milestone 29.0 on Oct 25, 2024
  62. ci: remove UPnP options 94ad614482
  63. doc: remove mentions of UPnP 953533d021
  64. depends: drop miniupnpc 1b6dec98da
  65. doc: add release note for #31130 b7b2435290
  66. darosior force-pushed on Oct 25, 2024
  67. darosior commented at 1:35 pm on October 25, 2024: member
    I know this can be cleaned up, but i wanted to keep it minimal and focused on dropping UPnP. I’m happy to do a few cleanups (such as the ones you suggest) but i don’t want to start refactoring the whole file. It can always happen in a follow-up if someone is interested and more familiar with this code.
  68. fanquake commented at 1:38 pm on October 25, 2024: member

    I know this can be cleaned up, but i wanted to keep it minimal and focused on dropping UPnP. I’m happy to do a few cleanups (such as the ones you suggest) but i don’t want to start refactoring the whole file.

    Sure, that’s fine, any more-invasive changes could be done later, but we shouldn’t be leaving around dead code, or commentary that is incorrect.

  69. laanwj commented at 1:41 pm on October 25, 2024: member

    i don’t want to start refactoring the whole file. It can always happen in a follow-up if someone is interested and more familiar with this code.

    i agree. Let’s make sense there are no clearly wrong comments, or dead code. But as we’re sure that the only port mapping method we want to keep is PCP/NATPMP, a lot of the logic in this file can be removed and methods can be renamed. But might as well do that in a future follow-up to #30043, as some of those need larger restructures to the loop too (to keep state between runs).

  70. theuni commented at 4:04 pm on October 25, 2024: member

    Getting on the Concept ACK train. Another dependency gone!

    Agree with @laanwj about the refactor as a separate PR.

  71. sipa commented at 4:52 pm on October 25, 2024: member
    Concept ACK. While we don’t actually know how many systems are out there which could be made reachable through UPnP but not through NAT-PMP/PCP, I suspect the number of them actually doing that is very low (given it being default off, and not widely advertized).
  72. edilmedeiros commented at 6:04 pm on October 25, 2024: contributor

    utACK b7b24352906f1dba64826e7a093069b5bfc504dc.

    Agree with @fanquake comments.

  73. DrahtBot requested review from sipa on Oct 25, 2024
  74. DrahtBot requested review from theuni on Oct 25, 2024
  75. DrahtBot requested review from maflcko on Oct 25, 2024
  76. mapport: drop outdated comments 38fdf7c1fb
  77. mapport: remove dead code in DispatchMapPort 
    Since there is now only two options in the MapPortProtoFlag enum, the
four possible combinations of current and enabled are already covered in
the four `if` branches.
    40e5f26a3f
  78. darosior commented at 7:22 pm on October 25, 2024: member
    Pushed two commits to remove outdated comments and dead code. I opened #31157 as a follow-up to clean up some of the logic which became unnecessary or confusing now that there is only a single protocol option.
  79. 1440000bytes approved
  81. laanwj approved
  82. laanwj commented at 12:28 pm on October 27, 2024: member

    Code review ACK 40e5f26a3ff77e50df808f6f850c617aec2df203 i have also checked that:

    • no mentions of miniupnp as a dependency remain
    • -natpmp still works to forward ports on my router after this
    • -upnp, if still present in the configuration file or command line, gives a custom message and exits
  83. i-am-yuvi approved
  84. i-am-yuvi commented at 3:10 pm on October 27, 2024: none

    Tested ACK 40e5f26a3ff77e50df808f6f850c617aec2df203

    • -upnp gives custom messages and exits
    • -natpmp works
  85. jarolrod commented at 3:57 am on October 28, 2024: member

    ACK https://github.com/bitcoin/bitcoin/commit/40e5f26a3ff77e50df808f6f850c617aec2df203

    Tested GUI changes, and GUI changes are sufficient. Error message appears in GUI as well when moving from master with UPNP having been set in the gui, then to this PR branch with same settings.json file.

  86. fanquake referenced this in commit a10acc7d80 on Oct 28, 2024
  87. fanquake commented at 10:42 am on October 28, 2024: member

    Guix Build:

     0541e5a857759e6e34aa8020e70b2b89be7cadfb358aecb0d83c10df214c5b1f7  guix-build-40e5f26a3ff7/output/aarch64-linux-gnu/SHA256SUMS.part
 13973c1c93d969f4b1a338c734df151db3bdfdca0fcb5800f126890a426d5b35d  guix-build-40e5f26a3ff7/output/aarch64-linux-gnu/bitcoin-40e5f26a3ff7-aarch64-linux-gnu-debug.tar.gz
 24cdeb68207b4f71819618b53f1c463f368ccca2912617945caceb49049f30a80  guix-build-40e5f26a3ff7/output/aarch64-linux-gnu/bitcoin-40e5f26a3ff7-aarch64-linux-gnu.tar.gz
 35da6d6e1ab2ac471dfcaf9276f54d3a81ed951700d78354d9b3431f7954e70f4  guix-build-40e5f26a3ff7/output/arm-linux-gnueabihf/SHA256SUMS.part
 44d8126f08b2474408dbc72c44c557397edaa318b6abc43bbb27fbc0ff9081420  guix-build-40e5f26a3ff7/output/arm-linux-gnueabihf/bitcoin-40e5f26a3ff7-arm-linux-gnueabihf-debug.tar.gz
 5dc04c665dd446426532335a9049e93f739031db7568e6f17a7cb9f45c99d9687  guix-build-40e5f26a3ff7/output/arm-linux-gnueabihf/bitcoin-40e5f26a3ff7-arm-linux-gnueabihf.tar.gz
 64ea112d12ac5c0c527fcef92bf02cba4f56ff75296131eb20b4b28c134460cd4  guix-build-40e5f26a3ff7/output/arm64-apple-darwin/SHA256SUMS.part
 739ebe16080b018d1ddb20c486f66a5e6180da02670a8fcc91dcf2464bbe29e9d  guix-build-40e5f26a3ff7/output/arm64-apple-darwin/bitcoin-40e5f26a3ff7-arm64-apple-darwin-unsigned.tar.gz
 852ed74eb097078de8a8b68233d113ad826f9e937d71081733e4ecd5dcbdf1024  guix-build-40e5f26a3ff7/output/arm64-apple-darwin/bitcoin-40e5f26a3ff7-arm64-apple-darwin-unsigned.zip
 9b5a67473c600ccca0f5416939b9e1eebc95caa624522fede3319a31f0276bba7  guix-build-40e5f26a3ff7/output/arm64-apple-darwin/bitcoin-40e5f26a3ff7-arm64-apple-darwin.tar.gz
109049e160d1cbb7ba3f616ee4ef4078ebbb7580af1feb03f1eedddaf17e3f574c  guix-build-40e5f26a3ff7/output/dist-archive/bitcoin-40e5f26a3ff7.tar.gz
11d1bd12e1450bcd1e513a9b5bf6721d9a91da54db116651eae135f36cfd3bc3c5  guix-build-40e5f26a3ff7/output/powerpc64-linux-gnu/SHA256SUMS.part
1238dacbe2edd48f9b3e54835cb4c15ebc0288abc80ae2e4d2f0376df2c9c64c33  guix-build-40e5f26a3ff7/output/powerpc64-linux-gnu/bitcoin-40e5f26a3ff7-powerpc64-linux-gnu-debug.tar.gz
134170b6b3ac4ee6cd2aa26c7767831469fd83dc2d1fe18145ec425cbcf917a38f  guix-build-40e5f26a3ff7/output/powerpc64-linux-gnu/bitcoin-40e5f26a3ff7-powerpc64-linux-gnu.tar.gz
146aeebc67d7052641176bfb7cd05a3c69b50780e97aebd2793db8bdd0c0a524d3  guix-build-40e5f26a3ff7/output/riscv64-linux-gnu/SHA256SUMS.part
15393e5c2429d78c3fc9183a6cb8b9c56c98b01d45a8a85bd14d2ff1f73088626d  guix-build-40e5f26a3ff7/output/riscv64-linux-gnu/bitcoin-40e5f26a3ff7-riscv64-linux-gnu-debug.tar.gz
160103ad4699282b908cbba54afd82828ca9b30a8e13f447a4a6c2c50275835203  guix-build-40e5f26a3ff7/output/riscv64-linux-gnu/bitcoin-40e5f26a3ff7-riscv64-linux-gnu.tar.gz
17c4c66663a8280577e0f116eaa42522d93a8535144018d9eb40f7a8b96bff636c  guix-build-40e5f26a3ff7/output/x86_64-apple-darwin/SHA256SUMS.part
1875e0e43cdb0842c57f4b58ba100f8f5270137c236ac9c83c0586a8041df90729  guix-build-40e5f26a3ff7/output/x86_64-apple-darwin/bitcoin-40e5f26a3ff7-x86_64-apple-darwin-unsigned.tar.gz
198f23f92e1e542c5c5c933ef6e8fad89fe0a667946d8630dbbc9090425aa4b605  guix-build-40e5f26a3ff7/output/x86_64-apple-darwin/bitcoin-40e5f26a3ff7-x86_64-apple-darwin-unsigned.zip
20f8b109eb63bd1cb37a18f664a123c3dce206394f3649510330efab33e0e2bdfd  guix-build-40e5f26a3ff7/output/x86_64-apple-darwin/bitcoin-40e5f26a3ff7-x86_64-apple-darwin.tar.gz
219b38c99dedbc0f7841587532cb7ccaf899006f9fcfe0f7bd35999aa6775bc41d  guix-build-40e5f26a3ff7/output/x86_64-linux-gnu/SHA256SUMS.part
2265efd9a3fd967069db09c2e705449b8223809ac1d4e1f62f312eaeb059122ee6  guix-build-40e5f26a3ff7/output/x86_64-linux-gnu/bitcoin-40e5f26a3ff7-x86_64-linux-gnu-debug.tar.gz
234a9439f81cb98dc3943d8bc3c14dd3fa96dafe20401b8c8be97ee97ee0bd996c  guix-build-40e5f26a3ff7/output/x86_64-linux-gnu/bitcoin-40e5f26a3ff7-x86_64-linux-gnu.tar.gz
2452a47e5908b85a1324e5c2d90dd55c188cb0712495a32801bd201adab6b4490f  guix-build-40e5f26a3ff7/output/x86_64-w64-mingw32/SHA256SUMS.part
25186521473489789e6e2cd3bb4f9c3e901168282305b99c9e66741caffa8db4d8  guix-build-40e5f26a3ff7/output/x86_64-w64-mingw32/bitcoin-40e5f26a3ff7-win64-debug.zip
26ceb9204a1c643b1aee480ed5fb24da436713753fa1ff966124174908743db342  guix-build-40e5f26a3ff7/output/x86_64-w64-mingw32/bitcoin-40e5f26a3ff7-win64-setup-unsigned.exe
27f816302f2023d42df33fd3896f380c4734d2333ff7e8bb06004926aabeb25128  guix-build-40e5f26a3ff7/output/x86_64-w64-mingw32/bitcoin-40e5f26a3ff7-win64-unsigned.tar.gz
28440d575cc18508ad40e9959ca5e479216232a76ecd46dc96d3a7528dee145475  guix-build-40e5f26a3ff7/output/x86_64-w64-mingw32/bitcoin-40e5f26a3ff7-win64.zip
  88. fanquake merged this on Oct 28, 2024
  89. fanquake closed this on Oct 28, 2024
  90. fanquake referenced this in commit d7b64152a9 on Oct 28, 2024
  91. darosior deleted the branch on Oct 28, 2024
  92. Sjors commented at 7:16 pm on October 30, 2024: member

    The GUI needs to handle this more gracefully:

    A user would have to figure out that they need to manually edit settings.json or delete it and redo all their settings.

    We should probably automatically delete it from settings.json. And then display a warning / info (instead of error) that tells users to consider the PCP setting in the GUI.

  93. jarolrod commented at 7:18 pm on October 30, 2024: member

    A user would have to figure out that they need to manually edit settings.json or delete it and redo all their settings.

    We should probably automatically delete it from settings.json. And then display a warning / info (instead of error) that tells users to consider the PCP setting in the GUI.

    I posted about this on irc but no one commented on it :(

    GUI should have option to remove the setting.

  94. laanwj commented at 7:43 pm on October 30, 2024: member

    We should probably automatically delete it from settings.json. And then display a warning / info (instead of error) that tells users to consider the PCP setting in the GUI.

    I posted about this on irc but no one commented on it :(

    Yes. Please create a new issue for this

  95. Sjors commented at 9:57 pm on October 30, 2024: member
    Done in https://github.com/bitcoin-core/gui/issues/843


darosior DrahtBot TheCharlatan hebasto laanwj theStack stickies-v kristapsk kevkevinpal fjahr Christewart edilmedeiros sipa jarolrod 1440000bytes pythcoiner i-am-yuvi maflcko fanquake theuni Sjors


theStack theuni sipa fjahr maflcko TheCharlatan stickies-v

Labels
Build system P2P

Milestone
29.0

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: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me