💬 hebasto commented on pull request "ci: Avoid cd into build dir":
(https://github.com/bitcoin/bitcoin/pull/32880#issuecomment-3068845648)
> > ACK [fa0eca8](https://github.com/bitcoin/bitcoin/commit/fa0eca82ec1222ec1c68835ce7acdf9c8c4740ad), I have reviewed the code and it looks OK.
>
> I think you reviewed a commit that still had the pre-existing $GOAL bug?

I did review the correct branch but accidentally copied the wrong hash from my Sublime Merge instance. Now fixed.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3068922992)
Re https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3068613759

Thanks for writing all of that up and your detailed tour @purpleKarrot. I think you raise some excellent points on your blog, but I am not sure how I am to interpret your NACK here.

You mention that fundamental changes are required, but after reading some of your proposed changes in `/btck` I am not sure how materially different those are from what is proposed here. I think naming conventions is probably the easiest w
...

🚀 fanquake merged a pull request: "ci: Avoid cd into build dir"
(https://github.com/bitcoin/bitcoin/pull/32880)

💬 purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069008389)
> am not sure how I am to interpret your NACK here.

Just as "I think this PR should not be merged in its current form." I definitely do agree with the approach of adding a C API.

Regarding the other points, maybe we should have a private discussion.

💬 maflcko commented on pull request "[POC] ci: Skip compilation when running static code analysis":
(https://github.com/bitcoin/bitcoin/pull/32953#discussion_r2204607453)
It seems fine to wait a few months until the 26.04 tag is available and then just use that. Also, it seems fine to just require cmake 3.31 (or higher) if anyone wants to use the codegen target, but no strong opinion.

💬 fanquake commented on issue "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]":
(https://github.com/bitcoin/bitcoin/issues/32965#issuecomment-3069047625)
https://github.com/bitcoin/bitcoin/actions/runs/16263971992/job/45915380111#step:13:114

💬 Sjors commented on pull request "descriptor: don't underestimate the size of a Taproot spend (instead, overestimate it)":
(https://github.com/bitcoin/bitcoin/pull/32964#issuecomment-3069141228)
I don't think we should be overpaying by default, for two reasons:
1. it's expensive
2. it reveals the presence of script paths

An intermediate solution could be that we never sign script paths by default, i.e. make `keypath_only` the default after #32857. Only when the user opts in to script path spending do we apply this solution of using the worst case fee. Since at least that's better than the current "solution" of manually picking a higher fee rate.

💬 Sjors commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069171378)
> Over that period the USD price of BTC has risen by roughly 2-3 orders of magnitude, making the economic cost of relaying small transactions disproportionately high.

The question is whether DoS protection back then was actually sufficient, or if the increase in fiat terms was the luck we needed. I don't know the answer to that.

Another difference between now and back then is the presence of Lightning and other systems that are sensitive to mempool shenanigans in general. Again I don't if
...

💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2204657999)
In 89831dace9d21456eb4a019d86164304e22f458e _wallet/rpc: add create silent-payments wallet option_: while testing I found that it doesn't disable block filters.

🤔 fanquake requested changes to a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3016058319)
This makes the CI dependant on being run on x86_64; the tidy job will no-longer pass when run on aarch64.

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069275601)
may be good to fix the aarch64 ci. Otherwise:

re-ACK bfbf7de389649f848dffe23be6727df715648b01 🏖

<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+krxU1A3Yux4bpwZNLvVBKy0wL
...

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069279294)
> may be good to fix the aarch64 ci.

How would you fix it, given IWYU is giving different recommendations, based on the architecture?

💬 flack commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069313602)
small nit: The PR description should rather point to this commit: 9e93640be6c49fa1505ba5c5df8c89210da5a6e4

💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069317099)
ACK 44f3bae300dcafbe53f9b07e6cc22a112833e579

> Yikes. Concept ACK.

Yea. It's an interesting example of how bumping/changing a CMake minimum version, can opt you into, or out of, unexpected behaviour (I imagine this is why this stopped being the default).

✅ fanquake closed an issue: "cmake: searching across directories for config files"
(https://github.com/bitcoin/bitcoin/issues/32938)

🚀 fanquake merged a pull request: "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`"
(https://github.com/bitcoin/bitcoin/pull/32943)

💬 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-3069321057)
reACK 3afc73134bf652d01e25232701c2ef192b3fea0b

just a reminder that you don't need to rebase on master unless a silent merge conflict is detected or drahtbot yells at you about an issue merging

`git range-diff master 4ddc577953a7407ea44a3e3d8d9a4635929307ca 3afc73134bf652d01e25232701c2ef192b3fea0b`

💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069329852)
Backported to 29.x in #32863.

💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069344912)
> How would you fix it, given IWYU is giving different recommendations, based on the architecture?

Easiest way is to use `// IWYU pragma: keep` when IWYU incorrectly tells you to remove an include that should be kept and `// IWYU pragma: no_include` when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms. I dealt with this recently in https://github.com/bitcoin-core/libmultiprocess/comm
...

📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32969)
Backports:
* #32943