💬 m3dwards commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260511885)
Can you elaborate a little more on what this would look like? Would this be different labels for different jobs? As in a label that runs the Previous Releases job and a different label that would run a different job?

💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260543464)
I don't know. Not sure if we want to see pull requests spammed with "added and removed" label events (https://github.com/testing-cirrus-runners/bitcoin/pull/19#event-19014793971).

GitHub doesn't make this easy and I wonder if we may want to just spin our own CI to detect silent merge conflicts.

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260557261)
how do evictions happen, if `export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:-500M}` is set to 500M in total for all tasks?

💬 maflcko commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2260562862)
```suggestion
LogInfo("Failed to detect filesystem type for: %s", util::Join(error_paths, ", "));
```

nit: no need for newline in new code

💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260589218)
To give some more context: We are now trying to add 200+ lines of code to this repo which have to be maintained, but they cause label spam, and likely aren't sufficient to achieve the goal, because the ci-failed notification will go to the one adding the label and triggering the workflow?

If so, we'd have to create a comment (or other notification) anyway. So if additional glue code is needed, we might as well handle all code externally.

🤔 Crypt-iQ reviewed a pull request: "validation: detect witness stripping without re-running Script checks"
(https://github.com/bitcoin/bitcoin/pull/33105#pullrequestreview-3097524181)
ACK 370770fb361fdb745cfa7bb9abc7ee4a7833b83b modulo commit message change and test comment fix

💬 Crypt-iQ commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260577684)
should be "P2SH-wrapped P2WPKH"

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260635134)
Good question; remote backends are not governed by client-side CCACHE_MAXSIZE settings, see https://ccache.dev/manual/4.11.3.html#_http_storage_backend for a bit more info.

Basically the onus is on the server to periodically run `ccache --trim-dir` to keep it under control (we have asked for more details on this process cc @m3dwards )

💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259731802)
In "[test] check miner doesn't select 0fee transactions"
2778e4691664c55d5699bca2b68f15796e8a9a75

Maybe we should test both ways.
1. When the `blockmintxfee_btc_kvb` is > 0 we verify that we never create 0 or less fee tx.
2. When a 0 or less fee tx is created it is because of `blockmintxfee_btc_kvb` being 0.

```suggestion
for txid in block_template_txids:
for txid in block_template_txids:
fee = node.getmempoolentry(txid)['fees']['base'
...

💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259885809)
In "[test] check bypass of minrelay for various minrelaytxfee settings" d40de30fdf0470a4ec6712aef4d5e8ed40e5ea91


We dont need the cleanup here I think
```suggestion
def test_minrelay_in_package_combos(self)
```

👍 ismaelsadeeq approved a pull request: "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee"
(https://github.com/bitcoin/bitcoin/pull/33106#pullrequestreview-3096282456)
Code review ACK a43e1b28b2899e1707e7867fd46efe9fcc08f241

Although it's worth mentioning that there is a scenario were the price could crash to the point where the default min relay allows for free relay or atleast close to free, but a counter argument to that is that even current default is susceptible to such scenario.

> minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represen
...

💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2260507858)
I think `blockmintxfee` should be deprecated and eventually be removed.
What should be in the block template should just depends on the mempool transactions

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260658713)
Would it not be easier for Cirrus to just fix the reverse-age order to pick the latest instead? Or somehow work around this by embedding the epoch seconds in the ccache blob name?

💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164662786)
> Also, the error string CMake is throwing, is also incorrect, as the compiler does support LTO.

CMake doesn't blame the compiler, rather itself: "CMake doesn't support IPO for current compiler".

For some reason, this occurs with the "Unix Makefiles" generator but not with the "Ninja" generator.

💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164664412)
cc @purpleKarrot

💬 maflcko commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3164698282)
This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it'll likely miss the milestone.

💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164733222)
It seems that the `$<TARGET_OBJECTS:..>` added to `bitcoinkernel` sources in #33077 are causing the issue.

cc @theuni

🤔 pinheadmz reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3097669539)
3e7abceecf -> 598bee6bd5

Address nits from @jonatack including commit messages that indicate code is copied from CConnman and will be modernized in following commits.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2260675972)
fixed, thanks

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2260702590)
Thanks, removed the years entirely which seems to be the style for new files.