🤔 Prabhat1308 reviewed a pull request: "qa: Improve framework.generate* enforcement (#31403 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/31599#pullrequestreview-2559846404)
ACK [1b51616](https://github.com/bitcoin/bitcoin/pull/31599/commits/1b51616f2e3b58a1c63a19e6dba8e7e9c2aefdeb)

💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1920648795)
Not worth doing now, but in the future it'd be helpful to split up changes like this into separate commits so it's more clear to reviewers what's necessary for the described change vs what's a helpful cleanup.

👍 theuni approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2559811192)
This is much simpler than the last time I looked. Thanks ryanofsky for all the feedback here. We can argue about libexec in the other PR :)

utACK 186680acef661bd139bf3d16f494365ea55993b5

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2599106040)
> > This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.
>
> Once you figure that out, you can use a previous release to demonstrate that this works. See e.g. `mempool_compatibility.py`.

The compatibility problem was actually caught by the `feature_unsupported_utxo_db` previous release check. So passing here indicates that we're good with old blockstores (and thankfully that we have a working check for that :)

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1920688000)
The arg that `blocksdir` takes is actually the profiledir which contains the blocks dir, not the blocks dir itself. So passing blocksdir=datadir here means "use my own datadir but node0's blocksdir", which is what this test intends to cover. Unless I'm missing something?

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1920690469)
I'd rather stay old-school unless you have a strong preference :)

💬 theuni commented on pull request "depends: Always set `CMAKE_BUILD_TYPE=None` globally":
(https://github.com/bitcoin/bitcoin/pull/31661#issuecomment-2599131430)
> For packages that do not set a default build type, this change does not affect their behaviour.

Are you sure this is true? It definitely _could_ affect behavior. I'm wondering if we should do it per-package, even if it's every package, as a sign-off that we've actually checked that it does the right thing.

💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1920735492)
3474524ee56dc3d0d009fe1462092ae5d6cbd775: It should be in the solvables wallet, so we could check it in `getaddressinfo`:
```diff
@@ -1346,6 +1346,7 @@ class WalletMigrationTest(BitcoinTestFramework):
# The multisig address should be in the solvables wallet
addr_info = solvables.getaddressinfo(addr)
assert_equal(addr_info["ismine"], True)
+ assert_equal(addr_info['solvable'], True)
assert "hex" in addr_info
```

💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2599279456)
Pushed a new commit to avoid the implicit `u16` -> `u8` conversion, which for some reason i can't reproduce (and didn't hit) locally.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#issuecomment-2599279517)
rebased to address merge conflicts and dropped unnecessary logic for getting the host machine arch.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920876351)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893655528

Thanks, fixed

🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2560158721)
Thanks for the reviews and testing!

Rebased 044c1129db06983da598f427dff85513d8480b3a -> 17f25149c680c1fdb2f79dbe373e8cf820e62488 ([`pr/wrap.11`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.11) -> [`pr/wrap.12`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.11-rebase..pr/wrap.12)) to fix conflicts, also made many suggested updates and some fixes, particularly for windows.

---

re: https://github.com/bitcoi
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920878009)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893661643

Removed from documentation for now since maybe this isn't something we want to support going forward, and in any case I think it would be better to document internal commands for debugging and testing and external commands intended to be used by typical users separately.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920881130)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893680784

Thanks, fixed now

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920883094)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893702291

Thanks, added doxygen documentation and descriptions.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920883201)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893709992

> I find the names confusing because I would assume this will execute `argv0 args[0] args[1] args[2] ...`. Maybe rename `argv0` to `parent` or `wrapper_prog`.

Makes sense, and nice suggestion. renamed to wrapper_argv0. Also renamed other related variables to use wrapper_ prefix to be consistent.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920879100)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893670004

> So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either bitcoin -M gui or bitcoin -m gui. Could we spare them this? The bitcoin executable has that knowledge.

No this isn't how it works. Specifying -m or -M is optional, and the current default is to run monolithic binaries. Multiprocess binaries could become the default at some some point in the future b
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920882677)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893696352

> This adds a trailing newline, but the above 2 `tfm::format()` calls don't. I think they should.

The output looks right to me currently and trailing newlines are not missing. Adding \n to format strings would add double line breaks to the end of output and not be right, and in general there should be no need to include \n in format strings after substituting variables that already end in newlines.

If the idea is t
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920891035)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893800793

> Isn't `fs::exists()` redundant? `fs::is_regular_file()` will return true if the file exists and is a regular file.

Good catch, simplified.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920891135)
> re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893808229

Thanks fixed