💬 achow101 commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2232105186)
The first sentences starts oddly to me. I would write it as

> If the legacy wallet only contains watch-only scripts and no private keys, then only `<name>_watchonly` ...

💬 theStack commented on pull request "rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes`":
(https://github.com/bitcoin/bitcoin/pull/33065#issuecomment-3120565816)
> What about these 2 (`-addresstype` and `-changetype`) in `src/wallet/init.cpp`?

Thanks, missed those due to `grep`ping too tightly, added another commit.

💬 martinatime commented on issue "getbestblockhash is sometimes taking a very long time":
(https://github.com/bitcoin/bitcoin/issues/32733#issuecomment-3120567532)
I'm using the 64bit version of PiOS and I also seem to have more than half my memory free.

$ free -mh
total used free shared buff/cache available
Mem: 3.7Gi 1.1Gi 38Mi 0.0Ki 2.5Gi 2.5Gi
Swap: 2.0Gi 2.0Gi 0.0Ki

$ uname -a
Linux raspibolt2 6.1.21-v8+ #1642 SMP PREEMPT Mon Apr 3 17:24:16 BST 2023 aarch64 GNU/Linux

$ getconf LONG_BIT
64

I can't imagine that the 32/64 bit allocations are the reas
...

🤔 cedwies reviewed a pull request: "p2p: never check tx rejections by txid"
(https://github.com/bitcoin/bitcoin/pull/33066#pullrequestreview-3056948992)
Built PR 33066 on macOS 14 (Apple Silicon) with CMake + Ninja, Debug.
- unit tests: 141/141 pass, 9 min wall time

what I checked:
‒ AlreadyHaveTx(): tx-id path now skips both reject filters; wtx-id path unchanged.
‒ MempoolRejectedTx(): parent-txid rejection bail-out removed → orphans kept even if parents sit in a reject cache
‒ updated tests (expected_behaviors table + orphan-handling cases) run green.

Question (still wrapping my head around this):
MempoolRejectedTx still inserts
...

🤔 achow101 reviewed a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-3056922125)
ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2232112912)
In 63c6d364376907c10b9baa3c6f4d72e3f1881abc "test: Migration of a wallet with `../` in path."

This is unnecessary since `migrate_and_get_rpc` also sets and unsets the mocktime.

👍 pablomartin4btc approved a pull request: "rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes`"
(https://github.com/bitcoin/bitcoin/pull/33065#pullrequestreview-3056964820)
re-utACK 251d02084688c67523e9ec92ec79ee657454ab93

💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2232211925)
The phrasing "Test that when the max weight is exceeded, only the most valuable encountered UTXOs are returned" sounds potentially misleading to me, because it could be understood as the algorithm changing to largest-first if the max weight is exceeded or the input set getting pruned beyond dropping below the weight limit, but this only tests that a solution is found that stays within the limit.

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232213129)
This is indeed a lot nicer, thanks!

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232216843)
I quickly pushed a rebase, thinking I'll follow with applying the recommendations - but ended up in a `CompressedScript` rabbithole (which I decided to leave to a follow-up in the end).

Looks like the extractions and inlines I did here ended up with lots of unnecessary casts, thanks for noticing, cleaned it up in latest push!

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3120688624)
Thanks for the recommendations, took them and extended the tests slightly to document the current behavior of `CompressedScript` (unchanged) and uncompressed P2PK as well (also unchanged).
I have also rebased to make sure all new test cases are run - ready for re-review.

💬 achow101 commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3120690351)
I'm not really a fan of changing binary locations yet again. If this is merged for v30.0, we'll have the 3 active major release branches all with different binary locations. I expect this will make my workflow a bit harder and make backports more annoying to test.

Mainly my complaint is about the locations of `bench_bitcoin` and `test_bitcoin` in builds from source since those end up being used quite a lot in my development workflow.

For the other (new) binaries that would be in libexec, I
...

🤔 naiyoma reviewed a pull request: "util: Revert "common: Close non-std fds before exec in RunCommandJSON""
(https://github.com/bitcoin/bitcoin/pull/33063#pullrequestreview-3057157243)
Concept ACK

`
From POSIX.1-2017 fork() documentation:

'A process shall be created with a single thread. If a multi-threaded process calls fork(), the new process shall contain a replica of the calling thread and its entire address space, possibly including the states of mutexes and other resources. Consequently, to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called.'
`

🤔 dr8931240-ux reviewed a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3057166764)
Thank you

💬 l0rinc commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3120795667)
Post-merge ACK

It seems the post-merge build was partially cancelled for some reason, but the follow-ups passed fully 👍

📝 naiyoma opened a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067)
This PR improves mempool_accept_wtxid.py by:

1. Using a pre-mined chain instead of generating new blocks 522bf76d7d8058872a008a721831da264881746d

2. Using MiniWallet to avoid RPC calls for signing transactions 4ec5ae9fe126ffabcd429277092e3b27f483d430

3. Removing child txid variables and using txid.hex directly 361ebd56eb817b1e054ee1aacc87bf60c06c6b94

🤔 dr8931240-ux reviewed a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3057214081)
Ok

💬 l0rinc commented on pull request "test: Move `script_assets_tests` into its own suite":
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-3120838340)
test-only post-merge ACK

It's reporting `script_assets_tests` as skipped on Mac

<details>
<summary>Details</summary>

```bash
129/144 Test #86: script_assets_tests ..................***Skipped 0.36 sec
...
The following tests did not run:
86 - script_assets_tests (Skipped)
```

</details>

and on Linux

<details>
<summary>Details</summary>

```bash
139/144 Test #86: script_assets_tests ..................***Skipped 0.20 sec
...
The following tests did not
...

💬 l0rinc commented on pull request "ci: Run unit test parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33000#issuecomment-3120840192)
Concept ACK

💬 l0rinc commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3120847592)
> Looks like this was introduced in https://github.com/bitcoin/bitcoin/commit/421218d3040279c84616891e8d14b05576b07c57

Maybe @sipa can help us out here: should we try making read/write symmetric (either by `write` returning, or `read` throwing) or is the current refactor the cleanest direction?