💬 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

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743353913)
I don't recall when wine was ever working. There was always a a number of intermittent or consistent errors, regardless of cmake, see https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632851441

Fixing that seems unrelated to the cmake transition.

I think simply removing the accidentally added line is fine and other follow-ups can be done in a follow-up, if they are needed.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2328325006)
Rebased after #26619.

💬 remyers commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328327755)
Fwiw, I also ran across this problem (Ubuntu 20.04) while trying to run `clang-tidy-diff` on my code. A workaround is to just install and use clang-17.
```
cmake -B build -DCMAKE_C_COMPILER=clang-17 -DCMAKE_CXX_COMPILER=clang++-17 -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
cmake --build build -j $(nproc)
git diff HEAD~2 | ( cd ./src/ && clang-tidy-diff -p2 -path ../build -j $(nproc) )
```

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2328330492)
Rebased after trivial conflict with #29553.

👍 fanquake approved a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802#pullrequestreview-2279470892)
ACK fa78ed83be1f6831416a6f6632e2161f12d359e4

🚀 fanquake merged a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802)

📝 maflcko opened a pull request: "lint: Check for release note snippets in the wrong folder"
(https://github.com/bitcoin/bitcoin/pull/30812)
It is a common mistake to place the snippets in the wrong folder, where they could be missed. For example https://github.com/bitcoin/bitcoin/pull/30719#pullrequestreview-2262535007 or commit 84900ac34f6888b7a851d0a6a5885192155f865c.


Fix all issues by adding a simple lint check.

Can be tested by reverting a prior commit that violated the rule and then running the new check:

```
git revert 35ef34eab7b36e3c53ed438d74a9b783cbcaec27
( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo
...

💬 maflcko commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328378759)
> Fwiw, I also ran across this problem (Ubuntu 20.04)

I guess you also installed a different boost version, because the 1.71 boost version in Ubuntu Focal 20.04 is not supported starting with https://github.com/bitcoin/bitcoin/pull/29066 and shouldn't even pass the `cmake -B` command.

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743476276)
For reference, the len reporting (and error handling) was added after https://github.com/bitcoin/bitcoin/issues/9040#issuecomment-257161620.

Also, you added len reporting in a similar place recently in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499, so I fail to see why there it is "helpful" while here it would be not.

I am happy to review and ACK either version, but I think the length can be useful and may be better to not remove from the error string.