💬 mzumsande commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2375136490)
That's strange. I just tried to reproduce syncing this snapshot with current master (39219fe145e5e6e6f079b591e3f4b5fea8e71804) and didn't experience this (although I do get get similar "Cache size" log messages).

Are you syncing from random peers or do you have some local setup?

👍 ryanofsky approved a pull request: "test: add test for specifying custom pidfile via `-pid`"
(https://github.com/bitcoin/bitcoin/pull/30724#pullrequestreview-2329424579)
Code review ACK 04e4d52420a0e6bf40d4bd6fe1f31f66db9eab0a

🤔 jarolrod reviewed a pull request: "doc: Add `nproc` support for Mac through `coreutils`"
(https://github.com/bitcoin/bitcoin/pull/30936#pullrequestreview-2329456514)
Don't agree with having this in the build doc, no need to have a user install such a package that is not needed to build bitcoin. Docs should only have users install required dependencies. If it really helps, productivity notes could point a user as to how they can have `nproc` support.

A mac user can also just `alias nproc="sysctl -n hw.logicalcpu"`, no need for us to be opinionated on installing a dependency for this in this doc.

💬 theuni commented on pull request "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job":
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2375175185)
Makes sense to me. I assume this could also be solved with a symlink or update-alternatives, but even if so, using the env var seems simpler.

💬 l0rinc commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2375187987)
> A mac user can also just alias nproc="sysctl -n hw.logicalcpu"

Absolutely, but let's document these somewhere, since we're using `-j$(nproc)` in a few other places in the code, without mentioning that Mac users will need to do a few extra steps

💬 pablomartin4btc commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2375191697)
@fanquake what `getchaintips` returns?

💬 achow101 commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2375198412)
ACK 1a332817665f77f55090fa166930fec0e5500727

💬 jarolrod commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2375206192)
@l0rinc sure, but this nproc thing on macOS isn't necessarily something we have to document, its not like its new or unique to our project in a way, and docs are written in a way where we expect the user to know a bit about their OS already. I would assume most people who build bitcoin on macOS are already used to the in-and-outs of building on macOS.

In any case, i agree it can be in productivity if it proves to be helpful to builders.

🚀 achow101 merged a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510)

💬 theStack commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1776022125)
could add a lock assertion here (since the method has the `EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);` annotation)
```suggestion
void CWallet::SetupOwnDescriptorScriptPubKeyMans()
{
AssertLockHeld(cs_wallet);

assert(!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER));
```

💬 theStack commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1776024276)
in c436b516: shouldn't those calls also happen as a batch transaction, i.e. wrapped around a TxnBegin/TxnCommit pair (at least in the context of this commit)?

🤔 l0rinc reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2329375556)
Approach ACK

I did a first scan over the impl, left a few nits, some refactoring ideas, let me know what you think and I'll continue the reviews based on that.

<details>
<summary>Suggestions</summary>

```patch
Subject: [PATCH] TODO
---
Index: src/test/util/setup_common.h
<+>UTF-8
===================================================================
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
--- a/src/test/util/setup_common.h (revision 3e39cf7b48a4166a
...

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897208)
seems to me we're treating the input `value` as an implicit optional (via pointers), even though originally it's an actual `std::optional<std::string>`.
Given that we're already returning an `std::optional` already, can we consider avoiding pointers and using a `std::optional<std::string_view> value` parameter instead?

It would also allow us writing: `if (value != "1") {` instead of

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489)
we could group the two bools and use an optional for the in parsing (assuming `std::optional` `value` parameter):
```suggestion
if (flags & ArgsManager::ALLOW_INT) {
if (auto parsed = TryParseInt64(*value)) return *parsed;
}
if (flags & ArgsManager::ALLOW_BOOL) {
if (value == "0") return false;
if (value == "1") return true;
}
```

<details>
<summary>TryParseInt64</summary>

```C++
std::optional<int64_t> TryParseI
...

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775905567)
nit: this is only used once, maybe we could have a local `TryParseInt64` next to `InterpretBool` - details below

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897382)
nit: alignment

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776007657)
I find it quite confusing that have a return value, and error return value and even warning as possible outcomes.

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776023938)
* the last part seems to check if we're typed
* what if we introduced flag helpers to make this more readable and easier to transition later:
```C++
//! Whether the type of the argument has been specified and extra validation rules should apply.
inline bool DisallowNegation(uint32_t flags) { return flags & ArgsManager::DISALLOW_NEGATION; }
inline bool DisallowElision(uint32_t flags) { return flags & ArgsManager::DISALLOW_ELISION; }

inline bool IsAnyArg(uint32_t flags) { return flags & Ar
...

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776026761)
nit:
```suggestion
std::string error;
BOOST_REQUIRE_MESSAGE(args.ParseParameters(argv.size(), argv.data(), error), error);
BOOST_CHECK_EQUAL(error, "");
```

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776028495)
nit:
```suggestion
BOOST_CHECK_EQUAL(ParseOptions({"-assumevalid=0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"}).assumevalid, uint256{"0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"});
```