💬 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
```
💬 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.
(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)
(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`?
(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`?
💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1822306060)
Maybe i'm missing something but the intent of `g_mapport_current` is hazy to me. It's turned on in `ThreadMapPort` while `ProcessPCP` is running, then turned off temporarily when `ProcessPCP` returns false.
- If we want to know if the mapport thread is running, `g_mapport_thread.joinable()` seems to suffice.
- If we want to know if mapport thread is supposed to be running, or stop it, there's `g_mapport_enabled`.
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1822306060)
Maybe i'm missing something but the intent of `g_mapport_current` is hazy to me. It's turned on in `ThreadMapPort` while `ProcessPCP` is running, then turned off temporarily when `ProcessPCP` returns false.
- If we want to know if the mapport thread is running, `g_mapport_thread.joinable()` seems to suffice.
- If we want to know if mapport thread is supposed to be running, or stop it, there's `g_mapport_enabled`.
💬 fanquake commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2446497505)
I think you could just remove the doc block being added here, and then this can go ahead (also rebase). Doesn't seem like it needs to be blocked on #30861, if that is why it was drafted.
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2446497505)
I think you could just remove the doc block being added here, and then this can go ahead (also rebase). Doesn't seem like it needs to be blocked on #30861, if that is why it was drafted.
🤔 hodlinator reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2404320459)
Concept ACK e53829d3952c6ed275507a66e77636aad67d106b
Cleanest attempt at increased compile time validation of format so far. When reviewing #31149 I had the gnawing feeling that more complete format string support would have reduced the diff, but pushed it away for expediency (an earlier attempt at more complete support was attempted in #30999).
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2404320459)
Concept ACK e53829d3952c6ed275507a66e77636aad67d106b
Cleanest attempt at increased compile time validation of format so far. When reviewing #31149 I had the gnawing feeling that more complete format string support would have reduced the diff, but pushed it away for expediency (an earlier attempt at more complete support was attempted in #30999).
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822308585)
```suggestion
}
```
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822308585)
```suggestion
}
```