-
Notifications
You must be signed in to change notification settings - Fork 37.7k
upnp: fix build with miniupnpc 2.2.8 #30283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
Whoops, fixed typo. s/2.1.8/2.2.8/ in several places. |
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 |
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. |
We faced a similar API-breaking change in libevent. See #23606. |
looks like this is the diff between verison 2.2.7 and 2.2.8 in case anyone is looking |
Concept ACK - API fix without a bump seems fine.
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.
Dropped the bump (will PR that separately) and rebased. |
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 |
reACK 8acdf66 |
Guix hashes (match):
|
@@ -161,8 +161,11 @@ static bool ProcessUpnp() | |||
struct UPNPUrls urls; | |||
struct IGDdatas data; | |||
int r; | |||
|
|||
#if MINIUPNPC_API_VERSION <= 17 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8acdf66
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
Backport for 27.x in #30305. |
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
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
Backported to 26.x in #30319. |
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
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
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
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
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>
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
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
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`
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
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
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
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
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
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
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.