💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#issuecomment-3220153666)
re-ACK [`37d2246`](https://github.com/bitcoin/bitcoin/pull/31725/commits/37d22461aa8f62d7fee9b65c8580b7f7a25f3382)
(https://github.com/bitcoin/bitcoin/pull/31725#issuecomment-3220153666)
re-ACK [`37d2246`](https://github.com/bitcoin/bitcoin/pull/31725/commits/37d22461aa8f62d7fee9b65c8580b7f7a25f3382)
💬 frankomosh commented on pull request "fuzz: add target for `DifferenceFormatter` and p2p invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3220234558)
dropped the serialization roundrip as @maflcko suggests, they can be done elsewhere (./src/test/fuzz/deserialize.cpp).
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3220234558)
dropped the serialization roundrip as @maflcko suggests, they can be done elsewhere (./src/test/fuzz/deserialize.cpp).
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3220359214)
test_framework.py:565: error: Need type annotation for "extra_init"
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3220359214)
test_framework.py:565: error: Need type annotation for "extra_init"
💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2298234266)
Looking at this more closely, the answer was clear and so I removed the additional lines of the TODO as well.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2298234266)
Looking at this more closely, the answer was clear and so I removed the additional lines of the TODO as well.
💬 instagibbs commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3220576303)
related comment: https://github.com/bitcoin/bitcoin/pull/29752#issuecomment-3216827263
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3220576303)
related comment: https://github.com/bitcoin/bitcoin/pull/29752#issuecomment-3216827263
👋 frankomosh's pull request is ready for review: "fuzz: add target for `DifferenceFormatter` and p2p invariant check"
(https://github.com/bitcoin/bitcoin/pull/33252)
(https://github.com/bitcoin/bitcoin/pull/33252)
💬 instagibbs commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3220633110)
For my machine on a first pass, it seems a03aef9cec35b0d03aa63d7e8093f0420cd4b40b is the only problematic commit (2x slowdown though)?
No reversions:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 3,945,310.00 | 253.47 | 3.0% | 0.05 | `BlockEncoding`
Both reverted:
| ns/op | op/s | err% | total | benchmark
|----
...
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3220633110)
For my machine on a first pass, it seems a03aef9cec35b0d03aa63d7e8093f0420cd4b40b is the only problematic commit (2x slowdown though)?
No reversions:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 3,945,310.00 | 253.47 | 3.0% | 0.05 | `BlockEncoding`
Both reverted:
| ns/op | op/s | err% | total | benchmark
|----
...
🤔 rkrux reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3151519710)
Started reviewing.
Code review cf588607fa36964597a6d6acc853eeb26e51b0ea
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3151519710)
Started reviewing.
Code review cf588607fa36964597a6d6acc853eeb26e51b0ea
💬 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_r2298155422)
In cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
This `getnewaddress` fails as expected because 10 pre-generated keys were there in the offline before export. I understand this seems to be a caveat for the hardened ranged descriptors that few pre-generated keys must be there before export otherwise the online wallet can't create new addresses - I feel this should be mentioned in the RPC Help section wrt the usability of the online wallet where the user might
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298155422)
In cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
This `getnewaddress` fails as expected because 10 pre-generated keys were there in the offline before export. I understand this seems to be a caveat for the hardened ranged descriptors that few pre-generated keys must be there before export otherwise the online wallet can't create new addresses - I feel this should be mentioned in the RPC Help section wrt the usability of the online wallet where the user might
...
💬 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_r2298140555)
In cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
In this particular subtest, there are 5 inactive and 8 active descriptors in the `listdescriptors` RPC response after importing few descriptors. This subtest can also check that the active and inactive descriptors remain the same after export.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298140555)
In cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
In this particular subtest, there are 5 inactive and 8 active descriptors in the `listdescriptors` RPC response after importing few descriptors. This subtest can also check that the active and inactive descriptors remain the same after export.
💬 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_r2298283741)
In cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
Nit for clarity in the subtests. Otherwise it takes a while to figure out that these funds are not part of the wallet(s) under consideration.
```diff
- self.funds = self.online.get_wallet_rpc(self.default_wallet_name)
+ self.outside_wallet_funds = self.online.get_wallet_rpc(self.default_wallet_name)
```
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298283741)
In cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
Nit for clarity in the subtests. Otherwise it takes a while to figure out that these funds are not part of the wallet(s) under consideration.
```diff
- self.funds = self.online.get_wallet_rpc(self.default_wallet_name)
+ self.outside_wallet_funds = self.online.get_wallet_rpc(self.default_wallet_name)
```
💬 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_r2298309162)
In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
While calling this RPC here is fine, maybe add 10 as the default value for keypool in the node extra_args argument? That would represent a scenario that's more likely to happen in the real world.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2298309162)
In https://github.com/bitcoin/bitcoin/commit/cf588607fa36964597a6d6acc853eeb26e51b0ea "test: Test for exportwatchonlywallet"
While calling this RPC here is fine, maybe add 10 as the default value for keypool in the node extra_args argument? That would represent a scenario that's more likely to happen in the real world.
💬 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.