scripted-diff: Rename `Sock::{RECV,SEND,ERR}` #35588

hebasto commented at 11:48 AM on June 23, 2026: member

The ERR macro is defined on illumos-based systems in the regset.h header included by the Boost.Test framework, which causes a compilation error:

on OmniOS:

[484/792] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o
FAILED: [code=1] src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o 
/usr/bin/g++ -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/src -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/univalue/include -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/minisketch/include -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/secp256k1/include -isystem /usr/gnu/include -pthread -O2 -g -std=c++20 -fno-extended-identifiers -fmacro-prefix-map=/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src=. -fstack-reuse=none -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -Werror -MD -MT src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o -c /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp 
In file included from /usr/include/sys/procfs_isa.h:39,
                 from /usr/include/sys/procfs.h:66,
                 from /usr/include/procfs.h:45,
                 from /usr/local/include/boost/test/impl/debug.ipp:85,
                 from /usr/local/include/boost/test/included/unit_test.hpp:20,
                 from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp:10:
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/sock.h:162:28: error: expected unqualified-id before numeric constant
  162 |     static constexpr Event ERR = 0b100;
      |                            ^~~

on OpenIndiana:

[44](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958536/job/82891036242#step:7:545)
[ 59%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o
[ 59%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/addrman_tests.cpp.o
In file included from /usr/include/sys/procfs_isa.h:39,
                 from /usr/include/sys/procfs.h:66,
                 from /usr/include/procfs.h:45,
                 from /usr/include/boost/test/impl/debug.ipp:85,
                 from /usr/include/boost/test/included/unit_test.hpp:20,
                 from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp:10:
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/sock.h:162:28: error: expected unqualified-id before numeric constant
  162 |     static constexpr Event ERR = 0b100;
      |                            ^~~

StatusLevel::{INFO,WARN,ERR} has also been renamed to ensure future-proofing and consistency.

hebasto added the label Refactoring on Jun 23, 2026

DrahtBot commented at 11:49 AM on June 23, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35588.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited, maflcko, hodlinator
Stale ACK	w0xlt, davidgumberg

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35105 (Refactor: Updated TransactionError to use util::Expected and removed TransactionError:OK by kevkevinpal)

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.

hebasto commented at 12:33 PM on June 23, 2026: member

See this PR being tested on the affected systems here: https://github.com/hebasto/bitcoin-core-nightly/pull/276.

hebasto commented at 8:08 PM on June 24, 2026: member

git bisect points to f5bc018948f389559ccfaca128b07c9b17b3218b from #35182. Specifically, this line is the culprit: https://github.com/bitcoin/bitcoin/blob/f5bc018948f389559ccfaca128b07c9b17b3218b/src/test/util/setup_common.h#L16

This can be reproduced with the following patch on top of the master branch @ 08a25a7231341e7da54eae468d7411c2c85ced19:

--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -18,6 +18,7 @@
 #include <util/chaintype.h> // IWYU pragma: export
 #include <util/fs.h>
 #include <util/signalinterrupt.h>
+#include <util/sock.h>
 #include <util/vector.h>
 
 #include <cstddef>

in src/util/sock.h:151 in e1bc1e2342

 147 | @@ -148,25 +148,25 @@ class Sock
 148 |      /**
 149 |       * If passed to `Wait()`, then it will wait for readiness to read from the socket.
 150 |       */
 151 | -    static constexpr Event RECV = 0b001;
 152 | +    static constexpr Event BITCOIN_RECV = 0b001;

maflcko commented at 8:19 PM on June 24, 2026:

Not sure if we want to prefix all compile-time constants in this codebase with BITCOIN_ this seems verbose and violates https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum5-dont-use-all_caps-for-enumerators

Why not use PascalCase or so?

maflcko commented at 8:24 PM on June 24, 2026:

Also, the dev notes seem to imply that the core guidelines say that only snake_case is allowed, which is not true?

Could make sense to update the dev notes to be more accurate:

diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index 962e885118..1e332116d4 100644
--- a/doc/developer-notes.md
+++ b/doc/developer-notes.md
@@ -41,8 +41,8 @@ code.
     - Global variables have a `g_` prefix.
   - Constant names are all uppercase, and use `_` to separate words.
-  - Enumerator constants may be `snake_case`, `PascalCase` or `ALL_CAPS`.
-    This is a more tolerant policy than the [C++ Core
+  - Enumerator constants may be `snake_case`, or `PascalCase`. They should not
+    be `ALL_CAPS`, according to the [C++ Core
     Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#renum-caps),
-    which recommend using `snake_case`.  Please use what seems appropriate.
+    to avoid clashing with macros.
   - Class names, function names, and method names are UpperCamelCase
     (PascalCase). Do not prefix class names with `C`. See [Internal interface
@@ -887,6 +887,6 @@ Foo(vec);
 ```cpp
 enum class Tabs {
-    info,
-    console,
+    Info,
+    Console,
 };
 
@@ -895,6 +895,6 @@ int GetInt(Tabs tab)
     int ret = [&]() {
         switch (tab) {
-        case Tabs::info: return 0;
-        case Tabs::console: return 1;
+        case Tabs::Info: return 0;
+        case Tabs::Console: return 1;
         } // no default case, so the compiler can warn about missing cases
         assert(false);

hodlinator commented at 8:28 PM on June 24, 2026:

- ... The should not
+ ... They should not

hebasto commented at 9:02 PM on June 24, 2026:

Why not use PascalCase or so?

Some PascalCase names are already in use: https://github.com/bitcoin/bitcoin/blob/633044f14362108b782b37408385b6e024eaa41b/src/util/sock.h#L73 https://github.com/bitcoin/bitcoin/blob/633044f14362108b782b37408385b6e024eaa41b/src/util/sock.h#L79

hebasto commented at 9:15 PM on June 24, 2026:

Thanks! Reworked.

davidgumberg commented at 11:57 PM on June 24, 2026:

Lucky in this case that these happen to be enumerator constants so developer-notes.md allows PascalCase, but ALL_CAPS for non-enumeration constants pose exactly the same danger, and are also disrecommended by the CppCoreGuidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es9-avoid-all_caps-names

- Constant names are all uppercase, and use `_` to separate words.

This rule from the developer notes should probably be loosened to allow PascalCase for situations like this, and maybe (I'm less sure of this) encourage new constants to avoid ALL_CAPS.

hebasto commented at 11:48 AM on June 25, 2026:

Also, the dev notes seem to imply that the core guidelines say that only snake_case is allowed, which is not true?

Good point. I'd be happy to see those Developer Notes clarified in a separate PR.

hebasto force-pushed on Jun 24, 2026

DrahtBot added the label CI failed on Jun 24, 2026

DrahtBot removed the label CI failed on Jun 24, 2026

davidgumberg commented at 5:39 PM on June 25, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/780d4d1edbf8b5c107c3263bca39030ae1c86ff1

<details> <summary> Sanity checked that scripted diff produces the commit here. </summary>

$ git checkout --detach 780d4d1^
HEAD is now at 633044f143 Merge bitcoin/bitcoin#35266: rpc, wallet: add an option to not load the wallet after migrating
$ ren1() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/util/sock.* ) ; }
$ ren1 RECV RecvEvent
$ ren1 SEND SendEvent
$ ren1 ERR  ErrorEvent

$ ren2() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
$ ren2 Sock::RECV Sock::RecvEvent
$ ren2 Sock::SEND Sock::SendEvent
$ ren2 Sock::ERR  Sock::ErrorEvent

$ git diff 780d4d1
 [ no output ]

w0xlt commented at 8:26 PM on June 25, 2026: contributor

ACK 780d4d1edbf8b5c107c3263bca39030ae1c86ff1

in src/util/sock.cpp:83 in 780d4d1edb

  81 |  
  82 |      std::unique_ptr<Sock> sock;
  83 |  
  84 |      const auto socket = accept(m_socket, addr, addr_len);
  85 | -    if (socket != ERR) {
  86 | +    if (socket != ErrorEvent) {

hodlinator commented at 1:10 PM on June 26, 2026:

Would prefer having a distinct name for this local error code constant - it is unrelated to the Sock::ErrorEvent = 0b100 constant and is not an event. Could be done in a separate commit ahead of the scripted diff.

--- a/src/util/sock.cpp
+++ b/src/util/sock.cpp
@@ -72,15 +72,15 @@ int Sock::Listen(int backlog) const
 std::unique_ptr<Sock> Sock::Accept(sockaddr* addr, socklen_t* addr_len) const
 {
 #ifdef WIN32
-    static constexpr auto ERR = INVALID_SOCKET;
+    static constexpr auto accept_error = INVALID_SOCKET;
 #else
-    static constexpr auto ERR = SOCKET_ERROR;
+    static constexpr auto accept_error = SOCKET_ERROR;
 #endif
 
     std::unique_ptr<Sock> sock;
 
     const auto socket = accept(m_socket, addr, addr_len);
-    if (socket != ERR) {
+    if (socket != accept_error) {
         try {
             sock = std::make_unique<Sock>(socket);
         } catch (const std::exception&) {

hebasto commented at 8:12 PM on June 26, 2026:

Thanks! Taken.

util, refactor: Rename local `ERR` in `Sock::Accept`

The local `static constexpr auto ERR` shadowed the `Sock::ERR` static
data member. Rename it to `accept_error`, per the Developer Notes'
shadowing guidance.

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

7ac25c9177

hebasto force-pushed on Jun 26, 2026

hebasto commented at 8:13 PM on June 26, 2026: member

The feedback from @hodlinator has been addressed.

hodlinator approved

hodlinator commented at 8:26 PM on June 26, 2026: contributor

ACK f7d928fc68a475df9e3ed09c1590255a1053662b

Thanks for accepting my feedback on what should not be included in the scripted diff (https://github.com/bitcoin/bitcoin/pull/35588#discussion_r3481579848)!

Tested by running:

./test/lint/commit-script-check.sh HEAD~~..HEAD

...then editing away part of the scripted diff commit and re-running to verify it fails.

Edit: Oops, accidentally ACKed my edited commit from the test. :)

DrahtBot requested review from davidgumberg on Jun 26, 2026

DrahtBot requested review from w0xlt on Jun 26, 2026

davidgumberg commented at 12:32 AM on June 27, 2026: contributor

reACK https://github.com/bitcoin/bitcoin/commit/f7d928fc68a475df9e3ed09c1590255a1053662b

maflcko commented at 10:14 AM on June 27, 2026: member

not a blocker, since I know the gui is not compiled in your nightly repo, but the same failure can happen there and it could make sense to fix all cases in one go?

src/qt/psbtoperationsdialog.h:        ERR

Edit:

review ACK f7d928fc68a475df9e3ed09c1590255a1053662b 🐪

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK f7d928fc68a475df9e3ed09c1590255a1053662b 🐪
gOKvTK5+us9Dqi4x1Zsa6PyQImRoV6FpE+6EjPWR7eTr2ar5CvXh6gR/MsghfMj7VHMCxnv2w0xdD8PDwITxDA==

</details>

sedited commented at 10:20 AM on June 27, 2026: contributor

Do the docstrings in httpserver.h:336 need updating after this?

scripted-diff: Rename `Sock::{RECV,SEND,ERR}`

The `ERR` macro is defined on illumos-based systems in the `regset.h`
header included by the Boost.Test framework, which causes a compilation
error.

-BEGIN VERIFY SCRIPT-

ren1() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/util/sock.* ./src/httpserver.h ) ; }
ren1 RECV RecvEvent
ren1 SEND SendEvent
ren1 ERR  ErrorEvent

ren2() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren2 Sock::RECV Sock::RecvEvent
ren2 Sock::SEND Sock::SendEvent
ren2 Sock::ERR  Sock::ErrorEvent

-END VERIFY SCRIPT-

f395acdeee

scripted-diff: Rename `StatusLevel::{INFO,WARN,ERR}`

The `ERR` macro is defined on illumos-based systems in the `regset.h`
header included by the Boost.Test framework, which may cause a
compilation error.

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/qt/psbtoperationsdialog.* ) ; }
ren StatusLevel::INFO StatusLevel::Info
ren              INFO              Info
ren StatusLevel::WARN StatusLevel::Warn
ren              WARN              Warn
ren StatusLevel::ERR  StatusLevel::Error
ren              ERR               Error

-END VERIFY SCRIPT-

41ceea400e

hebasto force-pushed on Jun 27, 2026

hebasto commented at 10:36 AM on June 27, 2026: member

@maflcko @sedited

Thank you for the review! Your comments have been addressed.

sedited approved

sedited commented at 10:48 AM on June 27, 2026: contributor

ACK 41ceea400e891a9f51d2dbb91f7acca03ac7fc46

Do we care about the similar definitions in the pcp tests?

DrahtBot requested review from maflcko on Jun 27, 2026

DrahtBot requested review from hodlinator on Jun 27, 2026

maflcko commented at 11:21 AM on June 27, 2026: member

Do we care about the similar definitions in the pcp tests?

They are not ERR, so probably fine for now.

review ACK 41ceea400e 💭

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 41ceea400e 💭
8Qmza17bs0z80CmE6Wj0raj3Gmouc6VUnkiDRDx1Qp5K3C4n/muoDZAKqs26vOrtcG1iW4ZH2XNbL3nKHfJcDw==

</details>

hebasto commented at 11:37 AM on June 27, 2026: member

Do we care about the similar definitions in the pcp tests?

They are not ERR, so probably fine for now.

I agree.

hodlinator approved

hodlinator commented at 5:59 PM on June 27, 2026: contributor

re-ACK 41ceea400e891a9f51d2dbb91f7acca03ac7fc46

Only remaining result in any C/C++-files for git grep -E "\bERR\b" is a string in src/secp256k1/src/bench.h which the preprocessor is probably able to skip over anyway.

sedited merged this on Jun 28, 2026

sedited closed this on Jun 28, 2026

Kino1994 referenced this in commit 084c3b09b7 on Jun 28, 2026

Kino1994 referenced this in commit 1a39fbe8ba on Jun 28, 2026

yuvicc referenced this in commit 75e21edcc5 on Jun 29, 2026

hebasto deleted the branch on Jun 29, 2026