💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680336040)
No, we need to invoke `MarkDone` even when the `iterations_done_now == max_iterations_now` condition is not satisfied.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345658)
Does looking at the documented serialization in the unit tests help? If not, I will add a worked-out example in more detail to the `DepGraphFormatter` documentation.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345744)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345825)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345874)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345931)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345987)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680346568)
Done, and added more consistency checks on top (chunks must be non-empty, match the highest-feerate remaining prefix, and be topological).

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680347774)
Good question.

Clusters, as intended to be used by the mempool/txgraph code, are always connected (if not, they'd be separate clusters), but nothing actually relies on this property, so all cluster_linearization tests should pass even with non-connecting clusters. I believe that earlier it was helpful to forcefully make all clusters for most fuzz tests connected, as this helped the fuzzer find extreme cases, though I have not tested this recently.

This is also the reason why this commit is
...

💬 maflcko commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680403465)
thanks, fixed

💬 maflcko commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680403780)
Left the nit for now.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680414541)
not sure about overloading this. Otherwise, one may get a runtime evaluation when switching from a raw string literal pointer to a consteval string_view, no?

Seems clearer to make it a constructor taking a string_view? I guess the only confusion could be that the `Span<const unsigned char>` and string_view (aka `Span<const char>`) do different things (one is hex decoding and the other is not), but that should be fine, because both are distinct types.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680415419)
Same

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680415361)
What is the point of this? Seems odd to add space before compilation and then remove it during compilation.

Seems easier to not add the space in the first place and fail compilation when there is one?

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680419847)
Not sure about hardcoding this for every base_blob.

Seems easier to just implement it once for `base_blob` and then have it available for all?

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680422462)
Not sure about fuzzy decoding here. Accidentally truncating an `uint256` and thus parsing it as something else seems dangerous.

The convenience of being able to truncate leading zeros of a hex-encoded base_blob are never used, are they? The two constants ZERO and ONE are constructed without hex-decoding, so this isn't needed there, and I fail to see another use case right now.

It seems easier to just assert WIDTH.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680425993)
Same here. Overloading `ParseHex` seems fragile, because a compile time string_view will be evaluate at runtime, while a string literal will be evaluated at compile time.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680427277)
Please add `{`, `}` for multiline-if, according to the dev notes.

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680444983)
See #16545

💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2232487097)
Are you still working on this, or do you want someone to pick it up?