💬 hebasto commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2890611969)
Friendly ping @laanwj @davidgumberg @hodlinator @sipsorcery :)

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095472966)
Although there are no `break` or `continue` statements in the loop, it would feel a bit more robust to move this check below the loop.

🤔 rkrux reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2850393076)
Benchmarks at 29cf059e6e592f7f82b982493a043219cdfa5b40

An update to my previous comment: https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2848375525

I dug a bit more and realised that in order to have a fair assessment between master and the PR, the `AvailableCoins` benchmark changes of the last commit 29cf059e6e592f7f82b982493a043219cdfa5b40 must be present in master as well so that both the results come from the same benchmark cases. I gcp'ed the commit onto master, remove
...

💬 laanwj commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2890660965)
Concept ACK

💬 janb84 commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2890663386)
Would move the remark of the -rpcwhitelist to the end of the section and add something about the -rpcwhitelistdefault function that is needed to set the default on no whitelist for all users. The added documentation is still strong imo 👍

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095501048)
Could add: `The per-transaction sigop limit introduced in BIP54 does not cover the witness.`

✅ maflcko closed a pull request: "rev_32343_wip_nomerge_ci"
(https://github.com/bitcoin/bitcoin/pull/32535)

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095516997)
// Unlike the block wide sigop limit, the BIP54 per
// transaction limit includes the prevout scriptPubKey.

📝 fanquake opened a pull request: "doc: remove // for ... comments"
(https://github.com/bitcoin/bitcoin/pull/32562)
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs, if changed.

They are also wrong, i.e `DEFAULT_SCRIPTCHECK_THREADS` is not in
`validation.h`.

💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2095520803)
`libressl` ?

💬 maflcko commented on issue "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]":
(https://github.com/bitcoin/bitcoin/issues/32558#issuecomment-2890705811)
> A duplicate of [bitcoin-core/gui#874](https://github.com/bitcoin-core/gui/issues/874)?

Haven't seen it, and created it here, because GUI CI failures affect this repo as well. Feel free to close, though.


I can't reproduce this locally, so maybe printing the rows on failure could help to debug this further via CI?

💬 fanquake commented on issue "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]":
(https://github.com/bitcoin/bitcoin/issues/32558#issuecomment-2890712591)
I'd say leave it here, given it doesn't look like anyone is following up, or debugging, in the GUI repo.

💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095321942)
(same)

💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095532020)
forgot to remove the include?

```
[12:05:51.093] /ci_container_base/src/util/check.cpp should remove these lines:
[12:05:51.093] - #include <clientversion.h> // lines 9-9
[12:05:51.093]
```

However, the `CLIENT_NAME` symbol is still used in the kernel, so I am not sure removing the link dependency is meaningful. I'd say it should either be removed fully, or it can be kept. If you just want to turn the link-time dependency into a compile-time dependency, this is also possible, because
...

💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095321637)
```suggestion
LogWarning(STR_INTERNAL_BUG(strprintf("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i",
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex))));
```

nit: Could use the `STR_INTERNAL_BUG` helper, while touching this?

💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2095540696)
Right. But I prefer to be explicit rather than feed `SetProxy()` an invalid value and rely on it doing the "right thing". It is not documented what `SetProxy()` would do with an invalid input, so I prefer to have these checks.

🤔 janb84 reviewed a pull request: "docs: clarify RPC credentials security boundary"
(https://github.com/bitcoin/bitcoin/pull/32424#pullrequestreview-2850500482)
re ACK https://github.com/bitcoin/bitcoin/commit/d6f622f36cfcf04d94f96ea360e346c5b9337be4

Changes since last ACK:
- Added text to clarify that there are additional flags to restrict RPC access

💬 laanwj commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout)":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890755002)
Yes, bisecting it would be neigh-impossible, given that it seems impossible to reliably reproduce. i already looked into the git history of `directory_iterator` in the `gcc/libstdc++-v3` and wasn't able to find any bug fix that would explain the behavior here, or how it was fixed. So it's likely something indirect.

💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2095560957)
That would require some convoluted juggling with the parameters. Like:

1. `-proxy=addr:port=ipv6` sets the "ipv6" and "name" proxy, so that just a single `-proxy=1.2.3.4:5678=ipv6` works as expected. However:
2. `-proxy=0=ipv6` removes _only_ the "ipv6" proxy and not the "name" proxy, so that what you describe works: `-proxy=addr:port -proxy=0=ipv6`.

Another option would be to provide a separate name proxy, e.g. `-proxy=addr:port=name` or `-proxy=addr:port=dns`. But then users will probab
...

💬 Sjors commented on pull request "test: properly check for per-tx sigops limit":
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2890768586)
It would be nice to also have a test (against an accidental soft fork) that fails if you change:

`MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5 -1};`