💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263732536)
nit: Can remove the empty lines in this file, and other files?

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635866926)
Looks like you forgot to include the doc change? https://github.com/bitcoin/bitcoin/commit/abacaef3f3b541ead566532eaae9f5db31ccb7ff#diff-5e319039d67254bed6ca82562ec5f560f102004aa4806e4ca77f5d0065c65fbb

Also, could update title, description and commit message?

💬 jonatack commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263749946)
A potential downside with this change is the `util/check.h` code is added to a fair number of compilation units (unless the header was already missing). Also, it looks like some cases haven't been updated. Maybe only use it for new code?

🤔 jonatack reviewed a pull request: "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions"
(https://github.com/bitcoin/bitcoin/pull/26504#pullrequestreview-1530329568)
If this is done, IIUC the developer notes should be updated.

💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1635896789)
lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂

<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: lgtm ACK 1cd45d4e08c3dfd1
...

📝 MarcoFalke opened a pull request: "util: Replace filesystem include with util/fs.h include "
(https://github.com/bitcoin/bitcoin/pull/28076)
Using `std::filesystem` is problematic:

* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.

Fix all issues by removing use of it and adding a linter to avoid using it again in the future.

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263770200)
Done.

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635907218)
@MarcoFalke
> Also, could update title, description and commit message?

The title an the description have been updated. What update for commit messages would you like to see?

💬 jonatack commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263776033)
```suggestion
*Rationale*: The comment documents skipping the `default:` case and it complies with `clang-format` rules. The `UNREACHABLE` macro prevents firing of `-Wreturn-type`/`C4715` warnings on some compilers. In the RPC code, the `NONFATAL_UNREACHABLE` macro might be more appropriate instead.
```

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263779434)
Thanks! Updated.

👍 brunoerg approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1530384275)
reACK e010fddf15a6d4ff84bfebdcb33c9c1d9cea7f0a

💬 ItIsOHM commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1263790063)
Actually I wasn't sure if I should since I had removed the WIP tag and asked for a review, but I can change them right now if that's okay.

💬 sipa commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1263804601)
WIP just means "this is not ready for review yet".

You're always expected to address review comments, whether that means making changes, or giving a justification why it's better not to.

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1635954359)
Just ran both CI configs locally on aarch64 and they passed.

💬 hebasto commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1635956894)
Will it be the first Rust code in our codebase?

💬 fanquake commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635957683)
> Apparently, this macro prevents compiler warnings

Either it does or it doesn't? If it doesn't, then any workarounds should be removed, and warnings re-enabled.

If it doesn't, there's no point to this refactoring at all, if it doesn't even acheive the stated goals.

> I find it frustrating to play compiler golf against g++ in the CI on every code push that uses a switch-case on an enum class.

Which CI is this? The only one where it is an issue has the warnings disabled?

🤔 furszy reviewed a pull request: "wallet: don't duplicate change output if already exist"
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1530430019)
Updated. Added required changes to make coin selection, change amount and fee calculation processes take into consideration the presence of an existing change output. Improving the accuracy of the results and avoiding cases where we end up having to increase the change output at the end of the process by the fee surplus.

Changes:
* Cleaned two change related members from the `CoinSelectedParams` struct that are not used inside the coin selection process. They were only intermediate results
...

:lock: fanquake locked an issue: "bitcoind dumps core when deriveaddresses is called with index 2147483647 (2^31-1)"
(https://github.com/bitcoin/bitcoin/issues/26274)

💬 kristapsk commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1635981682)
Why to introduce Rust as additional dependency just for the linter code?

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1636003349)
> Why to introduce Rust as additional dependency just for the linter code?

Because I don't think it is a good use of developer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a `TypeError`. (Just one example: https://github.com/bitcoin/bitcoin/pull/27921#issuecomment-1601003548, obviously the problem is a general one and it is left as an exercise to the reader to create a full list of past issues, including the ones that were more severe.)
...