💬 maflcko commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3219797956)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

🤔 151henry151 reviewed a pull request: "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3150945450)
Would it be better to leave the comments there and simply remove the TODO tag? It looks a bit like a comment was cut in half there, but perhaps adding back in the
```The `CMAKE_SKIP_BUILD_RPATH` variable setting has been removed```
Would be better than removing the rest of that information. What do you think?

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3219963543)
Holding off on rebasing until #32876 is merged or I have to touch it.

💬 Sjors commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#issuecomment-3220029611)
@Prabhat1308 can you update your review to the latest version?

I've rebased it just in case.

💬 Sjors commented on pull request "wallet: support bip388 policy with external signer":
(https://github.com/bitcoin/bitcoin/pull/33008#issuecomment-3220037327)
@pythcoiner thanks, updated the description.

👍 hodlinator approved a pull request: "[IBD] flush UTXO set in batches proportional to `dbcache` size"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-3128539265)
ACK 956a6b454704120ba4c482d7157db89c20130e75

Change scales hidden `-dbbatchsize` by the `-dbcache` setting if unset.

Compared IBD until block 910'000 from local peer in 3 variations (both nodes on SSD); PR change for `-dbcache` of 450 vs 45'000, and base commit for PR with 45'000.

* Baseline of `-dbcache=450` averaged 267mins across 3 runs.
* `-dbcache=45000` without this PR averaged 241mins across 3 runs (these were probably the runs where I was doing the least work on the other node)
...

💬 hodlinator commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2282516167)
I would just default to testing at compile time:
* No need to (re)run ctest/test_bitcoin to exercise checks.
* Only run when compilation unit is (re)built. Not re-run when iterating on test code in other compilation units.
* Inconsistency might push other tests towards being converted to compile time, which I would say is a positive effect.

💬 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_r2298006417)
I clicked commit suggestion too quickly -- It looks a bit like a comment was cut in half there. Would we want to remove the additional lines of the TODO as well, or rephrase them as comments?

💬 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)

💬 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).

💬 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"

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

💬 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

👋 frankomosh's pull request is ready for review: "fuzz: add target for `DifferenceFormatter` and p2p invariant check"
(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
|----
...

🤔 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

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

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

💬 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)
```

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