💬 ryanofsky commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1775897574)
Not important, for but both of the InitAndLoadChainstate calls in this PR, I'm surprised someone would prefer newline separated arguments over space separated:
```c++
InitAndLoadChainstate(node, do_reindex, do_reindex_chainstate, cache_sizes, args);
```
clang-format has a nice "BinPack" and "Align" options for packing arguments neatly without having to take up large amounts of space
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1775897574)
Not important, for but both of the InitAndLoadChainstate calls in this PR, I'm surprised someone would prefer newline separated arguments over space separated:
```c++
InitAndLoadChainstate(node, do_reindex, do_reindex_chainstate, cache_sizes, args);
```
clang-format has a nice "BinPack" and "Align" options for packing arguments neatly without having to take up large amounts of space
💬 TheCharlatan commented on pull request "build: Add missing USDT header dependency to kernel":
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2375104990)
Concept ACK
Good find, I think just including it like you've done here is ok for now.
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2375104990)
Concept ACK
Good find, I think just including it like you've done here is ok for now.
💬 TheCharlatan commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1775916783)
Mmh, I think this must have been something I did out of old habit. It would be good to have these stricter clang format options documented in the dev notes so we can at least all try to write similarly formatted new code.
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1775916783)
Mmh, I think this must have been something I did out of old habit. It would be good to have these stricter clang format options documented in the dev notes so we can at least all try to write similarly formatted new code.
💬 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?
(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
(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.
(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.
(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
(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?
(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
(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.
(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)
(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));
```
(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)?
(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
...
(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
(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
...
(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
(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
(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.
(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.