💬 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` ?

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680530382)
This is an internal error, isn't it? Outside of tests, we only invoke `SetAddrLocal()` when we receive the first `VERSION` message, so this is the first time `addrLocal` is touched, and hence it should have `valid=false` and never hit this condition. Couldn't this be replaced by `if (Assume(!addrLocal.IsValid())) { addrLocal = addrLocalIn; }` ?

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680704435)
"block index" might be clearer than "value", and could perhaps attempt to decode the block hash from the `key` and report it? Might be worth pointing out why this is a problem at a higher level, ie "block index database corrupted" or similar?

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680721302)
Would probably be better to replace `__func__` in all these messages with something more relevant to potential users (filename, or some more general indication of what's going on). `"block index database corrupted: proof of work check failed for %s\n", pindexNew->ToString()` eg?

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680595996)
I think all the Log statements in this function could be downgraded to `LogDebug(BCLog::PROXY, ...)` -- they just indicate a p2p connection failure? The `LogPrintf("... connect to %s:%d failed ...")` on line 448 generates a bit of noise for nodes with tor enabled, I believe:

```
2024-07-17T05:23:37.590158Z Socks5() connect to nz7rn2ukf3kwqx24iv6q6tyhiclxwzuk65thkihoo4btbdat4y5ee2qd.onion:8333 failed: host unreachable
2024-07-17T05:23:51.908547Z Socks5() connect to nz7rn2ukf3kwqx24iv6q6tyhic
...

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680749763)
Touching all the `LogPrintLevel` warning/error calls without switching them to `LogAlert` seems a shame. A separate scripted-diff prior to "Replace log error and warning..." with:

```sh
sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Error, */LogAlert(/' $files
sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Warning, */LogAlert(/' $files
```

should work, I think.

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680737774)
Should this be an `if (!Assume(redeemScript.size() <= MAX_SCRIPT_ELEMENT_SIZE)) return false;` ?

💬 ajtowns commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680738999)
Not much value testing the same thing twice.

💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1680773490)
> 3 is the peer timeout, so you'll have to change it to bump 2 here. Otherwise, the test will fail:

I didn't follow. The inactivity check trigger runs only when we bump by > 3 and not when the bump = 3 right.
I tried logging [`return node.m_connected + m_peer_connect_timeout < now;` condition in `ShouldRunInactivityChecks()`:](https://github.com/bitcoin/bitcoin/blob/6f9db1ebcab4064065ccd787161bf2b87e03cc1f/src/net.cpp#L1957)

```
[net] [net.cpp:1968] [ShouldRunInactivityChecks] [net] Inac
...

👍 stickies-v approved a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457#pullrequestreview-2182506187)
ACK fa6390df205513319f28e35e3e17c40ecaa7d731

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

For example, when building for macOS, the `CMAKE_SYSTEM_VERSION` value is used to define `DARWIN_MAJOR_VERSION` and `DARWIN_MINOR_VERSION`, which in turn affect the default build options.

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1680816119)
I took the patch.