Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 26, 2024

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 26, 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 TheCharlatan
Stale ACK pinheadmz, hebasto

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:

  • #29432 (Stratum v2 Template Provider (take 3) by Sjors)
  • #25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)

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.

@maflcko
Copy link
Member Author

maflcko commented Apr 26, 2024

@fanquake Can you cherry-pick this into your pull, to confirm that it also works for you? (I only tested it locally)

@fanquake
Copy link
Member

Pulled it in

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa5ba42, tested on Ubuntu 24.04 with -Wall.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/24297453221

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

ACK fa2353d

Confirmed the warning on master by setting -Wall and then forcing HAVE_SOCKADDR_UN = 0 in configure.ac. This patch fixes the warning in a clean way. I dunno if (void)unix; needs an explanatory comment or if thats a recognizable pattern for this kind of thing.

Thanks again @maflcko for cleaning up my mess 😬

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK fa2353d9c3ff68995e7478b9b80dcdb1fc638a53
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmYxJW0ACgkQ5+KYS2KJ
yTpXqg//VS2W2FATmVSHha7wDo/Kaj3rFAIH9nxlKr9tEeZCda2DCsY0bD8IYi53
1mfvrPhEpwhyv85Rztx35PEnGz7mvwWcL62diicx+qYTmy6DtctwzvRN9SKatF/V
xIVm8aPVHrcczlw30vvturmiZDvP0iFg8n6LpjZZvHmqiPYAQQ/wP63iaaYSdARY
iYb1cV5sZgh80Q6Tiy08/qTUyWxgwE50LXbAeyu7ZPx9yY6jOWxL3LP1o+0ZTkbH
1SWYjrdC2S+NxbMf5PfiT3VezA5Lezk90wZzgUjuPexzZChk1mZTh+XKn4RLTSGy
xqLAu9TPMylmuEsMb70g0YLsZHFdQI2k84xwZwmyXhLN77zWncFNmJiXQbmJoSWv
+BSGg/2tROOHs/KtbY50HVnVv1Fv5FPlg5llRH0EpaeFGlYn7pHt2x61uDjf83Wy
+dQU2Cx+L2AgioC0CD6tQzuSaklweFQw+t8wRVnC8gyS2ftBT99TB7/qCB0h0/vq
lypmobdOofSfEea/i13L91vGxyv7F9rw7SimzbPMPUaYzZs6OA3c9rLbcabPbja/
YdygzdYZKmTUKeExMNjCGyxRp+swIDTPwT3Ca9N00+vN+eVzfIbSDoXaVj7ZWzIO
aSPqUMdCjLiqAJ/AcFSBKFLUTCcg+99rcfA4/kkduQxJaGdNF04=
=Pu5n
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

@DrahtBot DrahtBot requested a review from hebasto April 30, 2024 17:10
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK fa2353d.

@fanquake
Copy link
Member

fanquake commented May 1, 2024

Could be:

diff --git a/src/init.cpp b/src/init.cpp
index 4d7638cd6e..ebfcb9cf8e 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1312,7 +1312,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         {"-zmqpubsequence",         true}
     }) {
         const std::string arg{port_option.first};
-        const bool unix{port_option.second};
+        [[maybe_unused]] const bool unix{port_option.second};
         for (const std::string& socket_addr : args.GetArgs(arg)) {
             std::string host_out;
             uint16_t port_out{0};

Avoids the worry about future compilers, and (void)blah which isn't explanatory

@maflcko
Copy link
Member Author

maflcko commented May 2, 2024

Avoids the worry about future compilers, and (void)blah which isn't explanatory

I think (void)blah; is well understood to mean this (to both code readers and compilers), but [[maybe_unused]] seems fine as well. Switched to that.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK fa9abf9

@DrahtBot DrahtBot requested review from hebasto and pinheadmz May 2, 2024 13:00
@fanquake fanquake merged commit 3d28725 into bitcoin:master May 2, 2024
@maflcko maflcko deleted the 2404-init- branch May 2, 2024 13:10
kwvg added a commit to kwvg/dash that referenced this pull request Apr 19, 2025
kwvg added a commit to kwvg/dash that referenced this pull request Apr 19, 2025
kwvg added a commit to kwvg/dash that referenced this pull request Apr 22, 2025
kwvg added a commit to kwvg/dash that referenced this pull request Apr 22, 2025
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Apr 25, 2025
, bitcoin#29649, bitcoin#27679, bitcoin#29968, partial bitcoin#26312 (UNIX domain sockets support)

10c7c1d merge bitcoin#29968: Avoid unused-variable warning in init.cpp (Kittywhiskers Van Gogh)
82a0fb2 merge bitcoin#27679: Support UNIX domain sockets (Kittywhiskers Van Gogh)
a23d651 merge bitcoin#29649: remove unnecessary log message (Kittywhiskers Van Gogh)
343bf83 merge bitcoin#27375: support unix domain sockets for -proxy and -onion (Kittywhiskers Van Gogh)
0fd83f3 merge bitcoin#28695: Sanity check private keys received from SAM proxy (Kittywhiskers Van Gogh)
6e55ab5 partial bitcoin#26312: Remove Sock::Get() and Sock::Sock() (Kittywhiskers Van Gogh)
859da12 merge bitcoin#22087: Validate port-options (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6630

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] 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 10c7c1d

Tree-SHA512: cf3bb84a34315fd188bf1427f59e8b050a04f26d2d4a152a8477c50daef154b35e0f29e49787ac8b585078e985d7192bc39bde27ea39a3b547c297cb2fc3a2ae
@bitcoin bitcoin locked and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants