🤔 jonatack reviewed a pull request: "util: Don't derive secure_allocator from std::allocator"
(https://github.com/bitcoin/bitcoin/pull/27930#pullrequestreview-1541647065)
Initial ACK b79d9563169cc990ff2da9a44fef67205e907a8b review and debug built/tested each commit locally

Note that per https://en.cppreference.com/w/cpp/memory/allocator and related pages, many of the member types and functions are changing with C++20. These can be revisited for updates when this codebase migrates to C++20 or later.

🤔 mzumsande reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1541662993)
Maybe getting disconnected could be beneficial in this rare situation? If our tip is that much behind but we're not in IBD, we can't help them and they can't help us, so our priority should be to get the missing blocks asap, and getting disconnected by useless peers seems like it's not such a bad thing towards that goal.

What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better pe
...

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1270993538)
f1b46f4017a
```
init.cpp:948:12: error: 'operator=' is a private member of 'util::Result<void>'
result = init::SetLoggingLevel(args);
~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
./util/result.h:47:13: note: declared private here
Result& operator=(Result&&) = default;
^
```

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271000370)
40f09de73e6 `lint-locale-dependence.py`

```
The locale dependent function std::to_string(...) appears to be used:
src/test/result_tests.cpp:101: return {std::move(result), std::to_string(*result)};
```

💬 manfreddd commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646182300)
Thanks for the suggestion. I reran Bitcoin Core with dbcache=2048, but it crashed the same way after about 2.5 hours. Enclosing the logfile from this latest run.
[bitcoinlog.txt](https://github.com/bitcoin/bitcoin/files/12134326/bitcoinlog.txt)

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271034517)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1270993538

Thanks for testing the early commit, should be fixed now

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1541767699)
Updated 40f09de73e61e7ae62d6639a49b7c7ac48d514d9 -> 775b54e88107b0b976bf995e607926013fa9bc42 ([`pr/bresult2.34`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.34) -> [`pr/bresult2.35`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.35), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.34..pr/bresult2.35)) with compile/lint fixes

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271033872)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271000370

Thanks, this should be fixed now

💬 psgreco commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1646183855)
> @psgreco See above; it turned out that what I intended to do here wasn't actually what was implemented (it was instead unconditionally preferring send over receive). Would you mind trying again if this fixes the issue for you?

It seems to fix the issue for me still with the new changes, but the refactor that I had to do to run in elements 22 (like bitcoin 22), doesn't let me make a hard confirmation.

💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1646212942)
The `ParseHexUV` function is used in `bitcoin-tx.cpp` only. Why not make it `static`/`namespace`d in there?

See: https://github.com/hebasto/bitcoin/commit/6d0fcfe3abc8b944d296d2033b9fd2e444d85ce4

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1646218170)
> The ParseHexUV function is used in bitcoin-tx.cpp only. Why not make it static/namespaced in there?

Sacrificing the fuzz test seems unfortunate. but we already don't fuzz a similar function in `bitcoin-tx` `AmountFromValue`. I guess we have poor coverage of `bitcoin-tx` in general though.

🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1541838521)
> Maybe getting disconnected could be beneficial in this rare situation? If our tip is that much behind but we're not in IBD, we can't help them and they can't help us, so our priority should be to get the missing blocks asap, and getting disconnected by useless peers seems like it's not such a bad thing towards that goal.

Agree but, while the outcome might be the desired one, the approach to achieve it isn't the best. It seems unwise to have the node sending a message through the wire, knowi
...

💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1646246065)
> Sacrificing the fuzz test seems unfortunate.

Maybe more fuzzing for `ParseHex` and `IsHex` is better way?

🤔 jonatack reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1541828147)
First-pass ACK 775b54e88107b0b976bf995e607926013fa9bc42

In https://github.com/bitcoin/bitcoin/commit/332e847c9ec0241efd9681eee3b03ff819aaddc3 and 4d995d3fa66fbc3eb87c6627e5ba1b2a809402a4, I wonder if some of the custom operator (i.e. move) definitions should have a noexcept-specification. Also, notating any methods where it would be incorrect if the return value isn't checked (e.g. for error-handling) and optionally getter-like pure functions with `nodiscard` may aid reviewers / readers of th
...

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271094067)
332e847c9ec tuple not needed per iwyu in tidy ci https://cirrus-ci.com/task/6540065057275904?logs=ci#L20325, while touching could add the others

```diff
+#include <assert.h>
#include <memory>
#include <optional>
-#include <tuple>
#include <utility>
#include <vector>
+#include <new>
+#include <type_traits>
```

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271096350)
https://github.com/bitcoin/bitcoin/commit/332e847c9ec0241efd9681eee3b03ff819aaddc3 per iwyu in https://cirrus-ci.com/task/6540065057275904?logs=ci#L20305

```diff
#include <util/result.h>
-#include <util/string.h>
+#include "util/translation.h"
+
+#include <algorithm>
+#include <initializer_list>
+#include <iterator>
```

💬 besoeasy commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646265590)
@manfreddd can you share your CPU Usage screenshot ?

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1646270585)
Added a missing check in the fuzz target since `GetExecStackSize()` now returns an optional.

💬 manfreddd commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646284658)
Sure, enclosing the Task Manager window screenshot (in Portuguese).

💬 besoeasy commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646325159)
@manfreddd you using SSD ? your disc seems slow