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

I can believe that it might not be good to allow certain options with commands, but would be helpful to know some specific examples you think are problems?

I don't like suggested change very much because it seems to provide less information to users while making the logic more deeply nested and complicated, and doesn't seem to follow the pattern of trying to provide usage information when the command is misused.

Bu
...

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

Nice catch finding that omitting an RPC command produces a potentially confusing error message:

```sh
$ bitcoin rpc
error: too few parameters (need at least command)
```

I think it would be good to address in a followup since it will require changing bitcoin-cli, but I think `bitcoin-cli` should behave have more like `bitcoin` and `git` and output usage information and a failing exit code when misused.

I thi
...

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

Which list of executables is this referring to? No executables are hidden currently, though https://github.com/bitcoin/bitcoin/pull/31679 is a PR that could hide them. (That PR is a little stuck, but I think it will become easier to understand if this PR is merged.) This PR should just provide a new way of starting executables, and allow some of them them be hidden, but not actually hide anything itself

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

> Note that some RPCs take Base64 parameters (finalizepsbt, verifymessage), and Base64 is commonly padded with `=`.

Wow, that's good to know about and looks like `bitcoin-cli -named` will not currently handle these well, just treating everything before the [first](https://github.com/bitcoin/bitcoin/blob/725c9f7780e0def3d79be151c08a36fe7a9dc59c/src/rpc/client.cpp#L374) '=' character as the parameter name and everything
...

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

Thanks! Updated

🤔 fjahr reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2845077479)
Code review ACK ee045b61efc1479c1866b786661ef39a863677d0

💬 fjahr commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2092035407)
Since there is now a `PSBTError::OK` it seems like there is no need for using an `std::optional<common::PSBTError>` return type. But this can be made more consistent in a follow-up.

💬 fjahr commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2092030045)
nit: Having an `OK` variant makes this more into an `PSBTResult` rather than `PSBTError`

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092076046)
Ah, I see, this is related to the discussion above. (https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077252899).

This job's prompt is not configured as a visual studio prompt, so it doesn't have the visual studio sdk executables in it's path, unlike the job above.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2885251842)
The main change in the latest force push is to address the [review comment](#discussion_r2044004503) about `validation_flush_tests.cpp`. I ended up almost completely rewriting the `getcoinscachesizestate` test case; it was kind of a mess. It was far too fragile because it required equality of various values related to memory usage (which is why this test started failing with this PR, even though nothing had actually broken). This is especially difficult when a test must run in both the 32-bit an
...

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092095535)
Fixed.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092092067)
That constant came from your [comment](#pullrequestreview-2697495377) (which I've adopted into this PR), so I really don't know how to document this value. Where did you get it from? This stuff tends to have magic numbers; I don't think this PR makes it any worse.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092094020)
Yes, and there is a fairly detailed comment on lines 194-195 (just before the `unordered_set` version of `DynamicUsage`), so I think that's sufficient. But if you feel strongly, I'll try to come up with something.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092095271)
I think the comment immediately above explains it sufficiently, and the "1" was not a named constant either (so this pull is not making it worse). But if you feel strongly, I can do something. (As someone once said, the hardest part of programming is coming up with good names!)

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092102849)
Agreed that checking just one is sufficient to verify that the function works for every exe the cmake function is invoked on, but it seems reasonable to me to glob check each executable in the `bin` folder, to enforce that e.g. new executables get the manifest embedded and existing ones don't silently lose them:

```powershell
Get-ChildItem -Filter "bin\*.exe" | ForEach-Object {
echo "Checking $($_.Name)"
mt.exe -nologo -inputresource:$_.FullName -validate_manifest
}
```

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092105581)
I think a more general approach as described here: https://github.com/microsoft/vswhere/wiki/Start-Developer-Command-Prompt#using-powershell would be better, but I think this question is better suited for #32513.

📝 w0xlt opened a pull request: "util: C++20 `ToIntegral()` Improvement"
(https://github.com/bitcoin/bitcoin/pull/32522)
While reviewing https://github.com/bitcoin/bitcoin/pull/32520, I noticed that the function `ToIntegral()` could be improved:

* Instead of manually asserting `std::is_integral_v<T>`, the template can be constrained with the standard `std::integral` for clearer intent.

* Annotation with `[[nodiscard]]` and `noexcept`. Compilers will warn if the return value is discarded and since `std::from_chars` is guaranteed to throw nothing, declaring the wrapper `noexcept` makes the exception guarantee
...

🤔 w0xlt reviewed a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2845212748)
ACK https://github.com/bitcoin/bitcoin/pull/32520/commits/fa24012c181850d4adc5cca0fdba76d584ec3a00

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2885338351)
Reviewed and tested ACK https://github.com/bitcoin/bitcoin/commit/8f4fed7ec70093e2535423d63e9f9dd400c378ac

<details><summary>
Checked that each executable has a manifest.
</summary>

```powershell
PS Z:\bitcoin-8f4fed7ec700> Get-ChildItem -Filter "bin\*.exe" | ForEach-Object {
>> echo "Checking $($_.Name)"
>> mt.exe -nologo -inputresource:$_.FullName -validate_manifest
>> }
Checking bitcoin-cli.exe
Parsing of manifest successful.
Checking bitcoin-qt.exe
Parsing of manifest suc
...

💬 gituser commented on issue "listunspent, fundrawtransaction, getwalletinfo locks wallet for any other operation":
(https://github.com/bitcoin/bitcoin/issues/27002#issuecomment-2885340621)
> its prerequisite [#27286](https://github.com/bitcoin/bitcoin/pull/27286)

Hey I'm using a build from this PR (from your branch) on production but it's not recent one, using branch 48545bdb3b9f59a8b78640dbf20afff6e2663ad7 october 2024 version.

It works much better, but there is still `fundrawtransaction` lock issue I think.