upnp: fix build with miniupnpc 2.2.8 #30283

1. 
   theuni commented at 2:02 pm on June 13, 2024: member

   Fixes #30266

   Miniupnpc 2.2.8 changed the function signature of UPNP_GetValidIGD without taking much care with the abi :(

   ~This is the minimal change to cope with that. Also included in this PR is a temporary bump to 2.2.8 to verify that it builds correctly. I’m happy to revert that and discuss the bump separately, as miniupnpc bumps require some scrutiny.~

   I believe that this is problematic if we build against one version and encounter a different one at runtime. This is not a problem for depends because we build statically. But for users who are self-building against shared system libs, care must be taken to run against the same version used for linking.

   Some quick digging shows that at least Ubuntu/Arch make the distinction between soversions: libminiupnpc.so.17 -> libminiupnpc.so.18. So in practice, I suppose this shouldn’t be much of a problem.

   Boooo for the upstream loose abi policy.

2. 
   DrahtBot commented at 2:02 pm on June 13, 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	edilmedeiros, fanquake

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

3. theuni renamed this:
   upnp: fix build with miniupnpc 2.1.8
   upnp: fix build with miniupnpc 2.2.8
   on Jun 13, 2024
4. theuni force-pushed on Jun 13, 2024
5. 
   theuni commented at 2:06 pm on June 13, 2024: member

   Whoops, fixed typo. s/2.1.8/2.2.8/ in several places.
6. theuni force-pushed on Jun 13, 2024
7. 
   edilmedeiros commented at 5:12 pm on June 14, 2024: contributor

   ACK bf1a6eec805b81819654dff4554bda8c95069aca.

   I inspected the upstream change and it looks this will be sufficient to fix the issue.

   I complied successfully against my updated macports package, but I have no use case for it to check functionality. @fanquake I see that you have been updating the bitcoin port in macports. Updating miniupnpc will break it. What do you think? Try to hold it until bitcoin-core v28?

8. 
   edilmedeiros commented at 5:18 pm on June 14, 2024: contributor

   Not ACKing a6df34dfa65560e4d4137c5d8d95f111af5df028, though.

   I have inspected the upstream changes and it doesn’t seem to do anything shady, but I’m not familiar with the full functionality of that package. Since this can potentially open a backdoor, I prefer to let more experienced people check it.

9. 
   hebasto commented at 8:50 am on June 16, 2024: member

   We faced a similar API-breaking change in libevent. See #23606.
10. 
    kevkevinpal commented at 1:31 am on June 18, 2024: contributor

    looks like this is the diff between verison 2.2.7 and 2.2.8 in case anyone is looking https://github.com/miniupnp/miniupnp/compare/miniupnpc_2_2_7...miniupnpc_2_2_8
11. fanquake added the label Needs backport (26.x) on Jun 18, 2024
12. fanquake added the label Needs backport (27.x) on Jun 18, 2024
13. 
    fanquake commented at 12:09 pm on June 18, 2024: member

    Concept ACK - API fix without a bump seems fine.

    fanquake I see that you have been updating the bitcoin port in macports. Updating miniupnpc will break it. What do you think? Try to hold it until bitcoin-core v28?

    If the miniupnpc package gets updated in macports, then pulling in the same patch that ends up being applied here (until it lands in a point release), for the bitcoin port seems like the best approach.

14. upnp: add compatibility for miniupnpc 2.2.8

    See: https://github.com/miniupnp/miniupnp/commit/c0a50ce33e3b99ce8a96fd43049bb5b53ffac62f
    
    The return value of 2 now indicates:
    "A valid connected IGD has been found but its IP address is reserved (non routable)"
    
    We continue to ignore any return value other than 1.

    8acdf66540
15. theuni force-pushed on Jun 18, 2024
16. 
    theuni commented at 12:25 pm on June 18, 2024: member

    Dropped the bump (will PR that separately) and rebased.
17. 
    fanquake commented at 1:39 pm on June 18, 2024: member

    Guix Build (x86_64):

     02f879df88862b94c211670a451f442eb951a6ec963d841be683d1ee7c14cf49a  guix-build-8acdf6654083/output/aarch64-linux-gnu/SHA256SUMS.part
     100f163274f35e955eb5ef00cb1e6e81e828b3b7415d15f594ea61bc921c000b9  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu-debug.tar.gz
     2e83b5155ab258164204fd117f7974af90b580c2f0f0da713a1066eb2caecf651  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu.tar.gz
     321ac9b233e814a13931141abb3d80d7d218c4f22e6dc453b7a7f96dfae58f71f  guix-build-8acdf6654083/output/arm-linux-gnueabihf/SHA256SUMS.part
     4117bb44719643aab2d116256881d93b955b3806ec24ecd3d262479a7ecf44bc0  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf-debug.tar.gz
     59998ce55de968a995c01d52d1ff95768c07e19adbd3637398b4d108138db17e2  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf.tar.gz
     6e39e0ae7422e36ec661baaae88b3e8f94fdbc84c86589bc2bb9de12eec9c3463  guix-build-8acdf6654083/output/arm64-apple-darwin/SHA256SUMS.part
     71ff207fffbb12acd885a62e9ffa3285d17379abf434210e203f5edf0c1f98644  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.tar.gz
     833ae8779d7ed761aebcd6876ee22efc0d28a6fc2453a7b8e537215ea9036da8f  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.zip
     9871f47fc16afe5ae9641f0c7261f065fd7ca88f0502d72c3301756e81ae61346  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin.tar.gz
    102349360ada6c7c413ff34194ef5d5623587cef3f7ffe1e1fd875d67fe9d67bda  guix-build-8acdf6654083/output/dist-archive/bitcoin-8acdf6654083.tar.gz
    11400cf4606947fde22e8899df95b60000edb9d83b8cf563d574aacba4ad2db168  guix-build-8acdf6654083/output/powerpc64-linux-gnu/SHA256SUMS.part
    128a6d5cd452dc5f633c497d2d0f8f33e696f57956a1de568eced15d384bea8230  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu-debug.tar.gz
    13b9a95dcdfef38d7e4908f865857fdb51c62c98df9160d59e376979f5397b7607  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu.tar.gz
    14165aab2780d8178cfa1712d84342097d0b8f0a3f904df225e4f1440b4e7c76d9  guix-build-8acdf6654083/output/riscv64-linux-gnu/SHA256SUMS.part
    1563ab3a5b82f8445cda82c31ef556f93130fa943428c3814980df3b3a5ed2d61c  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu-debug.tar.gz
    169fdc0b9782fdf25332ea579af591e2032043be19b7606a672a122d310ffdacbd  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu.tar.gz
    176980fe832934fe3c68700928b00f468e5ed36e24308527ea3c82bda39ae651c4  guix-build-8acdf6654083/output/x86_64-apple-darwin/SHA256SUMS.part
    18650cb9be70ffd879e2a637f8f69fd4094428dff3082212b8c531126804b0d77b  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.tar.gz
    19ea19280d6b22726621bf09a4b39081a1192281eed2334ffe46c31d7abe8a203d  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.zip
    2038450f34e96a3b4618e5280ac826c00e112e05a277904ba70bfcba86fe61e49c  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin.tar.gz
    21e5573facf4ef496236aebe7391c50638effe6b909e441a399a1fce2627af17be  guix-build-8acdf6654083/output/x86_64-linux-gnu/SHA256SUMS.part
    22bd4cbc3c8346a986152f401abbb4a37f2719c8f4ce243b1525180216673c162e  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu-debug.tar.gz
    23035d5dc702930895336c9bdc83db21e1e58c0694ff3a5806d011bd91fb99cbed  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu.tar.gz
    24087b16565cc2a7697ce0cea4a391d1fb955e5e81db04b6ff11bce67fed08c9a7  guix-build-8acdf6654083/output/x86_64-w64-mingw32/SHA256SUMS.part
    25d2a6465f9736b03cd0193d25dbbfd70d8df4918243a4c2a15c798e8191b68c4c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-debug.zip
    26a4c4504390a9d66dc99df2986a63891c4f0de59a86455e860a16a1392f88a16c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-setup-unsigned.exe
    278769b233abc41a69043869aeee94127b1bf70dad2cdfd707b6712c236bcb517b  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-unsigned.tar.gz
    282ae339265c959ab0635414dcdba0886fcf872436cff49047faf8269ef6666d51  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64.zip
    

18. DrahtBot added the label CI failed on Jun 18, 2024
19. 
    edilmedeiros commented at 4:59 pm on June 18, 2024: contributor

    reACK 8acdf66540834b9f9cf28f16d389e8b6a48516d5
20. DrahtBot requested review from fanquake on Jun 18, 2024
21. DrahtBot removed the label CI failed on Jun 18, 2024
22. 
    Sjors commented at 7:00 am on June 19, 2024: member

    Guix hashes (match):

     02f879df88862b94c211670a451f442eb951a6ec963d841be683d1ee7c14cf49a  guix-build-8acdf6654083/output/aarch64-linux-gnu/SHA256SUMS.part
     100f163274f35e955eb5ef00cb1e6e81e828b3b7415d15f594ea61bc921c000b9  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu-debug.tar.gz
     2e83b5155ab258164204fd117f7974af90b580c2f0f0da713a1066eb2caecf651  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu.tar.gz
     321ac9b233e814a13931141abb3d80d7d218c4f22e6dc453b7a7f96dfae58f71f  guix-build-8acdf6654083/output/arm-linux-gnueabihf/SHA256SUMS.part
     4117bb44719643aab2d116256881d93b955b3806ec24ecd3d262479a7ecf44bc0  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf-debug.tar.gz
     59998ce55de968a995c01d52d1ff95768c07e19adbd3637398b4d108138db17e2  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf.tar.gz
     6e39e0ae7422e36ec661baaae88b3e8f94fdbc84c86589bc2bb9de12eec9c3463  guix-build-8acdf6654083/output/arm64-apple-darwin/SHA256SUMS.part
     71ff207fffbb12acd885a62e9ffa3285d17379abf434210e203f5edf0c1f98644  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.tar.gz
     833ae8779d7ed761aebcd6876ee22efc0d28a6fc2453a7b8e537215ea9036da8f  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.zip
     9871f47fc16afe5ae9641f0c7261f065fd7ca88f0502d72c3301756e81ae61346  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin.tar.gz
    102349360ada6c7c413ff34194ef5d5623587cef3f7ffe1e1fd875d67fe9d67bda  guix-build-8acdf6654083/output/dist-archive/bitcoin-8acdf6654083.tar.gz
    11400cf4606947fde22e8899df95b60000edb9d83b8cf563d574aacba4ad2db168  guix-build-8acdf6654083/output/powerpc64-linux-gnu/SHA256SUMS.part
    128a6d5cd452dc5f633c497d2d0f8f33e696f57956a1de568eced15d384bea8230  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu-debug.tar.gz
    13b9a95dcdfef38d7e4908f865857fdb51c62c98df9160d59e376979f5397b7607  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu.tar.gz
    14165aab2780d8178cfa1712d84342097d0b8f0a3f904df225e4f1440b4e7c76d9  guix-build-8acdf6654083/output/riscv64-linux-gnu/SHA256SUMS.part
    1563ab3a5b82f8445cda82c31ef556f93130fa943428c3814980df3b3a5ed2d61c  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu-debug.tar.gz
    169fdc0b9782fdf25332ea579af591e2032043be19b7606a672a122d310ffdacbd  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu.tar.gz
    176980fe832934fe3c68700928b00f468e5ed36e24308527ea3c82bda39ae651c4  guix-build-8acdf6654083/output/x86_64-apple-darwin/SHA256SUMS.part
    18650cb9be70ffd879e2a637f8f69fd4094428dff3082212b8c531126804b0d77b  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.tar.gz
    19ea19280d6b22726621bf09a4b39081a1192281eed2334ffe46c31d7abe8a203d  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.zip
    2038450f34e96a3b4618e5280ac826c00e112e05a277904ba70bfcba86fe61e49c  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin.tar.gz
    21e5573facf4ef496236aebe7391c50638effe6b909e441a399a1fce2627af17be  guix-build-8acdf6654083/output/x86_64-linux-gnu/SHA256SUMS.part
    22bd4cbc3c8346a986152f401abbb4a37f2719c8f4ce243b1525180216673c162e  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu-debug.tar.gz
    23035d5dc702930895336c9bdc83db21e1e58c0694ff3a5806d011bd91fb99cbed  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu.tar.gz
    24087b16565cc2a7697ce0cea4a391d1fb955e5e81db04b6ff11bce67fed08c9a7  guix-build-8acdf6654083/output/x86_64-w64-mingw32/SHA256SUMS.part
    25d2a6465f9736b03cd0193d25dbbfd70d8df4918243a4c2a15c798e8191b68c4c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-debug.zip
    26a4c4504390a9d66dc99df2986a63891c4f0de59a86455e860a16a1392f88a16c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-setup-unsigned.exe
    278769b233abc41a69043869aeee94127b1bf70dad2cdfd707b6712c236bcb517b  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-unsigned.tar.gz
    282ae339265c959ab0635414dcdba0886fcf872436cff49047faf8269ef6666d51  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64.zip
    

23. in src/mapport.cpp:164 in 8acdf66540

    160@@ -161,8 +161,11 @@ static bool ProcessUpnp()
    161     struct UPNPUrls urls;
    162     struct IGDdatas data;
    163     int r;
    164-
    165+#if MINIUPNPC_API_VERSION <= 17
    

    ---

    ---

    

    fanquake commented at 9:03 am on June 19, 2024:

    nit: I guess given we assert >= 17, this could be = 17, but I’m not sure that is less confusing.
24. DrahtBot requested review from fanquake on Jun 19, 2024
25. fanquake approved
26. 
    fanquake commented at 9:03 am on June 19, 2024: member

    ACK 8acdf66540834b9f9cf28f16d389e8b6a48516d5
27. fanquake merged this on Jun 19, 2024
28. fanquake closed this on Jun 19, 2024

    ---

29. fanquake referenced this in commit ddbc56ee5a on Jun 19, 2024
30. fanquake removed the label Needs backport (27.x) on Jun 19, 2024
31. 
    fanquake commented at 9:13 am on June 19, 2024: member

    Backport for 27.x in #30305.
32. fanquake referenced this in commit 6338f92260 on Jun 19, 2024
33. fanquake referenced this in commit 391ce775f4 on Jun 21, 2024
34. fanquake removed the label Needs backport (26.x) on Jun 21, 2024
35. 
    fanquake commented at 2:51 pm on June 21, 2024: member

    Backported to 26.x in #30319.

Contributors
theuni DrahtBot edilmedeiros hebasto kevkevinpal fanquake Sjors