💬 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
...

💬 laanwj commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446055905)
Yes, using secure_allocator is a better option if possible.

💬 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-2446067820)

> I believe you can get more fine-grained benchmarks for this using the `rpc_blockchain` benchmarks.

Yes, I can, but I’ll need to add verbosity levels 1 and 2, where we can see some performance improvements.

<details>
<summary>Show diff</summary>

```diff
diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
index 7e3e2d8e48a..6d431685e66 100644
--- a/src/bench/rpc_blockchain.cpp
+++ b/src/bench/rpc_blockchain.cpp
@@ -45,7 +45,25 @@ struct TestBlo
...

💬 maflcko commented on issue "`Wunused-member-function` in test each commit":
(https://github.com/bitcoin/bitcoin/issues/31180#issuecomment-2446083130)
It may be trivial to suppress either way by using https://en.cppreference.com/w/cpp/language/attributes/maybe_unused in the commit?

It should also be trivial to remove the error completely from the CI task by specifying `Wno-error=unused-member-function`?

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2446116915)
> > It would be good if ctest/Windows actually printed the exception and error message instead of silently hiding it.
>
> I'm not aware of the specific case discussed above, but exceptions in tests are generally printed as follows (with a patched code):

Maybe the log is truncated in CI and only the head is printed, as opposed to the tail, which would be more useful?

💬 hebasto commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2446215480)
> 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.

Done in https://github.com/bitcoin/bitcoin/pull/31181.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1822182678)
This could be `ReattemptInitialBroadcast`, `BroadcastTransaction` (rpc/wallet), `ProcessValidTx`, or earlier upon handling `NetMsgType::TX`.

I think it's odd we have two calls to `RelayTransaction` in the two latter cases. I'd rather change this code, e.g. pass `already_relayed` (duplicating force) to `RelayTransaction`. Perhaps too much for this PR and we should do it elsewhere (especially now that the package stuff happens in-between). I understand why it was ok when `RelayTransaction` was
...

💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138)
FWIW, here is a way to cross-build UCRT-linked Windows binaries on Fedora 41:
```
$ sudo dnf install ucrt64-gcc-c++
$ make -C depends HOST=x86_64-w64-mingw32ucrt mingw32_CC=/usr/bin/x86_64-w64-mingw32ucrt-gcc mingw32_CXX=/usr/bin/x86_64-w64-mingw32ucrt-g++
$ cmake -B build --toolchain depends/x86_64-w64-mingw32ucrt/toolchain.cmake
$ cmake --build build
```

💬 hebasto commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2446416679)
> ... and suggest linking against UCRT instead.

I've built this branch on Fedora 41, linking against UCRT (see [this](https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138) workflow), but the issue persists.

📝 maflcko opened a pull request: "ci: Place datadirs for tests under tmpfs "
(https://github.com/bitcoin/bitcoin/pull/31182)

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1822298782)
DispatchMapPort is local and only called in one place; might as well roll it into `StartMapPort`?