💬 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.
💬 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
...
(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, "");
```
(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"});
```
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776028495)
nit:
```suggestion
BOOST_CHECK_EQUAL(ParseOptions({"-assumevalid=0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"}).assumevalid, uint256{"0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"});
```
💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776017230)
we could divide this method into typed and untyped parsing, would simplify the logic slightly:
```C++
std::optional<common::SettingsValue> HandleTypedArg(const KeyInfo& key, std::optional<std::string_view> value,
unsigned int flags, std::string& error)
{
if (key.negated) {
// If argument is typed, only allow negation with no value or with
// literal "1" value. Avoid calling InterpretBool and accepting
// othe
...
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776017230)
we could divide this method into typed and untyped parsing, would simplify the logic slightly:
```C++
std::optional<common::SettingsValue> HandleTypedArg(const KeyInfo& key, std::optional<std::string_view> value,
unsigned int flags, std::string& error)
{
if (key.negated) {
// If argument is typed, only allow negation with no value or with
// literal "1" value. Avoid calling InterpretBool and accepting
// othe
...
💬 instagibbs commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2375349934)
@sdaftuar to restate a bit more simply: Things are broken whether or not we remove this, but removing this rule would likely make incentive incompatible RBFs more common in the average or otherwise benign case?
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2375349934)
@sdaftuar to restate a bit more simply: Things are broken whether or not we remove this, but removing this rule would likely make incentive incompatible RBFs more common in the average or otherwise benign case?
💬 kevkevinpal commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2375355917)
Concept ACK the for loop is confusing and refactoring it to something better is a welcomed change
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2375355917)
Concept ACK the for loop is confusing and refactoring it to something better is a welcomed change
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1775532749)
nit (not really related, but I noticed it asking myself why `success=False` here): The doc "if success is False: assert that the node's tip doesn't advance" in `send_blocks_and_test` is wrong. It it is only checked that the node's tip doesn't advance to the last block provided, however it can advance to another block, as it happens here.
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1775532749)
nit (not really related, but I noticed it asking myself why `success=False` here): The doc "if success is False: assert that the node's tip doesn't advance" in `send_blocks_and_test` is wrong. It it is only checked that the node's tip doesn't advance to the last block provided, however it can advance to another block, as it happens here.
🤔 mzumsande reviewed a pull request: "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)"
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-2328776410)
Code Review ACK c6ca2a17ea4d76f21d7702f70ba892a9494576bf
I agree that it is more consistent to keep the same tip over a restart in case of multiple candidates (although I don't think this is much of a problem in practice).
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-2328776410)
Code Review ACK c6ca2a17ea4d76f21d7702f70ba892a9494576bf
I agree that it is more consistent to keep the same tip over a restart in case of multiple candidates (although I don't think this is much of a problem in practice).