💬 waketraindev commented on pull request "doc: add cmake help option in Windows build docs":
(https://github.com/bitcoin/bitcoin/pull/33789#issuecomment-3491311987)
ACK 9577daa

💬 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-3491314532)
review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687 🐮

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a8a33bc0c0a1
...

💬 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-3491343671)
I tried to reproduce what the CI is doing locally. Having to run the full CI either on push or locally just to fix the includes is not ideal. The following command seems to produce different results:

```
/home/user/Downloads/include-what-you-use/iwyu_tool.py -p /home/user/bitcoin/build_dev_mode_clang/compile_commands.json src/kernel/chainparams.cpp -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=/home/user/bitcoin/contrib/devtools/iwyu/bitcoin.core.imp -Xiwyu --max_line_length=160

(/home/user
...

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3491348202)
Thanks for the suggestions and reviews! Implemented some test cleanup below.

I do want to wait for feedback from @fjahr before going ahead with this PR since he expressed a preference for keeping behavior unchanged until #28792 is merged ("I am again leaning towards keeping the current behavior for the intermediary step" from https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3475259644) and I think would that would be reasonable. But I do think this PR is an improvement and resolutio
...

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2494531445)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2490759547

> [cfeb160](https://github.com/bitcoin/bitcoin/commit/cfeb160baec1369452c42d05c51f2a6af76ed077): nit: Perhaps we could remove this `(default: none)` to maintain consistency with other `=<file>` options?

Thanks for the suggestion. I looked into it and initially it seemed like a good simplification, but then I noticed that this is not the only `(default: none)` mention in help since there is also `-i2psam`. Also since P
...

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-3491367389)
Rebased 6137cecfd3a448c2befe043849fce0b4ea2cd1fe -> 53abfae26ffbdd09daff66fd253171bbab1b550e ([chainman_flush_destructor_7](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_7) -> [chainman_flush_destructor_8](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_7..chainman_flush_destructor_8))

* Fixed conflict with #30595 - the PR now simplifies the bindings glue cod
...

🤔 rkrux reviewed a pull request: "Wallet: "listreceivedby*" fix"
(https://github.com/bitcoin/bitcoin/pull/30972#pullrequestreview-3422179813)
Concept ACK 486cd4779909c2403a308b05f537db276e780f2a

IMO it'd be a good idea to rebase this branch over master so that the changes of #32523 PR are incorporated that removes the outdated `ismine` enum which is seen quite a lot in this branch currently while reviewing.

💬 rkrux commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2494592664)
In a1d90fb3b1baefeffb59cf00bad4be3268040ce5 "wallet: 'Filter' out 'send' addresses from 'listreceivedby*'"

Instead of trying to lock again, assert that the function is already holding the lock? I checked that both the callers of the `ListReceived` function are acquiring the required lock.

```diff
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -73,6 +73,8 @@ struct tallyitem

static UniValue ListReceived(const CWallet& wallet, const UniValue& params, c
...

💬 rkrux commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2494608807)
In https://github.com/bitcoin/bitcoin/commit/a1d90fb3b1baefeffb59cf00bad4be3268040ce5 "wallet: 'Filter' out 'send' addresses from 'listreceivedby*'"

For making it concise.

```diff
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -73,6 +73,8 @@ struct tallyitem
auto it = mapTally.find(address);
- if (it == mapTally.end() && !fIncludeEmpty) {
- return;
- } else if (it == mapTally.end()) {
- LOCK(wallet.cs_wall
...

💬 rkrux commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2494657150)
In https://github.com/bitcoin/bitcoin/commit/486cd4779909c2403a308b05f537db276e780f2a "test: 'listreceivedby*' does not return send address"

```diff
- self.nodes[1].setlabel(address, label) # address has now assigned a "send" purpose on node1
+ self.nodes[1].setlabel(address, label) # address has now been assigned a "send" purpose on node1's wallet
```

💬 rkrux commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2494649185)
In 486cd4779909c2403a308b05f537db276e780f2a "test: 'listreceivedby*' does not return send address"

Nit (feel free to ignore): For expressiveness.

```diff
--- a/test/functional/wallet_listreceivedby.py
+++ b/test/functional/wallet_listreceivedby.py
@@ -119,12 +119,12 @@ class ReceivedByTest(BitcoinTestFramework):

expected = {"address": address, "label": label, "amount": Decimal("0.1"), "confirmations": 0, "txids": [txid, ]}
assert_array_result(self.nodes[0].listrec
...

💬 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-3491410162)
> different results:

I wonder if it comes from different libstdc++ header versions used. Generally, I could imagine it being difficult to achieve fully reproducible and consistent results across all build configurations. If relying on the CI (locally or remote) is too tedious, then it probably doesn't make sense to treat iwuy suggestions as errors.

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors? cc @willcl-ark @purpleKarrot
...

💬 maflcko commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2494675000)

“changes addresses” -> “change addresses” [“change addresses” is the correct term for output addresses, “changes” is a verb here and incorrect]

💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3491421525)
Rebased 00c5a8736532a0ca3c483d300e1d09d87be948f1 -> a5a8bff198f9f641c10e159fa3cc51757d8f69f6 ([blocktreestore_6](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_6) -> [blocktreestore_7](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_6..blocktreestore_7))

* Fixed conflict with #30595

💬 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.