💬 maflcko commented on pull request "doc: remove Eclipser fuzzing documentation":
(https://github.com/bitcoin/bitcoin/pull/30908#issuecomment-2356545824)
review ACK 735436df8cebf28d4bcf0ad7e8f1fd7f19191001

👍 brunoerg approved a pull request: "doc: remove Eclipser fuzzing documentation"
(https://github.com/bitcoin/bitcoin/pull/30908#pullrequestreview-2310438395)
ACK 735436df8cebf28d4bcf0ad7e8f1fd7f19191001

💬 marcofleon commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1763652769)
Why is it that we don't initialize these with the`Txid` and `Wtxid` types? Have been thinking about making `RelayTransaction` type safe, so just wondering.

👍 theuni approved a pull request: "ci: Use `ninja` to build in macOS native CI job"
(https://github.com/bitcoin/bitcoin/pull/30915#pullrequestreview-2310455658)
ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f.

I was going to suggest this as well :)

💬 jonatack commented on pull request "ci: Use `ninja` to build in macOS native CI job":
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2356604457)
Concept ACK in using https://ninja-build.org / https://github.com/ninja-build/ninja for a CI task as a point of comparison.

🤔 jonatack reviewed a pull request: "ci: Use `ninja` to build in macOS native CI job"
(https://github.com/bitcoin/bitcoin/pull/30915#pullrequestreview-2310534703)
LGTM but am new to CMake builds.

ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f

💬 davidgumberg commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763724027)
Could be scripted diff:

```bash
sed -i -E 's/CheckWriteCoins\(([^,]+), ([^,]+), ([^,]+), ([^,]+), ([^,]+), ([^,]+)\);/CheckWriteCoins({\1, \4}, {\2, \5}, {\3, \6});/' src/test/coins_tests.cpp
```

👍 hebasto approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2310572569)
re-ACK 807a429a33cd9b1986f1ebb8fd761887f556afd8, rebased only since my recent [review](https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2300357639).

My Guix build:
```
aarch64
f6a91156bc9ac2ec9367d75a433d903db4fe5c4ff0c5680289882f1ca58b9759 guix-build-807a429a33cd/output/aarch64-linux-gnu/SHA256SUMS.part
2704a2fbc76888fb9073631513ea1fc2203edb344814dbe5db02fd51c6d9949d guix-build-807a429a33cd/output/aarch64-linux-gnu/bitcoin-807a429a33cd-aarch64-linux-gnu-debug.tar.gz
a5c
...

💬 davidgumberg commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763730500)
Although I guess this section would be ugly to do as a scripted diff

💬 hebasto commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#discussion_r1763730646)
@theuni

You might be interesting in reviewing this PR, as this comment was added at your request while working on the CMake staging branch, if I recall correctly :)

💬 davidgumberg commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763731654)
Nit: Could be made into a loop as below

🤔 instagibbs reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2310464388)
it's likely the wrong place for the coverage, but I added some coverage for iterative additional and removal of transactions. Take what you think is useful if any:

```
diff --git a/src/test/fuzz/cluster_linearize.cpp b/src/test/fuzz/cluster_linearize.cpp
index be06898a6e..62b5ec2343 100644
--- a/src/test/fuzz/cluster_linearize.cpp
+++ b/src/test/fuzz/cluster_linearize.cpp
@@ -1,19 +1,20 @@
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see
...

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1763665495)
Was reconstructing graphs incorrectly leading to inconsistent sets. Better to fail here early.

```Suggestion
{
// We can't add relationships to holes
Assume(parents.IsSubsetOf(m_used));
```
a check like this for `AddDependency` would also be good:

`Assume(SetType::Singleton(parent).IsSubsetOf(m_used));`

💬 davidgumberg commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2356666728)
Concept ACK on dropping the bitfield interface for `CCoinsCacheEntry` flags

🤔 theuni reviewed a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2310630029)
Concept ACK.

Though, `obj/` was acting as a bit of a namespace here (IIRC, that was another reason for it to be in a subdir). Especially since this is included with `<>` as opposed to `""`, while we're at it, I think we should rename this file. `build.h` seems generic enough to collide with something else. `bitcoin-build.h` maybe?

💬 pablomartin4btc commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2356746568)
Updates:
- Removed validation commit which I'll added in a separate PR as [suggested](https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354091658) by @jonatack.
- Updated description and title accordingly with above.
- Added test checking -rpcwallet=<filename> is not doable as [suggested](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762109275) by @jonatack.
- Replaced `-rpcwallet=<filename>` which is incorrect with `-rpcwallet=<walletname>` as suggested by @mzumsande
...

💬 maflcko commented on pull request "ci: Use clang-19 from apt.llvm.org":
(https://github.com/bitcoin/bitcoin/pull/30634#issuecomment-2356758336)
testing tsan on aarch gives me:

<details><summary>tsan error</summary>

```
src/wallet/test/wallet_tests.cpp(75): Leaving test case "scan_for_wallet_transactions"; testing time: 2316025us
src/wallet/test/wallet_tests.cpp(921): Entering test case "CreateWalletWithoutChain"
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=62275)
Cycle in lock order graph: M0 (0xffff74604608) => M1 (0xffff7460cb58) => M2 (0xffff7460cd08) => M0

Mutex M1 acq
...

💬 theuni commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356804690)
Likewise for `config/bitcoin-config.h` btw. IIRC `config/` part is an old artifact of the switch _to_ autotools. I don't see any reason why bitcoin-config.h shouldn't just live in `build/src/` now.

Err.. I guess that would mean touching a _lot_ of files though :)

💬 maflcko commented on pull request "ci: Use clang-19 from apt.llvm.org":
(https://github.com/bitcoin/bitcoin/pull/30634#issuecomment-2356842989)
Testing on aarch64, Asan and Fuzz pass. The Tsan issue on aarch64 is pre-existing and unrelated to this pull request, so can be ignored.

💬 hodlinator commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2356850948)
Nice work @rkrux!

Counter intuitive behavior for the TRUC novice towards the end of [V3 Transactions TRUC](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Candidate-Testing-Guide#2-v3-transactions-truc):

* `testmempoolaccept` shows `siblingTx2` as being in `TRUC-violation` and not allowed, but its child `txWithUncle` has no issues listed.
* `submitpackage` then accepts `siblingTx2` but rejects `txWithUncle`. (Wouldn't it be nice if `submitpackage` was atomic, all or noth
...