💬 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.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2232977932)
Rebased to include the latest interface changes and to take advantage of mocked sockets in #30205.

📝 fanquake opened a pull request: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/30467)
Backports:
* https://github.com/bitcoin/bitcoin/pull/29855
* https://github.com/bitcoin/bitcoin/pull/30357
* https://github.com/bitcoin/bitcoin/pull/30394 (modified test commit)
* https://github.com/bitcoin/bitcoin/pull/30435

💬 fanquake commented on pull request "init: change shutdown order of load block thread and scheduler":
(https://github.com/bitcoin/bitcoin/pull/30435#issuecomment-2232979945)
Backported to 27.x in #30467.

💬 fanquake commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2232980520)
Backported to 27.x in #30467.

💬 fanquake commented on pull request "psbt: Check non witness utxo outpoint early":
(https://github.com/bitcoin/bitcoin/pull/29855#issuecomment-2232980891)
Backported to 27.x in #30467.

💬 fanquake commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#issuecomment-2232981582)
Backported to 27.x in #30467.

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1680830600)
Can our previous [conversation](https://github.com/hebasto/bitcoin/pull/226#discussion_r1632362329) be helpful?

📝 stratospher opened a pull request: "test: bump mocktime only after node has received and sent bytes"
(https://github.com/bitcoin/bitcoin/pull/30468)
Fixes an intermittent failure for `p2p_v2_misbehaving.py` reported in https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1680462164.

A [different error message](https://github.com/bitcoin/bitcoin/blob/262260ce1e919613ba60194a5861b0b0a51dfe01/src/net.cpp#L1970) `"socket no message in first %i seconds"` will be displayed if `m_last_send=0` or if `m_last_recv is 0`. Fix this by:
1. mocktime bump is done after all the bytes are received. (`m_last_recv is not 0 now`)
2. wait until bytes
...

💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1680837624)
done in https://github.com/bitcoin/bitcoin/pull/30468