💬 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).
💬 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.
(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.
(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.
(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.
(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)
(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?
(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.
(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
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2376077438)
review ACK c1832584bfd1b352095bc41a13ff17564e456d43
💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2376088471)
> ⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
>
> * Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
> * Is it no longer relevant? ➡️ Please close.
> * Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
Will close if I don't get to it by this weekend, sorry for my del
...
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2376088471)
> ⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
>
> * Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
> * Is it no longer relevant? ➡️ Please close.
> * Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
Will close if I don't get to it by this weekend, sorry for my del
...
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2376088936)
@ryanofsky looks like I copy-pasted the wrong hash. On my machine I have 1a332817665f77f55090fa166930fec0e5500727 (before fetching today).
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2376088936)
@ryanofsky looks like I copy-pasted the wrong hash. On my machine I have 1a332817665f77f55090fa166930fec0e5500727 (before fetching today).
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2376091479)
I'm not sure, I'll do another post-merge look.
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2376091479)
I'm not sure, I'll do another post-merge look.
💬 vasild commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1776500085)
Without a socket (second argument to `CNode` constructor is `nullptr`) this will not test much of the handshake:
https://github.com/bitcoin/bitcoin/blob/65f6e7078b17e6e73d74dfeb00159878099fee1e/src/net.cpp#L1600-L1605
Any special reason to go socket-less? I am working on a `CConnman` refactor that is changing this anyway, should I assign a fuzzed socket to the node?
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1776500085)
Without a socket (second argument to `CNode` constructor is `nullptr`) this will not test much of the handshake:
https://github.com/bitcoin/bitcoin/blob/65f6e7078b17e6e73d74dfeb00159878099fee1e/src/net.cpp#L1600-L1605
Any special reason to go socket-less? I am working on a `CConnman` refactor that is changing this anyway, should I assign a fuzzed socket to the node?