💬 rstmsn commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2863484560)
The OP_RETURN `-datacarriersize` limit is 100% effective in cases where miners use it to filter their local mempools & inform template construction, prior to successfully mining a block.

This viable use case would be harmed as a result of this PR. On that basis, firm NACK.

💬 fanquake commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2863491522)
Backported to 29.x in #32292.

💬 fanquake commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-2863507309)
@laanwj you might be interested in circling back here to review?

💬 hebasto commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126)
nit: Is this really needed considering:https://github.com/bitcoin/bitcoin/blob/d8caa10c29321c9732dde3d4ccdcf0413041b33f/src/randomenv.cpp#L27 ?

👍 hebasto approved a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189)
Approach ACK d8caa10c29321c9732dde3d4ccdcf0413041b33f. I've reviewed the code. Still waiting for my Guix build.

🤔 hebasto reviewed a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825611533)
My Guix build:
```
aarch64
5bde67b2713e02dbc6fcf5aa5b90e21df89f7fd04debb4ba13302a8e6bcabc53 guix-build-d8caa10c2932/output/aarch64-linux-gnu/SHA256SUMS.part
09eeaa166590208ce1cdd5bd9298f4819a8b6014fdaf68f421f4797f5bc2d034 guix-build-d8caa10c2932/output/aarch64-linux-gnu/bitcoin-d8caa10c2932-aarch64-linux-gnu-debug.tar.gz
2ecfed14ba3fd7ee565969023e78b810a432042f719c30dd7962b6e070d712ef guix-build-d8caa10c2932/output/aarch64-linux-gnu/bitcoin-d8caa10c2932-aarch64-linux-gnu.tar.gz
1924ef5f
...

💬 fanquake commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079998731)
I've dropped the addition.

👍 hebasto approved a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825625856)
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a. Only addressed my [comment](https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126) and rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189).

💬 ismaelsadeeq commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-2863565098)
> Is this something we need to support? In a sense the current mechanism is honestly saying it doesn't have enough data.

I think it is. Even based on recent mainnet network activity, it’s possible for us to see frequent empty blocks. In cases where the estimator have seen enough blocks to be able to have the data but there is just no activity, paying the floating minfee is sufficient to get transactions to miners. I think this is the sane thing to do.

In test networks, this has been the r
...

📝 hebasto reopened a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669)
This was suggested in https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2541288435:
> An alternative would be to require python to be disabled explicitly. Otherwise, it seems odd that every setting in cmake has a static default that can only be overridden explicitly, except for some, which are silently downgraded?

This PR updates the build system to fail by default on systems where the minimum required Python version is unavailable. It introduces an option that allows users to exp
...

📝 hebasto converted_to_draft a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669)
This was suggested in https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2541288435:
> An alternative would be to require python to be disabled explicitly. Otherwise, it seems odd that every setting in cmake has a static default that can only be overridden explicitly, except for some, which are silently downgraded?

This PR updates the build system to fail by default on systems where the minimum required Python version is unavailable. It introduces an option that allows users to exp
...

💬 fanquake commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2863586774)
Guix Build
```bash
01f0af9244fe88c5e610b53cd4cc7e2c969f15ea6dc7aab8891db66f0ed2131d guix-build-dfeac76b951f/output/aarch64-linux-gnu/SHA256SUMS.part
e992511fece7d724c4b0e2d6153fdcf179b11e7beb07856a4a6206df3a0ebf14 guix-build-dfeac76b951f/output/aarch64-linux-gnu/bitcoin-dfeac76b951f-aarch64-linux-gnu-debug.tar.gz
9a6bd7181092a4373fe23345b8fa8f2f2bba26ca188f6b28b38161addab82842 guix-build-dfeac76b951f/output/aarch64-linux-gnu/bitcoin-dfeac76b951f-aarch64-linux-gnu.tar.gz
a1682fa97dd0212ce
...

👋 polespinasa's pull request is ready for review: "rpc: print P2WSH and P2SH redem Script in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/31252)

💬 polespinasa commented on pull request "rpc: print P2WSH and P2SH redem Script in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r2080054943)
done

💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2080055225)
It was introduced by e679ec969c8b22c676ebb10bea1038f6c8f13b33 in preparation for OP_EVAL (which was eventually replaced by P2SH). It seems the initial goal was to have a backward compatible sigop check:

```cpp
// This code should be removed when a compatibility-breaking block chain split has passed.
// Compatibility check for old clients that counted sigops differently:
```

The commit drops the `GetSigOpCount()` helper and copy-pasted its implementation into `CheckBlock()`, where it pre
...

💬 polespinasa commented on pull request "rpc: print P2WSH and P2SH redem Script in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r2080060094)
Thanks for the suggestion, sorry for the delay, got this PR a bit abandoned, re-taking it now :).

The problem is that I don't see the way to detect when is P2SH or P2WSH without the previous transaction.
Your patch is similar to my first approach. If you test with a P2SH-P2WPKH, the decoding is done wrong.

📝 furszy opened a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449)
Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.

This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.

This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.

💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2863644963)
> Not sure about compiler specific changes, that rely on the presence of an undocumented environment variable (which we don't explicitly set as far as I can tell?), to make tests work properly.

It is documented, and it is pre-defined by CI itself:
- in [GitHub Actions](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables):
> `CI` | Always set to `true`.

- in [Cirrus CI](https://cirrus-ci.org/gui
...

💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2080084146)
Good catch, and this was helpful in figuring out the history...

💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2863671202)
I expanded the comment to mention that p2sh isn't counted either.

I also added some history to the commit message. This seems to be a case of: none of the reviewers of #748 brought it up, but there were no reviewers :-)

Also rebased.