💬 Sjors commented on pull request "rfc: only put replaced txs in extra pool":
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2885815673)
Thanks everyone for their feedback. My current thinking is that there's two parallel ways forward:

1. Create a separate pool just for fee bumps, move those out of the extra pool
2. Harden the extra pool using the mechanism introduced in #31829

Fee bumps themselves are more DoS resistant than the other categories in extra pool. By moving them to their own structure (1), they can't be overcrowded by the other stuff. In that case there's no need to limit the extra pool; it's small, best case
...

✅ Sjors closed a pull request: "rfc: only put replaced txs in extra pool"
(https://github.com/bitcoin/bitcoin/pull/32510)

💬 maflcko commented on pull request "init: drop `-upnp`":
(https://github.com/bitcoin/bitcoin/pull/32500#issuecomment-2885816473)
> > An unknown upnp entry in settings.json would just be ignored?
>
> Confirmed.

Thanks for checking. I guess I was confused and the comment added in https://github.com/bitcoin/bitcoin/pull/31198/files was wrong?

review ACK 301993ebf7f8ec23050e91377e0fd05823bb372a

Also checked that unknown entries are ignored.

💬 maflcko commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2885823606)
> `std::string` no, it’s always a runtime, owning, heap‐allocated string, so it won´t work to produce the RPC docs.

Not sure what you mean by "won't work". What is the exact diff to reproduce the compile failure or error?

💬 maflcko commented on issue "Running out of memory on a 2GB box - Initializing chainstate Chainstate [ibd] @ height -1 (null)":
(https://github.com/bitcoin/bitcoin/issues/31573#issuecomment-2885831745)
26.0 is EOL. Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?

Did you compile a debug build? Are you using the release bins?

👍 hebasto approved a pull request: "build: Revert "Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC"
(https://github.com/bitcoin/bitcoin/pull/32525#pullrequestreview-2845701454)
ACK fa0a4473a88017e525829ecd3db47f536c35fb62.

Should a note about the broken MSVC 17.12 be added to the build docs?

💬 hebasto commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2885835138)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/32525.

✅ hebasto closed a pull request: "cmake: Restrict MSVC-specific workaround to affected versions"
(https://github.com/bitcoin/bitcoin/pull/32499)

💬 maflcko commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2885841787)
> `std::string_view` is known (and usable) at compile time as a view over some static data (it can be constexpr), so it can be concatenated to RPC docs. [1]

Also, I don't think this is true. I don't think it is possible in C++20 to concatenate two string_view into a single string_view at compile time.

💬 davidgumberg commented on pull request "build: Revert "Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC":
(https://github.com/bitcoin/bitcoin/pull/32525#issuecomment-2885848315)
ACK https://github.com/bitcoin/bitcoin/pull/32525/commits/fa0a4473a88017e525829ecd3db47f536c35fb62.

I would be partial to disabling this for MSVC 17.12 in the vein of #32499, I mean if it is known to be broken, and it's documented that it's broken, one might as well just disable it?

But, edge case of an edge case, so this seems fine to me.

💬 davidgumberg commented on pull request "build: Revert "Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC":
(https://github.com/bitcoin/bitcoin/pull/32525#issuecomment-2885854792)
reACK https://github.com/bitcoin/bitcoin/commit/fa0a4473a88017e525829ecd3db47f536c35fb62

📝 maflcko opened a pull request: "fuzz: Delete wallet_notifications"
(https://github.com/bitcoin/bitcoin/pull/32526)
The fuzz target has many issues:

* I has never found a meaningful issue.
* It is still slow, despite https://github.com/bitcoin/bitcoin/pull/28933 and https://github.com/bitcoin/bitcoin/pull/31238.
* It is unmaintained, see https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814654792 (missing meaningful coverage) or https://github.com/bitcoin/bitcoin/pull/31238#issuecomment-2460821784 (unstable) or https://github.com/bitcoin/bitcoin/pull/31467#issuecomment-2672649759 (fix slowness)
...

💬 hodlinator commented on pull request "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092465291)
> whats the rationale behind multiplying by factor of 60?

Default timeout-factor is 1, so we get 60 seconds.

Here's a similar pre-existing example:

https://github.com/bitcoin/bitcoin/blob/c47f634718d4248fd2a30e51a57944f89da72a64/test/functional/test_framework/test_framework.py#L765-L773

> on master I dont think running the functional test with timeout-factor=99998 will trigger any failure.

99998 does fail feature_framework_startup_failures.py on master, but skipping back before th
...

✅ maflcko closed an issue: "wallet: Data race in GetOrCreateLegacyScriptPubKeyMan vs IsMine"
(https://github.com/bitcoin/bitcoin/issues/27354)

💬 maflcko commented on issue "wallet: Data race in GetOrCreateLegacyScriptPubKeyMan vs IsMine":
(https://github.com/bitcoin/bitcoin/issues/27354#issuecomment-2885915250)
closing for now. unless there are steps to reproduce how a real user can be affected by this, it may be best to postpone this for now.

👍 hodlinator approved a pull request: "build: Revert "Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC"
(https://github.com/bitcoin/bitcoin/pull/32525#pullrequestreview-2845791215)
ACK fa2c6623626719b338880f7bb097b902019d5956

Confirmed at least later patch-versions of 17.13 work here: https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2883487720

It's possible that the regression in 17.12 never made it to any of the patch-versions of 17.13.

🚀 fanquake merged a pull request: "build: Revert "Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC"
(https://github.com/bitcoin/bitcoin/pull/32525)

💬 ajtowns commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2885938653)
ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18 -- only minor tweaks since previous review

[I wrote](https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2832143503):

> I don't think this PR will have any impact on fee estimation (the only way that would occur is if a large majority of the mempool were transactions with multiple OP_RETURN outputs or an OP_RETURN output larger than 83 bytes, and that's not the case).

[gmaxwell wrote](https://github.com/bitcoin/bitcoin/pull/32406#
...

🚀 fanquake merged a pull request: "qt, wallet: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/32238)

💬 fanquake commented on issue "test: failure in `mining_basic.py` AssertionError: not(4.656542373906924E-10 == 4.656542373906925E-10)":
(https://github.com/bitcoin/bitcoin/issues/32515#issuecomment-2885964963)
Yea. Have only seen it under Valgrind.