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

💬 achow101 commented on issue "depends: fallback server missing Qt downloads":
(https://github.com/bitcoin/bitcoin/issues/33898#issuecomment-3549403050)
The file is located at https://bitcoincore.org/depends-sources/CMakeLists.txt-6.7.3

Not sure how to have download figure that out.

🤔 maflcko reviewed a pull request: "qa: Improvements to debug_assert_log + busy_wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3479655338)
review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a 💨

<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: review ACK a1f76230209629c514198
...

💬 jlopp commented on pull request "BIP-322 basic support":
(https://github.com/bitcoin/bitcoin/pull/24058#issuecomment-3549483423)
For the record, I do believe there is a strong use case for this feature. In particular, due to the travel rule. We have clients who get requests to prove the ownership of their wallet and since they can't use the legacy message signing option with their multisig wallet, the exchanges and other service providers are requiring them to send dust-level amounts of bitcoin to prove control. This, of course, is not really a viable long-term strategy.

💬 achow101 commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3549508955)
> To compute the short channel Id we simply call `getrawtransaction` which gives us the block hash for the block that confirmed the tx, then load that block to get the tx position in that block.

So you need txindex and make 3 RPC calls? ISTM storing the position in this new index would make it much more efficient and more useful to other people.