💬 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.
(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.
(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
...
(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
(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
(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.
(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
...
(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.
(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.
(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.
(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:
...
(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;
(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
...
(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.
(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
...
(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`?
(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?
(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.
(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
...
(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
```
(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
```