💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2649356408)
> I wonder if it wouldn't be easier to just go with clang/llvm

I've tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack

💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2649374995)
> Could the `CAPNP_EXECUTABLE`, `CAPNPC_CXX_EXECUTABLE` and all `CapnProto_*` CMake variables be marked as advanced to avoid cluttering the CMake cache view when inspected by the user?

Agreed on the rest, but `CAPNP_EXECUTABLE` and `CAPNPC_CXX_EXECUTABLE` are relevant to cross-compilers and shouldn't be hidden. In fact, to be thorough, `multiprocess.md` should mention them as well.

🤔 ismaelsadeeq reviewed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803#pullrequestreview-2607326125)
Concept ACK

💬 ismaelsadeeq commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1949991208)
I don't think this conditional check is necessary. It will be simpler and more straightforward to just set the minimum rather than creating a new variable and then checking the conditional statement.

In the worst case, we are creating two variables here, whereas in my suggestion, we are just updating the initial variable.

```suggestion
miner_options.block_reserved_weight = MINIMUM_BLOCK_RESERVED_WEIGHT;
```

💬 ismaelsadeeq commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1949993496)
Nice moving this here, prevents creating `block_adjusted_max_weight` variable in each loop iteration.

💬 hodlinator commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949981506)
Nice touch on adding a check for non-existence of abc!
https://github.com/bitcoin/bitcoin/blob/bb8309723cdd3ce0378fbe9708bd600dfaa157aa/test/functional/feature_logging.py#L113

💬 hodlinator commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949995455)
Seems like you forgot to remove this? (`last_negated` should theoretically be able to be marked `const`, but makes line go over 80 chars).
```suggestion
```

💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2649431826)
Updated this to work even after the legacy wallet is deleted.

💬 hodlinator commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2649437799)
> > Attempted to run Guix build [e717fb5](https://github.com/bitcoin/bitcoin/commit/e717fb5a429d9d00e008fa1eb2b85179050be8fe) cross-built for Windows, but it fails already on test "0" / line 133:
>
> Is this failure introduced by this PR, or does it occur on the master branch as well?

I think the *way* cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.

With the same executable (e717fb5a429d = this PR which only includes Python changes) on master
...

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2649486439)
Changed the `test each commit` CI job to use `-DWARN_INCOMPATIBLE_BDB=OFF`, otherwise it results in a fatal error if `-DWCONFIGURE_ERROR=ON`.

💬 achow101 commented on pull request "test: expect that files may disappear from /proc/PID/fd/":
(https://github.com/bitcoin/bitcoin/pull/31614#issuecomment-2649491412)
ACK b2e9fdc00f5c40c241a37739f7b73b74c2181e39

💬 achow101 commented on pull request "test: Fuzz the human-readable part of bech32 as well":
(https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2649499608)
ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854

🚀 achow101 merged a pull request: "test: expect that files may disappear from /proc/PID/fd/"
(https://github.com/bitcoin/bitcoin/pull/31614)

🚀 achow101 merged a pull request: "test: Fuzz the human-readable part of bech32 as well"
(https://github.com/bitcoin/bitcoin/pull/30623)

📝 crStiv opened a pull request: "streams: Add stream position validation in BufferedFile::AdvanceStream"
(https://github.com/bitcoin/bitcoin/pull/31839)
Description:
Add safety check in BufferedFile::AdvanceStream to prevent potential integer overflow when calculating bytes_until_source_pos. This ensures that m_read_pos never exceeds nSrcPos, which could lead to undefined behavior.

The change:
- Adds validation before calculating (nSrcPos - m_read_pos)
- Throws std::ios_base::failure if invalid state is detected

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649739813)
Rebased 64b833854a34d87cde4e0ca4173d75012c401a7a -> 4fa7f72cb9d8f0290f56293989ec6ea950162c5b ([`pr/ipc-chain.12`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.12) -> [`pr/ipc-chain.13`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.13), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-chain.12-rebase..pr/ipc-chain.13)) due to various conflicts.

@pseudoramdom, with this update, errors you posted should be resolved

💬 rhysbeynon commented on pull request "Updated MacOS icon to more closely fit Apple's design standards":
(https://github.com/bitcoin-core/gui/pull/852#issuecomment-2649860867)
> > The .icns file was compiled using Apple's own "iconutil" cli program.
>
> What are steps to reproduce the new `src/qt/res/icons/bitcoin.icns` file?

I cloned bitcoin.icns, used the command `iconutil -c iconset bitcoin.icns` to convert the assembled icon file into a png IconSet. I then used Affinity Photo to make programmable size changes to each png identically. With the changed icon set, I then used `iconutil -c icns bitcoin.iconset` to assemble the modified .icns file. This process is
...

✅ maflcko closed a pull request: "streams: Add stream position validation in BufferedFile::AdvanceStream"
(https://github.com/bitcoin/bitcoin/pull/31839)

💬 maflcko commented on pull request "streams: Add stream position validation in BufferedFile::AdvanceStream":
(https://github.com/bitcoin/bitcoin/pull/31839#issuecomment-2649979750)
The prior assert with the exact same check covers this. This is adding dead code.

Is this AI generated?

💬 maflcko commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2649985978)
I still think there are two errors in the pull description, see https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2610142158