💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077721654)
I'm not confident anyone is going to followup with this, and don't see the rush to merge this PR. The fact that this code is already being used, doesn't warrent adding more of it.

🚀 fanquake merged a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710)

✅ fanquake closed an issue: "Proposed Timeline for Legacy Wallet and BDB removal"
(https://github.com/bitcoin/bitcoin/issues/20160)

💬 fanquake commented on issue "Proposed Timeline for Legacy Wallet and BDB removal":
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-2858784044)
Closing, given #28710 is merged.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077753175)
Good point, I agree that the `all` predicate makes much more sense than `any` here, not sure what motivated me to use the latter back then. Fixed.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077754605)
Good idea, done.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077754899)
Leaving that one for a follow-up.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077754185)
Right, strictly speaking they are not needed. I added them because we seem to do this also in many other functional tests, and I think it's nice to differentiate between "attribute is not available" and "attribute doesn't match the expected value" more explicitly by failing in different lines. Happy to drop them though if others feel strongly.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#issuecomment-2858796140)
Thanks for the review @rkrux, I rebased on master and addressed most of your review comments.

💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077754724)
In "doc: Mention bitcoin wrapper executable in documentation" b0989e3095f1964fdb523cdd4b44f0b198417014

I think it would be better to provide a single usage instruction for users. Otherwise, having separate instructions for both `bin/bitcoin` and `bin/bitcoind` in all places could be confusing.

Instead, I suggest we standardize on `bin/bitcoin` in all places, while still supporting the previous one for backward compatibility.

The release notes have clearly indicate this change to users
...

🤔 ismaelsadeeq reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821963515)
Concept ACK

💬 maflcko commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2077794853)
nit: "should recorded" looks like a typo

should recorded → should record

💬 maflcko commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2072667393)
nit in 93563ecef5e1be1522d4784b0092737487156bab: wallt → wallet

💬 maflcko commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2072667513)
tranasctions → transactions

🤔 pablomartin4btc reviewed a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2822045930)
post-merge ACK de054df6dc32cbd8b127c6761d9c65d95025e08f

🤔 hodlinator reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821205668)
Concept ACK 81c0b9edfe533afbb2f4dda56142afdedffdb347

Introducing a `bitcoin` wrapper command seems like a good step to improve discoverability of the project's functionality.

Only briefly tested on NixOS (Linux) for now.

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077503435)
nit: Could use util/string.h:
```suggestion
for (const auto& element : Split(std::string_view{path_env}, ':')) {
fs::path candidate = fs::path{std::string_view{element.data(), element.size()}} / argv0_path;
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077515690)
nit:
Could keep style consistent with function body rather than wrapped function.
```suggestion
int ExecVp(const char* file, char* const argv[])
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077299356)
Potential for use after free?

This relies on `escaped_args`-elements being moved around with the backing `char`-buffers remaining in place as the vector is resized. It probably holds for all stdlib-implementations by now, but might be slightly safer to declare and fill `new_argv` after this loop, or resize `escaped_args` ahead of the loop.

Maybe there's some standardeese reason this is guaranteed to work that I'm unaware of.

Could use `const_cast`?

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077548018)
nit: Could verify we succeeded:
```suggestion
assert(path.has_parent_path()); // Failed resolving directory.
}
```