📝 maflcko opened a pull request: "Use WitnessV0KeyHash in TestAddAddressesToSendBook"
(https://github.com/bitcoin-core/gui/pull/875)
Fixes https://github.com/bitcoin/bitcoin/issues/32558
Fixes https://github.com/bitcoin-core/gui/issues/874

This fixes a bug introduced in commit fafee85358397289aa4c6b799d2603a5d89e83a2, which changed the type of the dummy address from `WitnessV0KeyHash` to `PKHash`. It was expected that this is fine, given that this is just a dummy address. However, the base58 characters can include the substring "io", leading to test failures later on.

Fix it by just using `WitnessV0KeyHash` again.

F
...

💬 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-2890853944)
(created fix)

💬 willcl-ark commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2890875967)
Concept ACK.

https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/validationinterface.h#L11
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/cuckoocache.h#L10

The only others I found were in subtrees.

💬 fanquake commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2890881787)
Thanks, dropped.

💬 maflcko commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2095633829)
> Is there really a scenario where we could be reading a block, but not have its block index entry?

I can't see any, but this would require some more refactors

📝 fanquake opened a pull request: "[28.x] Backport #31407"
(https://github.com/bitcoin/bitcoin/pull/32563)
Backports #31407 + #32003.

🤔 Sjors reviewed a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2850636522)
Concept ACK

If only to remind ourselves that 32-bit systems exist.

Before #28358 we automatically rounded down the number, which wasn't great.

💬 Sjors commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095631168)
Maybe clarify this with something like:

```cpp
constexpr is_32bit{sizeof(void*) == 4};
```

💬 Sjors commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095631253)
ea561779abfb2298a9f20ceafa9e05fe2c58d841: let's just use 4096? This setting doesn't check actual memory capacity on 64-bit systems either, so we don't need to be this protective.

💬 Sjors commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095634131)
ea561779abfb2298a9f20ceafa9e05fe2c58d841: as with `-maxmempool`, I don't think we should be too "helpful" and just limit it to the physical maximum.

💬 fanquake commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2890884936)
I've opened something (not-yet-fully-tested) here #32563 for `28.x`.

💬 fjahr commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2890891024)
Concept ACK

I kind of like the idea but seems way too much effort to actually maintain this in a consistent way for very little gain. I don't remember if any of these ever saved me time, probably not because my workflow doesn't rely on them and most people probably use an editor that allows them to jump to definition.

💬 Sjors commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2890893688)
Concept ACK

💬 fjahr commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-2890904040)
Concept ACK

💬 fanquake commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32479#issuecomment-2890914607)
I've added a commit to drop ` --enable-external-signer` from the global CI config.

💬 fanquake commented on pull request "cmake: Add missed `SSE41_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2890918551)
Just roll this into #32551?

💬 fjahr commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#issuecomment-2890931358)
Concept ACK

💬 Sjors commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2890932134)
> > Do we have a linter for unused includes?
>
> No includes are being removed here, so I don't see how that's relevant? IWYU would be the closest tool you could run.

The comment were useful for knowing when an include could be dropped (though not if they're incomplete).

💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2095663048)
That would make the string_view point to the temporary returned by `substr()`. That temporary is destroyed after the end of the statement, so the string_view remains with a dangling pointer. Like in the example in https://en.cppreference.com/w/cpp/string/basic_string/operator_basic_string_view which says "// Bad: holds a dangling pointer".

Or a small standalone program to demonstrate:

```cpp
int main(int, char**)
{
std::string s{"abcd"};
std::string_view sv{s.substr(2)};
s
...

💬 fjahr commented on pull request "test: properly check for per-tx sigops limit":
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2890950605)
Concept ACK