Skip to content

Conversation

theuni
Copy link
Member

@theuni theuni commented Jun 13, 2024

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 13, 2024

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.

@theuni theuni changed the title upnp: fix build with miniupnpc 2.1.8 upnp: fix build with miniupnpc 2.2.8 Jun 13, 2024
@theuni
Copy link
Member Author

theuni commented Jun 13, 2024

Whoops, fixed typo. s/2.1.8/2.2.8/ in several places.

@edilmedeiros
Copy link
Contributor

ACK bf1a6ee.

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?

@edilmedeiros
Copy link
Contributor

Not ACKing a6df34d, 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.

@hebasto
Copy link
Member

hebasto commented Jun 16, 2024

We faced a similar API-breaking change in libevent. See #23606.

@kevkevinpal
Copy link
Contributor

looks like this is the diff between verison 2.2.7 and 2.2.8 in case anyone is looking
miniupnp/miniupnp@miniupnpc_2_2_7...miniupnpc_2_2_8

@fanquake
Copy link
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.

See: miniupnp/miniupnp@c0a50ce

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.
@theuni
Copy link
Member Author

theuni commented Jun 18, 2024

Dropped the bump (will PR that separately) and rebased.

@fanquake
Copy link
Member

Guix Build (x86_64):

2f879df88862b94c211670a451f442eb951a6ec963d841be683d1ee7c14cf49a  guix-build-8acdf6654083/output/aarch64-linux-gnu/SHA256SUMS.part
00f163274f35e955eb5ef00cb1e6e81e828b3b7415d15f594ea61bc921c000b9  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu-debug.tar.gz
e83b5155ab258164204fd117f7974af90b580c2f0f0da713a1066eb2caecf651  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu.tar.gz
21ac9b233e814a13931141abb3d80d7d218c4f22e6dc453b7a7f96dfae58f71f  guix-build-8acdf6654083/output/arm-linux-gnueabihf/SHA256SUMS.part
117bb44719643aab2d116256881d93b955b3806ec24ecd3d262479a7ecf44bc0  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf-debug.tar.gz
9998ce55de968a995c01d52d1ff95768c07e19adbd3637398b4d108138db17e2  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf.tar.gz
e39e0ae7422e36ec661baaae88b3e8f94fdbc84c86589bc2bb9de12eec9c3463  guix-build-8acdf6654083/output/arm64-apple-darwin/SHA256SUMS.part
1ff207fffbb12acd885a62e9ffa3285d17379abf434210e203f5edf0c1f98644  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.tar.gz
33ae8779d7ed761aebcd6876ee22efc0d28a6fc2453a7b8e537215ea9036da8f  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.zip
871f47fc16afe5ae9641f0c7261f065fd7ca88f0502d72c3301756e81ae61346  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin.tar.gz
2349360ada6c7c413ff34194ef5d5623587cef3f7ffe1e1fd875d67fe9d67bda  guix-build-8acdf6654083/output/dist-archive/bitcoin-8acdf6654083.tar.gz
400cf4606947fde22e8899df95b60000edb9d83b8cf563d574aacba4ad2db168  guix-build-8acdf6654083/output/powerpc64-linux-gnu/SHA256SUMS.part
8a6d5cd452dc5f633c497d2d0f8f33e696f57956a1de568eced15d384bea8230  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu-debug.tar.gz
b9a95dcdfef38d7e4908f865857fdb51c62c98df9160d59e376979f5397b7607  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu.tar.gz
165aab2780d8178cfa1712d84342097d0b8f0a3f904df225e4f1440b4e7c76d9  guix-build-8acdf6654083/output/riscv64-linux-gnu/SHA256SUMS.part
63ab3a5b82f8445cda82c31ef556f93130fa943428c3814980df3b3a5ed2d61c  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu-debug.tar.gz
9fdc0b9782fdf25332ea579af591e2032043be19b7606a672a122d310ffdacbd  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu.tar.gz
6980fe832934fe3c68700928b00f468e5ed36e24308527ea3c82bda39ae651c4  guix-build-8acdf6654083/output/x86_64-apple-darwin/SHA256SUMS.part
650cb9be70ffd879e2a637f8f69fd4094428dff3082212b8c531126804b0d77b  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.tar.gz
ea19280d6b22726621bf09a4b39081a1192281eed2334ffe46c31d7abe8a203d  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.zip
38450f34e96a3b4618e5280ac826c00e112e05a277904ba70bfcba86fe61e49c  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin.tar.gz
e5573facf4ef496236aebe7391c50638effe6b909e441a399a1fce2627af17be  guix-build-8acdf6654083/output/x86_64-linux-gnu/SHA256SUMS.part
bd4cbc3c8346a986152f401abbb4a37f2719c8f4ce243b1525180216673c162e  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu-debug.tar.gz
035d5dc702930895336c9bdc83db21e1e58c0694ff3a5806d011bd91fb99cbed  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu.tar.gz
087b16565cc2a7697ce0cea4a391d1fb955e5e81db04b6ff11bce67fed08c9a7  guix-build-8acdf6654083/output/x86_64-w64-mingw32/SHA256SUMS.part
d2a6465f9736b03cd0193d25dbbfd70d8df4918243a4c2a15c798e8191b68c4c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-debug.zip
a4c4504390a9d66dc99df2986a63891c4f0de59a86455e860a16a1392f88a16c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-setup-unsigned.exe
8769b233abc41a69043869aeee94127b1bf70dad2cdfd707b6712c236bcb517b  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-unsigned.tar.gz
2ae339265c959ab0635414dcdba0886fcf872436cff49047faf8269ef6666d51  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64.zip

@edilmedeiros
Copy link
Contributor

reACK 8acdf66

@DrahtBot DrahtBot requested a review from fanquake June 18, 2024 16:59
@Sjors
Copy link
Member

Sjors commented Jun 19, 2024

Guix hashes (match):

2f879df88862b94c211670a451f442eb951a6ec963d841be683d1ee7c14cf49a  guix-build-8acdf6654083/output/aarch64-linux-gnu/SHA256SUMS.part
00f163274f35e955eb5ef00cb1e6e81e828b3b7415d15f594ea61bc921c000b9  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu-debug.tar.gz
e83b5155ab258164204fd117f7974af90b580c2f0f0da713a1066eb2caecf651  guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu.tar.gz
21ac9b233e814a13931141abb3d80d7d218c4f22e6dc453b7a7f96dfae58f71f  guix-build-8acdf6654083/output/arm-linux-gnueabihf/SHA256SUMS.part
117bb44719643aab2d116256881d93b955b3806ec24ecd3d262479a7ecf44bc0  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf-debug.tar.gz
9998ce55de968a995c01d52d1ff95768c07e19adbd3637398b4d108138db17e2  guix-build-8acdf6654083/output/arm-linux-gnueabihf/bitcoin-8acdf6654083-arm-linux-gnueabihf.tar.gz
e39e0ae7422e36ec661baaae88b3e8f94fdbc84c86589bc2bb9de12eec9c3463  guix-build-8acdf6654083/output/arm64-apple-darwin/SHA256SUMS.part
1ff207fffbb12acd885a62e9ffa3285d17379abf434210e203f5edf0c1f98644  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.tar.gz
33ae8779d7ed761aebcd6876ee22efc0d28a6fc2453a7b8e537215ea9036da8f  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin-unsigned.zip
871f47fc16afe5ae9641f0c7261f065fd7ca88f0502d72c3301756e81ae61346  guix-build-8acdf6654083/output/arm64-apple-darwin/bitcoin-8acdf6654083-arm64-apple-darwin.tar.gz
2349360ada6c7c413ff34194ef5d5623587cef3f7ffe1e1fd875d67fe9d67bda  guix-build-8acdf6654083/output/dist-archive/bitcoin-8acdf6654083.tar.gz
400cf4606947fde22e8899df95b60000edb9d83b8cf563d574aacba4ad2db168  guix-build-8acdf6654083/output/powerpc64-linux-gnu/SHA256SUMS.part
8a6d5cd452dc5f633c497d2d0f8f33e696f57956a1de568eced15d384bea8230  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu-debug.tar.gz
b9a95dcdfef38d7e4908f865857fdb51c62c98df9160d59e376979f5397b7607  guix-build-8acdf6654083/output/powerpc64-linux-gnu/bitcoin-8acdf6654083-powerpc64-linux-gnu.tar.gz
165aab2780d8178cfa1712d84342097d0b8f0a3f904df225e4f1440b4e7c76d9  guix-build-8acdf6654083/output/riscv64-linux-gnu/SHA256SUMS.part
63ab3a5b82f8445cda82c31ef556f93130fa943428c3814980df3b3a5ed2d61c  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu-debug.tar.gz
9fdc0b9782fdf25332ea579af591e2032043be19b7606a672a122d310ffdacbd  guix-build-8acdf6654083/output/riscv64-linux-gnu/bitcoin-8acdf6654083-riscv64-linux-gnu.tar.gz
6980fe832934fe3c68700928b00f468e5ed36e24308527ea3c82bda39ae651c4  guix-build-8acdf6654083/output/x86_64-apple-darwin/SHA256SUMS.part
650cb9be70ffd879e2a637f8f69fd4094428dff3082212b8c531126804b0d77b  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.tar.gz
ea19280d6b22726621bf09a4b39081a1192281eed2334ffe46c31d7abe8a203d  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin-unsigned.zip
38450f34e96a3b4618e5280ac826c00e112e05a277904ba70bfcba86fe61e49c  guix-build-8acdf6654083/output/x86_64-apple-darwin/bitcoin-8acdf6654083-x86_64-apple-darwin.tar.gz
e5573facf4ef496236aebe7391c50638effe6b909e441a399a1fce2627af17be  guix-build-8acdf6654083/output/x86_64-linux-gnu/SHA256SUMS.part
bd4cbc3c8346a986152f401abbb4a37f2719c8f4ce243b1525180216673c162e  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu-debug.tar.gz
035d5dc702930895336c9bdc83db21e1e58c0694ff3a5806d011bd91fb99cbed  guix-build-8acdf6654083/output/x86_64-linux-gnu/bitcoin-8acdf6654083-x86_64-linux-gnu.tar.gz
087b16565cc2a7697ce0cea4a391d1fb955e5e81db04b6ff11bce67fed08c9a7  guix-build-8acdf6654083/output/x86_64-w64-mingw32/SHA256SUMS.part
d2a6465f9736b03cd0193d25dbbfd70d8df4918243a4c2a15c798e8191b68c4c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-debug.zip
a4c4504390a9d66dc99df2986a63891c4f0de59a86455e860a16a1392f88a16c  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-setup-unsigned.exe
8769b233abc41a69043869aeee94127b1bf70dad2cdfd707b6712c236bcb517b  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64-unsigned.tar.gz
2ae339265c959ab0635414dcdba0886fcf872436cff49047faf8269ef6666d51  guix-build-8acdf6654083/output/x86_64-w64-mingw32/bitcoin-8acdf6654083-win64.zip

@@ -161,8 +161,11 @@ static bool ProcessUpnp()
struct UPNPUrls urls;
struct IGDdatas data;
int r;

#if MINIUPNPC_API_VERSION <= 17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I guess given we assert >= 17, this could be = 17, but I'm not sure that is less confusing.

@DrahtBot DrahtBot requested a review from fanquake June 19, 2024 09:03
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8acdf66

@fanquake fanquake merged commit ac4ea78 into bitcoin:master Jun 19, 2024
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 19, 2024
See: miniupnp/miniupnp@c0a50ce

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.

Github-Pull: bitcoin#30283
Rebased-From: 8acdf66
@fanquake fanquake mentioned this pull request Jun 19, 2024
@fanquake
Copy link
Member

fanquake commented Jun 19, 2024

Backport for 27.x in #30305.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 19, 2024
See: miniupnp/miniupnp@c0a50ce

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.

Github-Pull: bitcoin#30283
Rebased-From: 8acdf66
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 21, 2024
See: miniupnp/miniupnp@c0a50ce

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.

Github-Pull: bitcoin#30283
Rebased-From: 8acdf66
@fanquake
Copy link
Member

Backported to 26.x in #30319.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 22, 2024
See: miniupnp/miniupnp@c0a50ce

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.

Github-Pull: bitcoin#30283
Rebased-From: 8acdf66
fanquake added a commit that referenced this pull request Jun 24, 2024
b3093eb doc: Update rel notes for 27.x (fanquake)
6338f92 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)
f34e446 ci: remove unused bcc variable from workflow (Max Edwards)
0d524b1 ci: move Asan / LSan / USDT job to Github Actions (Max Edwards)
43c40dd ci: add IPV6 network to ci container (Max Edwards)

Pull request description:

  Backports:
  * #30193
  * #30283
  * #30299

ACKs for top commit:
  willcl-ark:
    ACK b3093eb
  stickies-v:
    ACK b3093eb

Tree-SHA512: 325149f2b388072276e10fae2ebb7d8f3f5138d75f237c0182a09c631334fc2af9c2fe500db31bf41e94d4f154771e3cd386f8eb0d09d7a1ad656f637b71e735
fanquake added a commit that referenced this pull request Jun 24, 2024
10413ac doc: add 30283 cherry-pick to rel notes (fanquake)
391ce77 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)

Pull request description:

  Backports #30283 to the 26.x branch.

ACKs for top commit:
  edilmedeiros:
    ACK 10413ac
  theuni:
    ACK 10413ac

Tree-SHA512: 4a0f4eceefd5bbf9c97d19c4890b85963d56449856a56e6fe24161d3d6f37332de719da342d6c00ee67f2cd9434d849809a3cdc51719dc93219ec218c35a9f97
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 9, 2024
Summary:
```
See: miniupnp/miniupnp@c0a50ce

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.
```

Backport of [[bitcoin/bitcoin#30283 | core#30283]].

Test Plan:
On Arch:
  ninja

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16434
emilazy added a commit to emilazy/litecoin that referenced this pull request Jul 11, 2024
Based on bitcoin#30283.

See: miniupnp/miniupnp@c0a50ce

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.

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
roqqit pushed a commit to doged-io/doged that referenced this pull request Aug 1, 2024
Summary:
```
See: miniupnp/miniupnp@c0a50ce

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.
```

Backport of [[bitcoin/bitcoin#30283 | core#30283]].

Test Plan:
On Arch:
  ninja

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16434
hebasto added a commit to bitcoin-core/gui-qml that referenced this pull request Sep 27, 2024
52dfa7d upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)

Pull request description:

  This PR backports bitcoin/bitcoin#30283 to resolve the current CI issues with Homebrew's `miniupnpc` package version 2.2.8.

ACKs for top commit:
  MarnixCroes:
    lgtm ACK 52dfa7d
  pablomartin4btc:
    ACK 52dfa7d

Tree-SHA512: 8b2dc204c3c284cdd12f79bb08d47c68052d8b1629b63ab601c30f578ec9866ed86278e6f8facb3dcfd4013cfc1ea8f00da5365b46ccc589f1ea9b543f4d225e
melroy89 pushed a commit to BitcoinCash1/Bitcoin-Cash-Node that referenced this pull request Oct 10, 2024
This is a backport of bitcoin/bitcoin#30283

Summary
---

See: miniupnp/miniupnp@c0a50ce

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.

Test Plan
---

* Check with miniupnpc version 2.2.8 and 2.2.7
* `ninja all check-all`
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request Oct 17, 2024
4fffec0 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)

Pull request description:

  See: miniupnp/miniupnp@c0a50ce

  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.

  ---

  Upstream: bitcoin#30283

ACKs for top commit: 4fffec0
  panleone:
    utACK 4fffec0
  Liquid369:
    tACK 4fffec0

Tree-SHA512: 01adcb1e92badefc5ba0e9ec539aafed6c8aef3133321a1da954dbf99f31964f0250bebf6e85831b6d051362ed4d17f65151144942eab86ea4c7f6ababca37bc
akku1139 added a commit to akku1139/magi that referenced this pull request Oct 18, 2024
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2024
8acdf66 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)

Pull request description:

  Fixes bitcoin#30266

  Miniupnpc 2.2.8 [changed the function signature of `UPNP_GetValidIGD`](miniupnp/miniupnp@c0a50ce#diff-5a0d7cff00628c2c64a617edb347c0f283e3a75e7df910e7e8438fc6db23f610R122) 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.

ACKs for top commit:
  edilmedeiros:
    reACK 8acdf66
  fanquake:
    ACK 8acdf66

Tree-SHA512: d2236ec8aef57a5c879065fbbe20080a14e4bf7b44c0bf506707eb946f72aa5837aba2fb2426d6853d21a9b77db5d72561d29d7ea645714d90309e11fe11d354
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 27, 2024
8acdf66 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)

Pull request description:

  Fixes bitcoin#30266

  Miniupnpc 2.2.8 [changed the function signature of `UPNP_GetValidIGD`](miniupnp/miniupnp@c0a50ce#diff-5a0d7cff00628c2c64a617edb347c0f283e3a75e7df910e7e8438fc6db23f610R122) 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.

ACKs for top commit:
  edilmedeiros:
    reACK 8acdf66
  fanquake:
    ACK 8acdf66

Tree-SHA512: d2236ec8aef57a5c879065fbbe20080a14e4bf7b44c0bf506707eb946f72aa5837aba2fb2426d6853d21a9b77db5d72561d29d7ea645714d90309e11fe11d354
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 27, 2024
b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script)
745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow)
01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow)
432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script)
1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script)
8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script)
f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script)
ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script)
e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script)
df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script)
57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script)
e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script)
62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script)
745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow)
4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov)
69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script)
ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script)
9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow)
479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script)
ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script)
63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script)
3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script)
3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script)

Pull request description:

  ## Issue being fixed or feature implemented
  Trivial backports

  ## What was done?

  ## How Has This Been Tested?
  built locally

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK b654479
  kwvg:
    utACK b654479

Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043
charlesrocket pushed a commit to AXErunners/axe that referenced this pull request Nov 20, 2024
8acdf66540834b9f9cf28f16d389e8b6a48516d5 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)

Pull request description:

  Fixes bitcoin/bitcoin#30266

  Miniupnpc 2.2.8 [changed the function signature of `UPNP_GetValidIGD`](miniupnp/miniupnp@c0a50ce#diff-5a0d7cff00628c2c64a617edb347c0f283e3a75e7df910e7e8438fc6db23f610R122) 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.

ACKs for top commit:
  edilmedeiros:
    reACK 8acdf66540834b9f9cf28f16d389e8b6a48516d5
  fanquake:
    ACK 8acdf66540834b9f9cf28f16d389e8b6a48516d5

Tree-SHA512: d2236ec8aef57a5c879065fbbe20080a14e4bf7b44c0bf506707eb946f72aa5837aba2fb2426d6853d21a9b77db5d72561d29d7ea645714d90309e11fe11d354
tuaris added a commit to tuaris/bellscoinV3 that referenced this pull request Dec 13, 2024
Miniupnpc 2.2.8 changed the function signature of UPNP_GetValidIGD without taking much care with the ABI.

This just copies the same fix from Bitcoin Core: bitcoin/bitcoin#30283
@bitcoin bitcoin locked and limited conversation to collaborators Jun 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Won't compile with miniupnpc 2.2.8
7 participants