💬 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.
(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!
(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!
(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.
(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?
(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
(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?
(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.
(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.
```
(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?
(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.
(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.
(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` ?
(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; }` ?
(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?
(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?
(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
...
(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.
(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;` ?
(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.
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680738999)
Not much value testing the same thing twice.