💬 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"});
```

💬 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
...

💬 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?

💬 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

💬 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.

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

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776227563)
I've moved it to its own commit. I left the name `iterations_left` as I think calling it "optimizations" is misleading; most steps don't optimize anything. It's true that the iterations are not actually the loop's iterations anymore, but they're still very correlated to the number of iterations the algorithm makes.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776231855)
I was hoping to do a bit better than comparing with `AncestorCandidateFinder` (which only tries ancestor sets, not unions of two of them), but it's really not worth the complexity here. I've changed it to using `AncestorCandidateFinder` (here and below), and in a separate commit added an arbitrary subset (from fuzz input) to compare with.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776232273)
You're right that it shouldn't be repeated, but I don't think it's worth much effort to keep this. I've dropped it.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776235234)
They cannot be guaranteed to be identical; there may be multiple distinct optimal candidate sets.

👋 BrandonOdiwuor's pull request is ready for review: "Wallet: "listreceivedby*" fix"
(https://github.com/bitcoin/bitcoin/pull/30972)

💬 maflcko commented on pull request "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job":
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2376048428)
> Otherwise:

Can you explain this a bit better? The CI task is passing, see https://cirrus-ci.com/task/6297362342084608

So I presume this is just for the case when an error happens, because otherwise the symbolizer wouldn't be asked?

💬 fanquake commented on pull request "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job":
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2376068115)
> So I presume this is just for the case when an error happens, because otherwise the symbolizer wouldn't be asked?

Yea. Or fuzzing with no corpus.

💬 maflcko commented on pull request "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job":
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2376077438)
review ACK c1832584bfd1b352095bc41a13ff17564e456d43