🤔 BrandonOdiwuor reviewed a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2921936952)
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
<img width="759" alt="Screenshot 2025-06-12 at 19 31 15" src="https://github.com/user-attachments/assets/8a9dd961-ecb5-4ee6-b0a6-7b021083f00a" />
Master | c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
:-------------------------:|:-------------------
...
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2921936952)
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
<img width="759" alt="Screenshot 2025-06-12 at 19 31 15" src="https://github.com/user-attachments/assets/8a9dd961-ecb5-4ee6-b0a6-7b021083f00a" />
Master | c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
:-------------------------:|:-------------------
...
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2967550739)
Turns out I got a little carried away by #32421; blocks do still need `rehash()`.
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2967550739)
Turns out I got a little carried away by #32421; blocks do still need `rehash()`.
⚠️ achow101 pinned an issue: "build: RFC Coverage build type"
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.
It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.
There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.
It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.
There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...
⚠️ achow101 unpinned an issue: "build: RFC Coverage build type"
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.
It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.
There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.
It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.
There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...
📝 fanquake opened a pull request: "tsan: drop Qt wildcard suppressions"
(https://github.com/bitcoin/bitcoin/pull/32739)
Replaces the wildcards with non-wildcards. Enough for the CI to pass for me locally on x86_64 and aarch64. I would not be suprised if the CI here/someone else uncovers more. If this looks too unruly to maintain, maybe we should just drop the TODO? I haven't yet seen a `deadlock` issue.
(https://github.com/bitcoin/bitcoin/pull/32739)
Replaces the wildcards with non-wildcards. Enough for the CI to pass for me locally on x86_64 and aarch64. I would not be suprised if the CI here/someone else uncovers more. If this looks too unruly to maintain, maybe we should just drop the TODO? I haven't yet seen a `deadlock` issue.
💬 fanquake commented on pull request "depends: fix cmake compatibility error for freetype":
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2967591666)
Guix Build:
```bash
96d5182898d13bbe5ee972df791c7fba7a4c3927c6fd224eba20e8d99fbf5173 guix-build-d7c37906e7b1/output/aarch64-linux-gnu/SHA256SUMS.part
88d39050979a7441601f371a81e80fbe43035ff564d03dbca5de1fe61d9f6876 guix-build-d7c37906e7b1/output/aarch64-linux-gnu/bitcoin-d7c37906e7b1-aarch64-linux-gnu-debug.tar.gz
20f78def7e6b24c1901b67ec1d694d315da1c3db45ff616b3ffff36046f785e6 guix-build-d7c37906e7b1/output/aarch64-linux-gnu/bitcoin-d7c37906e7b1-aarch64-linux-gnu.tar.gz
1dafb25aa1ea1a89
...
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2967591666)
Guix Build:
```bash
96d5182898d13bbe5ee972df791c7fba7a4c3927c6fd224eba20e8d99fbf5173 guix-build-d7c37906e7b1/output/aarch64-linux-gnu/SHA256SUMS.part
88d39050979a7441601f371a81e80fbe43035ff564d03dbca5de1fe61d9f6876 guix-build-d7c37906e7b1/output/aarch64-linux-gnu/bitcoin-d7c37906e7b1-aarch64-linux-gnu-debug.tar.gz
20f78def7e6b24c1901b67ec1d694d315da1c3db45ff616b3ffff36046f785e6 guix-build-d7c37906e7b1/output/aarch64-linux-gnu/bitcoin-d7c37906e7b1-aarch64-linux-gnu.tar.gz
1dafb25aa1ea1a89
...
💬 PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2967598737)
@achow101 I modified the code as your review comments, and I hope you can recheck them. It seems i could still not run all ci tests without your approval, is it because of mechanism? or other reason which we did not realize...
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2967598737)
@achow101 I modified the code as your review comments, and I hope you can recheck them. It seems i could still not run all ci tests without your approval, is it because of mechanism? or other reason which we did not realize...
💬 hMsats commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2967625812)
@l0rinc I'm running `-reindex` and will come back with some results the beginning of next week.
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2967625812)
@l0rinc I'm running `-reindex` and will come back with some results the beginning of next week.
💬 PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#discussion_r2143258962)
`MockableBatch` actually has `m_read_only` because i have use `database.IsReadOnly` instead.
(https://github.com/bitcoin/bitcoin/pull/32685#discussion_r2143258962)
`MockableBatch` actually has `m_read_only` because i have use `database.IsReadOnly` instead.
💬 hebasto commented on pull request "doc: Update Qt 6 packages on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/32717#discussion_r2143266964)
Thanks! Taken.
(https://github.com/bitcoin/bitcoin/pull/32717#discussion_r2143266964)
Thanks! Taken.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2967662605)
Rebased on top of #30605.
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2967662605)
Rebased on top of #30605.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2143286465)
I am not sure how I feel about my approach here because the caller of `LogPrintStr_` now needs to be aware that `std::source_location&` can be moved-from?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2143286465)
I am not sure how I feel about my approach here because the caller of `LogPrintStr_` now needs to be aware that `std::source_location&` can be moved-from?
💬 furszy commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2143351000)
> I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param
Ok, I'm a ~0 on this. I think this field is special enough (reason below) to be signaled as a separate arg or it could be wrapped into a struct — similar to how it's done in the `interfaces::Chain::Notifications class`. Or could try the TxDownloadManager `enabled` flag approach too.
The only thing I'm somewhat convinced of is that, just lik
...
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2143351000)
> I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param
Ok, I'm a ~0 on this. I think this field is special enough (reason below) to be signaled as a separate arg or it could be wrapped into a struct — similar to how it's done in the `interfaces::Chain::Notifications class`. Or could try the TxDownloadManager `enabled` flag approach too.
The only thing I'm somewhat convinced of is that, just lik
...
🤔 PeterWrighten reviewed a pull request: "wallet: Allow read-only database access for info and dump commands"
(https://github.com/bitcoin/bitcoin/pull/32685#pullrequestreview-2922180881)
Run all tests.
(https://github.com/bitcoin/bitcoin/pull/32685#pullrequestreview-2922180881)
Run all tests.
💬 achow101 commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#discussion_r2143363060)
The exception should be thrown before `sqlite3_exec(m_db, "CREATE TABLE ...")` if we the database is readonly.
Something like
```
if (!table_exists) {
if (m_read_only) {
throw std::runtime_error(....);
} else {
ret = sqlite3_exec(...);
}
}
```
The latest changes are also fine as well.
(https://github.com/bitcoin/bitcoin/pull/32685#discussion_r2143363060)
The exception should be thrown before `sqlite3_exec(m_db, "CREATE TABLE ...")` if we the database is readonly.
Something like
```
if (!table_exists) {
if (m_read_only) {
throw std::runtime_error(....);
} else {
ret = sqlite3_exec(...);
}
}
```
The latest changes are also fine as well.
💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2143368568)
Sure, will do it if it needs a re-touch.
I don't see it as that important because if the flag wouldn't being set, the test would be failing due to the index ignoring the `BlockConnected` signals.
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2143368568)
Sure, will do it if it needs a re-touch.
I don't see it as that important because if the flag wouldn't being set, the test would be failing due to the index ignoring the `BlockConnected` signals.
🤔 marcofleon reviewed a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2921965126)
code review ACK 9877e275b95400e46f336bc34ce41ed68ce43683
Previously, the test helpers (`SimpleCandidateFinder` and `SimpleLinearize`) were being checked for correctness within the same fuzz targets that are testing the production cluster linearization. Separating out these checks into their own targets reduces redundancy and makes the hierarchy of all these tests easier to follow.
I ran `clusterlin_linearize`, `clusterlin_search_finder`, `clusterlin_simple_finder`, and `clusterlin_simple_
...
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2921965126)
code review ACK 9877e275b95400e46f336bc34ce41ed68ce43683
Previously, the test helpers (`SimpleCandidateFinder` and `SimpleLinearize`) were being checked for correctness within the same fuzz targets that are testing the production cluster linearization. Separating out these checks into their own targets reduces redundancy and makes the hierarchy of all these tests easier to follow.
I ran `clusterlin_linearize`, `clusterlin_search_finder`, `clusterlin_simple_finder`, and `clusterlin_simple_
...
💬 marcofleon commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2143221865)
nittiest of nits: Missing a space here. Could be fixed in #32545 if this is isn't touched anymore.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2143221865)
nittiest of nits: Missing a space here. Could be fixed in #32545 if this is isn't touched anymore.
💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#issuecomment-2967792157)
Will try to not touch the code while we are all happy with the current state. Moving #26966 and #24230 forward after ~3 years sounds good enough to me :)
(https://github.com/bitcoin/bitcoin/pull/32694#issuecomment-2967792157)
Will try to not touch the code while we are all happy with the current state. Moving #26966 and #24230 forward after ~3 years sounds good enough to me :)
💬 achow101 commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2967793006)
Please squash your commits; reviewer feedback does not need to be addressed in a separate commit.
***
When I said that all commits need to pass the tests, I did not mean that everything should be squashed into one commit. While that can be okay to do, it's better to have individual commits that introduce specific things, as long as those commits still pass the tests. My suggestion was more that the commits could be reordered, and some of them squashed, in order to allow all of the tests to
...
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2967793006)
Please squash your commits; reviewer feedback does not need to be addressed in a separate commit.
***
When I said that all commits need to pass the tests, I did not mean that everything should be squashed into one commit. While that can be okay to do, it's better to have individual commits that introduce specific things, as long as those commits still pass the tests. My suggestion was more that the commits could be reordered, and some of them squashed, in order to allow all of the tests to
...
📝 mzumsande converted_to_draft a pull request: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290)
Fixes #32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
-
...
(https://github.com/bitcoin/bitcoin/pull/32290)
Fixes #32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
-
...