💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1821389449)
comment above this isn't for `Apply`.

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1821407375)
c8b896067d42001f2a47cd837c0aed07bb39f329 commit message could probably use a little massaging since removals can happen in a changeset? I'm assuming these are all block/trimming related

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1821342151)
`GetAddedTxn` ?

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1821435939)
text is better thanks

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1821436409)
still curious about this if you figure out how it was hit

💬 jonatack commented on issue "`Wunused-member-function` in test each commit":
(https://github.com/bitcoin/bitcoin/issues/31180#issuecomment-2445195488)
As a reviewer, my preference is for the member or method to be used in the commit where it is added. This helps to more easily understand why it was added, and aids in reviewing the change without needing to flip repeatedly between the commits or git rebasing to squash them together.

🤔 ryanofsky reviewed a pull request: "refactor: Clean up messy strformat and bilingual_str usages"
(https://github.com/bitcoin/bitcoin/pull/31072#pullrequestreview-2402821164)
Rebased 022ffd149a64f944d1ade75664e8dc19d04bcda7 -> 46c55ab427c0771cacaed7a20612ffe2cc3543db ([`pr/bfmt.2`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.2) -> [`pr/bfmt.3`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bfmt.2-rebase..pr/bfmt.3)) due to conflict with #31042, also addressing review comments.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1821374807)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801085975

Thanks, updated the commit message to avoid being misleading, and can update it again if one of the other PRs gets merged first.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1821382046)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801093580

Makes sense. Just dropped this commit even though it is a refactoring, because I don't want to make a different change that would change behavior.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1821384626)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801097275

Thanks added a note to clarify. The point of this change is to prevent a future change in a different part of the call graph from causing a bug here, but there is no bug currently.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1821411765)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801118756

Thanks, we also talked about this offline and I agree that strprintf can often be nicer than concatenation for building strings, but I think this approach doesn't work very well for untranslated bilingual strings. For example in the HTML case, you would probably only want to format the translated strings as HTML, not both translated and original strings. I think the downsides of supporting untranslated format strings out
...

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2445259353)
rebased on master due to conflict

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1821533301)
I've added it to the initial commit and reworked it on the last one

💬 m3dwards commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2445432688)
I have been able to recreate a roughly 2x slowdown on both x86 Linux and AArch64 Mac. Not quite as much as the corcheck.dev slowdown but still significant. Just for reference, corecheck.dev runs on AArch64 Linux.

I compared f0130ab1a1 (October 17th) with 97b790e844abd2 (October 19th).

I'll have a scan for the offending commit and worse case do a binary search.

💬 davidgumberg commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2445523055)
Reproduced the same crash in Windows 10 22H2 (Build 19045.5011)

It appears that windows subsystem versions above 6,2 require some [buffer canary features](https://learn.microsoft.com/en-us/cpp/build/reference/gs-buffer-security-check) that we can't have with MinGW ([similar downstream issue in kde's QBS](https://bugreports-test.qt.io/browse/QBS-1724) and [upstream bug report in mingw sourceforge](https://sourceforge.net/p/mingw-w64/bugs/968/)) it seems vestigial support in MinGW for the `/GS
...

💬 andrewtoth commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2445588756)
I believe you can get more fine grained benchmarks for this using the rpc_blockchain benchmarks.

💬 maflcko commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2445987996)
In theory you could also add UniValue to `VectorLikeClasses` ( https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html#cmdoption-arg-VectorLikeClasses), but this is probably best done in a follow-up.

💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446003968)
[Here](https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2445523055) is another reason to switch to UCRT.

📝 hebasto opened a pull request: "cmake: Revamp `FindLibevent` module"
(https://github.com/bitcoin/bitcoin/pull/31181)
This PR generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former.

Addresses https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2444700876:
> We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely.

Similar to https://github.com/bitcoin/bitcoin/pull/30903.

Guix build:

...

💬 davidgumberg commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446020491)
Maybe should do:

```diff
CExtKey DecodeExtKey(const std::string& str)
{
CExtKey key;
- std::vector<unsigned char> data;

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2446055786)
> I think we can take this further and reserve for `VOBJ` types instead of just `VARR`. The `VOBJ` type uses both values and keys, so we can reserve for both.

Done in the latest push, see [diff](https://github.com/bitcoin/bitcoin/compare/fa73b9a9984d5dd2efa6fd5dd70b3db617803a25..f8d5d2cfc63bea5b58b08ceaa743c26820c1763d), but I am reserving keys only when `typ` is of `VOBJ`.

> We can count how many times we use `pushKV` and reserve that amount for both keys and values.

+1, this approach
...