💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298311703)
In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
Nit: would be nice to tie this `9` to the `10` above in the `keypoolrefill` RPC - `KEYPOOL_SIZE - 1`
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298311703)
In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
Nit: would be nice to tie this `9` to the `10` above in the `keypoolrefill` RPC - `KEYPOOL_SIZE - 1`
💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298304988)
In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
In this subtest, can also assert for few `getwalletinfo` RPC response values being equal as well such as `txcount, keypoolsize, keypoolsize_hd_internal, birthtime, lastprocessedblock`. For the `flags` RPC response key, it can be asserted that for the watch-only one, the value is the union of the original flags and `disable_private_keys`.
Might as well put all these a
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298304988)
In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
In this subtest, can also assert for few `getwalletinfo` RPC response values being equal as well such as `txcount, keypoolsize, keypoolsize_hd_internal, birthtime, lastprocessedblock`. For the `flags` RPC response key, it can be asserted that for the watch-only one, the value is the union of the original flags and `disable_private_keys`.
Might as well put all these a
...
💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298327208)
In cdaed2bab95870f77efb3cbca1dc98e5d4f5695c "wallet, rpc: Add exportwatchonlywallet RPC"
Based on some threshold value of remaining unused keys available, should this RPC also pre-generate keys before exporting the watch-only wallet so that the user doesn't end up with not being able to generate new addresses quickly after exporting and has to export again?
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298327208)
In cdaed2bab95870f77efb3cbca1dc98e5d4f5695c "wallet, rpc: Add exportwatchonlywallet RPC"
Based on some threshold value of remaining unused keys available, should this RPC also pre-generate keys before exporting the watch-only wallet so that the user doesn't end up with not being able to generate new addresses quickly after exporting and has to export again?
💬 instagibbs commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3220639788)
Seems like a03aef9cec35b0d03aa63d7e8093f0420cd4b40b is the problematic commit for my machine? Doubles the runtime.
Both reverted:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,955,234.00 | 511.45 | 2.1% | 0.02 | `BlockEncoding`
No reversions:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--
...
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3220639788)
Seems like a03aef9cec35b0d03aa63d7e8093f0420cd4b40b is the problematic commit for my machine? Doubles the runtime.
Both reverted:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,955,234.00 | 511.45 | 2.1% | 0.02 | `BlockEncoding`
No reversions:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--
...
🤔 jonatack reviewed a pull request: "Revert compact block cache inefficiencies"
(https://github.com/bitcoin/bitcoin/pull/33253#pullrequestreview-3152074460)
ARM64 M1 Max (2022), macOS 15.6.1
benchmark only
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,643,000.00 | 378.36 | 1.5% | 0.03 | `BlockEncoding`
```
benchmark with reversions
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:--------
...
(https://github.com/bitcoin/bitcoin/pull/33253#pullrequestreview-3152074460)
ARM64 M1 Max (2022), macOS 15.6.1
benchmark only
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,643,000.00 | 378.36 | 1.5% | 0.03 | `BlockEncoding`
```
benchmark with reversions
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:--------
...
💬 cedwies commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298491922)
This test also passes on master (MacOS environment with ARG_MAX = 1048576). It does not fail with the test_framework changes reverted.
However, the following test only passes on this PR, not on my master:
```python
self.log.info("test many small args (sum-size trigger)")
tiny = "a" * 120000
args = [tiny] * 10
assert_equal(args, node.echo(*args))
```
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298491922)
This test also passes on master (MacOS environment with ARG_MAX = 1048576). It does not fail with the test_framework changes reverted.
However, the following test only passes on this PR, not on my master:
```python
self.log.info("test many small args (sum-size trigger)")
tiny = "a" * 120000
args = [tiny] * 10
assert_equal(args, node.echo(*args))
```
💬 cedwies commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3220904717)
ACK fabe214
This PR fixes how the functional test framework estimates command-line argument size. Previously it checked only the longest argument, but the OS limit is on the total argv/env size, so it now sums all args. It also adds a functional test using very large echo args, which exercises the `-stdin` fallback. The test passes for me.
I appreciate lowering `CLI_MAX_ARG_SIZE` to `1024`. It’s a very safe, conservative threshold that avoids platform-dependent `ARG_MAX` issues, and since
...
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3220904717)
ACK fabe214
This PR fixes how the functional test framework estimates command-line argument size. Previously it checked only the longest argument, but the OS limit is on the total argv/env size, so it now sums all args. It also adds a functional test using very large echo args, which exercises the `-stdin` fallback. The test passes for me.
I appreciate lowering `CLI_MAX_ARG_SIZE` to `1024`. It’s a very safe, conservative threshold that avoids platform-dependent `ARG_MAX` issues, and since
...
🤔 enirox001 reviewed a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3152161767)
Concept ACK fa18c82 — The fix is a welcome improvement: summing arg sizes better reflects kernel ARG_MAX behavior. Good use of >= to match inclusive boundary semantics. Left a few non-blocking clarifier nits inline.
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3152161767)
Concept ACK fa18c82 — The fix is a welcome improvement: summing arg sizes better reflects kernel ARG_MAX behavior. Good use of >= to match inclusive boundary semantics. Left a few non-blocking clarifier nits inline.
💬 enirox001 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298536682)
```
2025-08-25T15:54:00.131311Z TestFramework.bitcoincli (DEBUG): Cli: Command size 131140 too large, using stdin
2025-08-25T15:54:00.148742Z TestFramework.bitcoincli (DEBUG): Cli: Command size 262212 too large, using stdin
2025-08-25T15:54:00.179312Z TestFramework.bitcoincli (DEBUG): Cli: Command size 4194372 too large, using stdin
2025-08-25T15:54:00.418554Z TestFramework.bitcoincli (DEBUG): Cli: Command size 8388676 too large, using stdin
```
fa18c82 — The debug logs show byte-based s
...
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298536682)
```
2025-08-25T15:54:00.131311Z TestFramework.bitcoincli (DEBUG): Cli: Command size 131140 too large, using stdin
2025-08-25T15:54:00.148742Z TestFramework.bitcoincli (DEBUG): Cli: Command size 262212 too large, using stdin
2025-08-25T15:54:00.179312Z TestFramework.bitcoincli (DEBUG): Cli: Command size 4194372 too large, using stdin
2025-08-25T15:54:00.418554Z TestFramework.bitcoincli (DEBUG): Cli: Command size 8388676 too large, using stdin
```
fa18c82 — The debug logs show byte-based s
...
💬 enirox001 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298540591)
fa18c82 — Does this test use the system `ARG_MAX` at runtime or intentionally use a conservative value? Could we add a short comment that `1024` is a test-only, conservative value to ensure the stdin fallback is exercised, and that production behavior still relies on the system `ARG_MAX`
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298540591)
fa18c82 — Does this test use the system `ARG_MAX` at runtime or intentionally use a conservative value? Could we add a short comment that `1024` is a test-only, conservative value to ensure the stdin fallback is exercised, and that production behavior still relies on the system `ARG_MAX`
💬 enirox001 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298546674)
fa18c82 — Perhaps add a short note that these sizes target historical/modern `ARG_MAX` boundaries (64KiB, ~128KiB, ~2MiB, ~4MiB) and that `-1` tests just-below the power-of-two to catch off-by-ones? Took me a while to understand the intent.
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2298546674)
fa18c82 — Perhaps add a short note that these sizes target historical/modern `ARG_MAX` boundaries (64KiB, ~128KiB, ~2MiB, ~4MiB) and that `-1` tests just-below the power-of-two to catch off-by-ones? Took me a while to understand the intent.
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2298621307)
These are extremely fast tests, keeping the usual test format has more advantages in my opinion.
If you have a string preference or if other reviewers prefer that, I don't mind changing, but I don't think the current one has measurable disadvantages compared to the suggestion - while being more in-line with how we usually test, making `GetDbBatchSize` easily debuggable (often helps with understanding if you can step through it).
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2298621307)
These are extremely fast tests, keeping the usual test format has more advantages in my opinion.
If you have a string preference or if other reviewers prefer that, I don't mind changing, but I don't think the current one has measurable disadvantages compared to the suggestion - while being more in-line with how we usually test, making `GetDbBatchSize` easily debuggable (often helps with understanding if you can step through it).
💬 Galoretka commented on pull request "doc: add Linux GUI runtime instructions to doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/33197#issuecomment-3221188022)
> The dependency on `libxcb-xinerama0` has been removed in #33217 (which will also be backported to the `29.x` branch) so I think pretty much all of the text being added here, is no-longer needed?
so i should close pr?
(https://github.com/bitcoin/bitcoin/pull/33197#issuecomment-3221188022)
> The dependency on `libxcb-xinerama0` has been removed in #33217 (which will also be backported to the `29.x` branch) so I think pretty much all of the text being added here, is no-longer needed?
so i should close pr?
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3221463982)
Ping :)
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3221463982)
Ping :)
💬 achow101 commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3221526194)
> _aj_> instagibbs: re: 33253, the benchmark doesn't exercise [a8203e9](https://github.com/bitcoin/bitcoin/commit/a8203e94123b6ea6e4f4a6320e3ad20457f44a28) -- it just passes an empty vector of extra transactions
Can we bench that?
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3221526194)
> _aj_> instagibbs: re: 33253, the benchmark doesn't exercise [a8203e9](https://github.com/bitcoin/bitcoin/commit/a8203e94123b6ea6e4f4a6320e3ad20457f44a28) -- it just passes an empty vector of extra transactions
Can we bench that?
💬 martinatime commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3221628972)
As an update, my bitcoin instance has finally finished the initial verification. I started it on June 1st and it has finished between last night (August 24) and today.
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3221628972)
As an update, my bitcoin instance has finally finished the initial verification. I started it on June 1st and it has finished between last night (August 24) and today.
📝 151henry151 opened a pull request: "wallet: Replace fee magic numbers with named constants"
(https://github.com/bitcoin/bitcoin/pull/33254)
## Summary
This PR addresses a TODO item in the wallet fee handling code by replacing magic numbers with descriptive named constants. It also adds `constexpr` constructors to `CFeeRate` to support compile-time constants.
## Changes
- **Add `constexpr` CFeeRate constructors** to support compile-time constants
- **Replace magic `CFeeRate(0)` values** with descriptive named constants:
- `DEFAULT_FEERATE` for default/unset fee rates
- `NO_FEE_DATA` for fee estimation failures
- `FEERATE_D
...
(https://github.com/bitcoin/bitcoin/pull/33254)
## Summary
This PR addresses a TODO item in the wallet fee handling code by replacing magic numbers with descriptive named constants. It also adds `constexpr` constructors to `CFeeRate` to support compile-time constants.
## Changes
- **Add `constexpr` CFeeRate constructors** to support compile-time constants
- **Replace magic `CFeeRate(0)` values** with descriptive named constants:
- `DEFAULT_FEERATE` for default/unset fee rates
- `NO_FEE_DATA` for fee estimation failures
- `FEERATE_D
...
💬 achow101 commented on pull request "Update libmultiprocess subtree to fix build issues":
(https://github.com/bitcoin/bitcoin/pull/33241#issuecomment-3221668831)
ACK 323b3fd27283282f2f8eb1096f56f23d8230f2d6
Verified the subtree update and that it fixes the Ubuntu issue (after removing the 0.8.0 restriction).
(https://github.com/bitcoin/bitcoin/pull/33241#issuecomment-3221668831)
ACK 323b3fd27283282f2f8eb1096f56f23d8230f2d6
Verified the subtree update and that it fixes the Ubuntu issue (after removing the 0.8.0 restriction).
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299137075)
I don't think any of those are actually useful things to assert. The point of the offline wallet is that it doesn't have access to the blockchain, so actually `txcount`, and `lastprocessedblock` should be different. The keypool sizes may be different depending on the node startup options, and birthtime may be different if something was used before being imported to the offline wallet.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299137075)
I don't think any of those are actually useful things to assert. The point of the offline wallet is that it doesn't have access to the blockchain, so actually `txcount`, and `lastprocessedblock` should be different. The keypool sizes may be different depending on the node startup options, and birthtime may be different if something was used before being imported to the offline wallet.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299144047)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299144047)
Done