💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092025994)
Question: Why is it necessary to search for mt.exe here, but not in the `windows-native-dll` job above?

💬 fkorotkov commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2885174918)
Cirrus CI founder here. Just wanted to add 2 cents that we now also have a service of self-hosted runners called [Cirrus Runners](https://cirrus-runners.app/) with fast machines and bigger caches. Main difference is also that it's fixed price for unlimited minutes per month hence we buy servers off traditional cloud like AWS/GCP/Azure.

💬 Drew72-ita commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2885200231)
I recently encountered this exact issue on a production Raspibolt node. Bitcoin Core was stuck in a restart loop due to a sudden corrupted txindex file (*.ldb) (cosmic ray XD?) . It wasn’t immediately obvious what was going on—the logs showed a LevelDB error, but there was no clear indication that I needed to delete the index manually or restart with -reindex.

It took me quite some time to understand that a manual cleanup was required. At the very least, the log message should clearly explain t
...

🤔 ismaelsadeeq reviewed a pull request: "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/32509#pullrequestreview-2845113491)
Concept ACK
thanks for following up.

💬 ismaelsadeeq commented on pull request "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092055590)
In "qa: Handle --timeout-factor=0" 1235a0df5ff6278d07dc95e7acc3ccb14f536286 whats the rationale behind multiplying by factor of 60?

Wondering if this is the right fix? if I pass timeout-factor=99998 the test will fail, but on master I dont think running the functional test with timeout-factor=99998 will trigger any failure.

💬 ismaelsadeeq commented on pull request "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092055972)
Nice this is much better I think.

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

I wouldn't necessarily expect a user looking for help to know whether they are supposed to type `bitcoin help` or `bitcoin --help` or something else, so it seems reasonable to support both or these, as long as they are both fit in with other syntax we support. `git` could be a model here as well since it also supports both `git --help` and `git help`.

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

> Shouldn't we throw when that happen?

Yes, that's a good idea, added this.

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

Updated 7af6e1089ea264e870b26ac83e81e7aa374acbe1 -> a5ac43d98d1ad3ebed934f2c50208a85aae17e5e ([`pr/wrap.33`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.33) -> [`pr/wrap.34`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.34), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.33..pr/wrap.34)) just updating copyright and adding exception to handle an unexpected condition

re: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2
...

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