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