💬 pinheadmz commented on issue "importdescriptors: check for errors before rescanning":
(https://github.com/bitcoin/bitcoin/issues/33655#issuecomment-3491424830)
@naiyoma thanks for taking an interest in this and digging in! I think a cleaner approach would be to separate the validation into a separate function entirely, so we validate in a loop first then process in a second loop. It smells better than adding an extra argument and feels more bitcoin-core-y. If you open a PR like that I'll help you get it merged ;-)

💬 willcl-ark commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491457664)
> Having to run the full CI either on push or locally just to fix the includes is not ideal.

Something I was thinking about while review the first part of this PR was whether using CMake's `CXX_INCLUDE_WHAT_YOU_USE ${iwyu_path}` to run this as another configuration "natively" with cmake. The issue here is it's project-wide, and you have to set [`SKIP_LINTING`](https://cmake.org/cmake/help/latest/prop_sf/SKIP_LINTING.html#prop_sf:SKIP_LINTING) on those files you don't want it run on. Then we c
...

📝 TheCharlatan opened a pull request: "kernel: Use enumeration type for flags argument"
(https://github.com/bitcoin/bitcoin/pull/33791)
Just a small followup from https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3485634089.

💬 maflcko commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491493431)
> > Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors
>
> Would be possible relatively easily, I think, if there's interest.

Nice. I was thinking that maybe the CI in this repo could have an auth token to push the iwyu for all builds to the dashboard. This way, it could be easier to check the warnings/error for each pull request, without it being a merge blocker. Also, there could be regular "fix iwyu" pull requests to bring th
...

💬 fanquake commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491494704)
> Would be possible relatively easily, I think, if there's interest.

I'm wondering what problem we are trying to solve. Externallising this would just lead to random PRs to this repo, to "fix" includes, which can't be verified here? If developers aren't actively engaged in doing that, and no CI will ever fail, that sounds like endless churn.

💬 TheCharlatan commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263)
Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3491554129)
Guix Build (x86_64):
```bash
6c31a0f19daec94c3d629e50d6bc2461ceb6e0fbb0823cb10a40bc735e37f9b5 guix-build-25ede968bada/output/aarch64-linux-gnu/SHA256SUMS.part
f3ba2f0c5f1d99d377ea11ea2b9983187192d733c7db444060d112d1be48ecd2 guix-build-25ede968bada/output/aarch64-linux-gnu/bitcoin-25ede968bada-aarch64-linux-gnu-debug.tar.gz
321db43f3a4327287813274b7f9874fb76799643fdc9cbbc9ee13bce10b6e004 guix-build-25ede968bada/output/aarch64-linux-gnu/bitcoin-25ede968bada-aarch64-linux-gnu.tar.gz
58f4490
...

💬 willcl-ark commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3491572687)
```
src/core/bitcoin on  pr-33775 [$?] via △ v3.31.6 via 🐍 v3.13.5 via ❄️ impure (nix-shell-env)
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
6c31a0f19daec94c3d629e50d6bc2461ceb6e0fbb0823cb10a40bc735e37f9b5 guix-build-25ede968bada/output/aarch64-linux-gnu/SHA256SUMS.part
f3ba2f0c5f1d99d377ea11ea2b9983187192d733c7db444060d112d1be48ecd2 guix-build-25ede968bada/output/aarch64-linux-gnu/bitcoin-25ed
...

💬 stickies-v commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2494809479)
It's a nit, so not a blocker either way. The debug value would come from knowing which block was rolled back/forward before some other event happened. Sure it's noisy when the roll{back/forward} happens but those events are rare so shouldn't be an issue in general, and when debug is enabled during these events, I think it's quite likely they're relevant information?

If not adopted here, I'm not sure it's worth opening a follow-up until people actually want the behaviour changed. Just seems li
...

💬 maflcko commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491592008)
> I'm wondering what problem we are trying to solve.

I think the problem is that it is inherently impossible to reproduce an iwyu run locally without the exact CI config. Maybe this is fine, and nothing needs to be fixed. Though, if devs think that relying exactly on the CI is too tedious, then a dashboard could seem like a nice solution.

> endless churn

I think iwyu will mean churn, regardless of how it is enforced, if it is run on more actively changed modules of the codebase.

I gu
...

👍 rkrux approved a pull request: "kernel: Use enumeration type for flags argument"
(https://github.com/bitcoin/bitcoin/pull/33791#pullrequestreview-3422514325)
lgtm ACK ed5720509f03ddd7f9158b9adf0d8fd7f56c8578 as per the mentioned review comment of the previous PR.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2494844175)
> Can we assert the behavior of the main cache as well so that the previous version doesn't pass?

Neither the temp cache or the main cache touch their backing cache in the input fetcher. So, we can't assert a failure if the backing cache is something else. We can assert that the backing view is not touched during `FetchInputs`, which is done in the fuzz harness.

👍 brunoerg approved a pull request: "test: remove obsolete `get_{key,multisig}` helpers from wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/33782#pullrequestreview-3422561866)
code review ACK ec8516ceb7568d7b09836b830023978bd37f8462

💬 alexanderwiederin commented on pull request "kernel: Use enumeration type for flags argument":
(https://github.com/bitcoin/bitcoin/pull/33791#issuecomment-3491703847)
ACK https://github.com/bitcoin/bitcoin/pull/33791/commits/ed5720509f03ddd7f9158b9adf0d8fd7f56c8578

👍 stickies-v approved a pull request: "kernel: Use enumeration type for flags argument"
(https://github.com/bitcoin/bitcoin/pull/33791#pullrequestreview-3422655653)
ACK ed5720509f03ddd7f9158b9adf0d8fd7f56c8578

🤔 pablomartin4btc reviewed a pull request: "doc: add cmake help option in Windows build docs"
(https://github.com/bitcoin/bitcoin/pull/33789#pullrequestreview-3422616007)
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29

_(please try to avoid mentions/ @-prefixed github names in PR descriptions as it can cause potential issues, primarily related to notification spam)_

💬 pablomartin4btc commented on pull request "doc: add cmake help option in Windows build docs":
(https://github.com/bitcoin/bitcoin/pull/33789#discussion_r2494905335)
nit:
```suggestion
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
(run `cmake -B build -LH` to see the full list of available options)

```

📝 vasild opened a pull request: "net_processing: reorder the code that handles the VERSION message"
(https://github.com/bitcoin/bitcoin/pull/33792)
Change the order in which code snippets are executed as a result of receiving the `VERSION` message. Move the snippets that do `MakeAndPushMessage()` near the end. This makes it easier to interrupt the execution when no messages should be sent as a response to the `VERSION` messages, in private broadcast connections.

This is a non-functional change.

---

This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull
...

💬 vasild commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3491723472)
This is just moving code around. Easier reviewed with `~/.gitconfig` like:

```gitconfig
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```

🚀 fanquake merged a pull request: "kernel: Use enumeration type for flags argument"
(https://github.com/bitcoin/bitcoin/pull/33791)