💬 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
...
(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
...
(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
...
(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
...
(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.
(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
...
(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
...
(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
```
(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
...
(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
...
(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]
(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
(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 ;-)
(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
...
(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.
(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
...
(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.
(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.
(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
...
(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
...
(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
...