👍 hebasto approved a pull request: "test: fixing failing system_tests/run_command under some Locales"
(https://github.com/bitcoin/bitcoin/pull/30788#pullrequestreview-2279109392)
ACK ae48a22a3df086fb59843b7b814619ed5df7557b, tested on Ubuntu 24.04 by switching locale.

https://github.com/bitcoin/bitcoin/pull/30788#issue-2500949706

> the run_command test under system_tests fails if the locale is anything other than US/UK/C (English ones) because it checks for things like `result.find_value("success");`
>
> To prevent this, a `setenv("LC_ALL", "C", 1);` is added to make sure that the tests are being run under C locale even if something else is set in the terminal.
...

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2328028855)
The commit description looks fine, so why not copy-paste it and replace the pull request description?

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2328029615)
review ACK ae48a22a3df086fb59843b7b814619ed5df7557b

💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328044971)
Could include https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742931121 to avoid having to open another pull for that?

📝 hebasto opened a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)
Closes https://github.com/bitcoin/bitcoin/issues/30799:

```
$ ./build/src/bitcoind -logsourcelocations -asmap=/tmp/no_file 2>&1 | head -1
2024-09-04T07:01:03Z [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v28.99.0-b94fe85fbe2f (release build)
```

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743189780)
Fixed in https://github.com/bitcoin/bitcoin/pull/30803.

💬 hebasto commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328087131)
> Could include [#30454 (comment)](https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742931121) to avoid having to open another pull for that?

Sure! Included.

💬 maflcko commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#discussion_r1743191660)
This isn't the only reason. The other is ccache, see commit 7abac98d3e3c1bc8ad66cb5c05184b9c5cc674d5. It would be good to mention this.

Ref: https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-ffile-prefix-map

💬 jadijadi commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2328091025)
> The commit description looks fine, so why not copy-paste it and replace the pull request description?

just did. thanks.

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743195595)
> I'd rather not include Wine installation instructions in this build doc. It is not a trivial process and could create an additional maintenance burden in the future.

Not sure I agree. Can you elaborate on this "burden"? Why is that better to avoid than running the tests?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743199107)
@fanquake I am not sure how useful it is to run the tests in the build environment on Linux on Wine, when the binaries itself are meant to be run on a completely different platform (Windows). It would be better to run the tests on the actual platform that will later run the binaries instead. If the docs don't mention it, they can be adjusted to say so.

💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328105897)
lgtm ACK cc9d65dc05f802aaaa108a2edcf30c3758c3f459

Let's wait for the outcome of the discussions in https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726684296 and https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1723106474 as well?

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743211010)
> Can you elaborate on this "burden"?

Wine config creation? Need of the `wine-binfmt` package? Peoples request to fix their Wine installations?

💬 hebasto commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#discussion_r1743220926)
Sure! The comment has been extended.

💬 maflcko commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2328142791)
According to https://reproducible-builds.org/docs/build-path/ "-ffile-prefix-map=OLD=NEW is an alias for both -fdebug-prefix-map and -fmacro-prefix-map. (available since GCC 8 and Clang 10)". Ref https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-ffile-prefix-map and https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffile-prefix-map

review ACK 93a0c2da34b45aaf0aab23c5f1b66cb32df06507

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2328178587)
`08c9a8254a...ed7040a9da`: rebase due to conflicts

💬 0xB10C commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2328268499)
> Could also look into adding more transaction details (e.g. orphan expiration time, and potentially other tx information similar to what's provided in getrawmempool or getrawtransaction).

> I think it would be helpful for visualizing / trying out different orphan protection methods.

Concept ACK

I agree that this would be interesting. The the orphan pool is mostly a black box to me (and probably others too). To discuss improvements/changes, having a better visual and mental model would
...

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743326932)
Seems like a better reason to not do this is that it doesn't actually work properly with CTest? Did this work when you tested it previously? i.e:
```bash
ctest --test-dir build -R mempool_tests --rerun-failed --output-on-failure
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 53: mempool_tests
1/1 Test #53: mempool_tests ....................***Failed 0.01 sec
/bitcoin/build/src/test/test_bitcoin.exe: 1: MZ????@???: not found
/bitcoin/build/
...

💬 fanquake commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328293922)
Let's also wait for https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743326932. To clarify if this ever worked (and was later broken?), or was just never expected to work. In either case, it could be worth documenting.

💬 fanquake commented on pull request "fuzz: Rename fuzz_seed_corpus to fuzz_corpora":
(https://github.com/bitcoin/bitcoin/pull/30804#issuecomment-2328307469)
cc @murchandamus @dergoegge