💬 hebasto commented on issue "Amount field too narrow on Windows in Send Coins dialog":
(https://github.com/bitcoin-core/gui/issues/906#issuecomment-3548808342)
It appears to be an upstream bug:: https://bugreports.qt.io/browse/QTBUG-124150.

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2539134137)
to recap, single rbf carveout is nuked later in e031085fd464b528c186948d3cbf1c08a5a8d624

👍 instagibbs approved a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3479025059)
reACK 3fb3c230c93e39706b813f67006ae86c9e34e803

Minor changes, squashes and commit reorderings as suggested by other reviewers.

`git range-diff master 5b5a41b94396c9171d1a8ce076b92ac0ade54c66 3fb3c230c93e39706b813f67006ae86c9e34e803`

👍 maflcko approved a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3479097629)
re-ACK 6db2551dc27c4a9b989e8814054c93dd9d8f1b36 🍓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 6db2551dc27c4a9b989e8814054c9
...

💬 maflcko commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2539190197)
nit in 34339dbfdcc439f394eed63af6dfb4e3e066d156:

All other give a more verbose explanation, so maybe this one can be as well?

Ref:

```
$ git grep --perl-regexp 'Invalid\(BlockValidationResult::.*?, ".*?", '
src/validation.cpp: state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops");
src/validation.cpp: return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "high-hash", "proof of work failed");
src/validation.cpp: return st
...

💬 maflcko commented on issue "ci: windows-native-dll-vcpkg-* cache does not work?":
(https://github.com/bitcoin/bitcoin/issues/33685#issuecomment-3548952471)
> Cache is evicted for some reason, presumably due to low space

Good find. I guess this could be due to several branches writing windows caches. For example, right now I see `win64-native-vcpkg-binary-04fcf06a88f8571c75c5068ea9f67b8012e86d99ba204a334c696f0ec5e79551 2.6 GB cached 5 hours ago
[29.x](https://github.com/bitcoin/bitcoin/tree/29.x)
Last used 5 hours ago`. I don't think there is any value in saving caches for non-main branches, as there won't be any pull requests that could use them w
...

📝 maflcko opened a pull request: "ci: Consistenly only cache on the default branch"
(https://github.com/bitcoin/bitcoin/pull/33905)
Fixes https://github.com/bitcoin/bitcoin/issues/33685

The general idea for caches is to only save them on pushes to the default branch, because the cache is limited in size and time that the only benefit of the cache can be to speed up pull requests against the default branch.

Backport pull requests to older branches don't benefit from caches, because usually they will be running into a cache miss anyway. Also, they would cause the cache size to overflow and lead to cache misses down the l
...

💬 plebhash commented on pull request "mining: add requestedOutputs field, e.g. for merged mining":
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3549010283)
is this aimed at Sv2? if not, please disregard everything below

---

even though the `bitcoin_core_sv2` crate introduced via https://github.com/stratum-mining/sv2-apps/pull/59 is leveraging IPC to interact with Bitcoin Core, on the Rust side we're still abstracting the IO around Sv2 Template Distribution Protocol, for which there's no message that would be able to carry this kind of information

---

> Although it's possible to modify pool software to add merge mining outputs on top of
...

💬 hebasto commented on issue "ci: windows-native-dll-vcpkg-* cache does not work?":
(https://github.com/bitcoin/bitcoin/issues/33685#issuecomment-3549035364)
> So the fix would be to to only write the cache on the default branch?

Agreed.

💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3549076531)

re: https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3483436641

> I think I agree with @theuni's comment in [#30988 (comment)](https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374) that the http server and sv2 server probably would not benefit very much from using this, but that seems like more of a concern for #32061 than this PR.
>
> I would be interested to know more about how the HTTP and sv2 implementations "ended up with very non-optimal performance becau
...

💬 janb84 commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#discussion_r2539270042)
NIT (Maybe) switch order of the 2 conditions in the if statement. It's slightly more optimized and it communicated the intent better of setting one or the other extra_env.

```suggestion
if os.environ.get("GITHUB_EVENT_NAME") != "pull_request" and os.environ.get("GITHUB_REPOSITORY") == "bitcoin/bitcoin":
```

💬 plebhash commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3549117345)
> so maybe they need an additional response status argument to communicate the situation.

indeed this would be essential to make sure we know why `waitNext` failed and what we need to do about it (drop old template instead of assuming something went bad)

💬 hebasto commented on issue "Amount field too narrow on Windows in Send Coins dialog":
(https://github.com/bitcoin-core/gui/issues/906#issuecomment-3549124189)
I'm quite sure that `QStyle::sizeFromContents()` is broken for `QWindows11Style`:https://github.com/bitcoin-core/gui/blob/2444488f6ad32dcbbed51a73cd4f59ff3a239e32/src/qt/bitcoinamountfield.cpp#L147

cc @laanwj, as the initial code author :)

💬 maflcko commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#discussion_r2539394381)
sure, done. And changed to `os.environ[key]` lookup, to get KeyError on key-typos or missing keys in the future

💬 janb84 commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3549223965)
re ACK 55555db055b59dd529526915dfc59e5a13e43160

changes since last ACK:
- if condition order and `os.environ[key]` lookup

thanks for incorporating my suggestion !

💬 achow101 commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3549269412)
> we can either add that in a follow-up

I'm not merging something that doesn't compile on my machine, it will break merging of other things.

💬 achow101 commented on issue "29.x depends: fallback server missing capnp downloads":
(https://github.com/bitcoin/bitcoin/issues/33901#issuecomment-3549276978)
Backport #33580?

We can also upload the tarball.

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3549356286)
> Agree that deduplicating this as a follow-up is a good idea. Maybe even something like a fancy ForkGenerator class would make sense

I think it would also be good to have optional args that make non-empty forks to test mix-and-match cases of things re-entering the mempool / already in.

For now I think this is RFM.

💬 achow101 commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3549362236)
I think it would be better to have the `prepareTransaction` have a `sign` parameter that is set if "Create Unsigned" is selected. Having PSBT silently drop `scriptSigs` (and `scriptWitness`es too) feels like there is the possibility for misuse/unexpected behavior. `PartiallySignedTransaction` probably needs some changes to more strongly enforce its invariants.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3549372484)
> I've been playing around with the fuzz test and wanted to update here, even if it's still a bit inconclusive.
>
> I'm running into non-reproducible timeouts when using `fork` with libFuzzer. I'm overusing my cores by a lot (`fork=25` plus the 3 workers each), and I'm not yet sure if these timeouts would happen eventually when using less cores.
>
> I have "fixed" this by changing `notify_one` in `Submit` to `notify_all`, although I can't say I'm exactly sure why this works. It almost seem
...