💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1269813522)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174297928

I agree with all of this and want to do more work to address it.

I think by default types convertible to bool should be disallowed with `Result`. I think `Result<bool>` or `Result<optional<T>>` types are inherently ambiguous and code accessing those return values would be fragile and prone to errors from dereferencing incorrectly.

But disallowing all types convertible to bool would go to far because being able to r
...

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270901466)
It's not avoiding them, it's hiding them behind `UCharCast`? :p

It boils down to choosing between casting to `uint8_t` or to `std::byte`, and i went for the option with less casts.

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270902655)
Done, thanks. (And also in the function's doc comment.)

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270903253)
`UCharCast` uses `reinterpret_cast`, but yeah this doesn't matter for the fuzz tests.

I only left the comment, so that it would be easier if CKey::Set was changed to take `std::byte`, it would be a smaller diff in the future.

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270904211)
Happy to do it then.

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270904366)
Done in the wrong commit, though? :sweat_smile:

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270906119)
Done.

(However i just checked and `UCharCast` does indeed use C-style casts)
https://github.com/bitcoin/bitcoin/blob/d23fda05842ba4539b225bbab01b94df0060f697/src/span.h#L272

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270906387)
:facepalm:

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270907877)
And i can't even blame it on anything but myself this time! Fixed.

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1646016856)
re-ACK 131314b62e899f95d1863083d303b489b3212b16 🐓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 131314b62e899f95d18
...

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270921379)
Yeah, it gives the tag separately; the RFC doesn't actually require the tag to be sent at the end.

💬 CaseyCarter commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1646037735)
I've made consistent changes to `zero_after_free_allocator`, and also performed some "aggressive cleanup" of cruft in both allocators that's unnecessary (as far as the Standard Library is concerned) after C++11. The CI will let me know if bitcoin itself was using any of that "cruft". If I have to fix any fallout, I'll learn how to build locally and stop lazily relying on CI ;)

📝 achow101 reopened a pull request: "Enable users to configure their monospace font specifically"
(https://github.com/bitcoin-core/gui/pull/497)
This replaces the overly-verbose radio-button font setting (which only allows embedded or autodetected from system) with a simple combobox providing both existing options as well as a custom option to allow the user to select any font of their choice/style.

💬 luke-jr commented on pull request "Enable users to configure their monospace font specifically":
(https://github.com/bitcoin-core/gui/pull/497#issuecomment-1646050308)
Rebased

📝 josibake opened a pull request: "Silent Payments: Implement BIP352"
(https://github.com/bitcoin/bitcoin/pull/28122)
This PR is a sub-section of https://github.com/bitcoin/bitcoin/pull/27827 and only implements the BIP logic without the wallet code. For the full PR with wallet logic, take a look at https://github.com/bitcoin/bitcoin/pull/27827 .

This PR focuses strictly on the BIP logic and attempts to separate it from the wallet and transaction implementation details. This is accomplished by working directly with public and private keys, instead of needing a wallet backend and transactions for testing. Lab
...

🤔 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)