💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232705519)
Tested with the steps I provided.

tested ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c 🎨

<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=
trus
...

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232711655)
> In any case, creating the symlinks seems like a good way to test this, no?

Sure thing. The PR description has been updated.

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232722842)
Friendly ping @stickies-v :)

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1680652673)
question: Is there a reason why this needs to change, but the other `os.path.dirname(os.path.realpath(__file__))` and `Path(__file__).parents` are fine?

👍 dergoegge approved a pull request: "refactor: Make m_last_notified_header private"
(https://github.com/bitcoin/bitcoin/pull/30466#pullrequestreview-2182266399)
utACK fa927055dd43dda945396574273a210186beec9f

💬 maflcko commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2232743499)
> which means that 29.0 will be built using CMake.

Could add the 29.0 milestone?

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680658887)
May do if I have to re-touch. This is only called once per process, so shouldn't matter.

💬 fanquake commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1680665264)
CPP flag handling seems inconsistent throughout the CI changes, which is confusing. It'd be good to consistently put all CPPFLAGS into CPPFLAGS, rather than having them sometimes in CXXFLAGS, sometimes in (append) CPPFLAGS, and sometimes spread between the two, i.e here.

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680671825)
Thanks, done!

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680671935)
Thanks, done!

💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2232775166)
> > which means that 29.0 will be built using CMake.
>
> Could add the 29.0 milestone?

Done.

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1680684781)
Other cases point to files that reside in the build directory. Or do you have any particular example?

👍 hodlinator approved a pull request: "blockstorage: XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052#pullrequestreview-2182346341)
re-ACK fa9d4aa9a1a8df6343f615572479db9c8e26b19d

💬 fanquake commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2232843612)
What change in behaviour should be expected here?

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1680748974)
> due to how bitcoin handles non-standard ports, we really want the configured external port

Bitcoin Core had a strong preference to connect to peers that listen on port `8333`. Is this what you mean? That was removed at some point.

💬 brunoerg commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#discussion_r1680749823)
```suggestion
# tolerably) early.
```

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1680755420)
Yeah, but this one uses `os.path.realpath(__file__)` as well (so should be in the srcdir already), no?

With my steps to reproduce, it passes before and after this commit.

Do you have other steps to reproduce?

🤔 ajtowns reviewed a pull request: "logging: Replace LogError and LogWarning with LogAlert"
(https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2182052463)
I think this approach makes sense, though the commit that explicitly adds "Warning" and "Error" prefixes does give me some doubts.

Either way, given we're touching a bunch of log lines, I think we should be thinking about whether they actually make sense to be alerts, so most of the following comments are in that vein.

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680518718)
I think this should probably be `LogDebug` rather than `Alert` messages? They're somewhat remote triggerable (that is, if your system is misconfigured so that an error occurs, the error can occur repeatedly on incoming connections via `AcceptConnection`), and it also gives very little information on how to fix the problem if it occurs.

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680519232)
Also seems like should be `LogDebug` ?