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

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1680462164)
`3` is the peer timeout, so you'll have to change it to bump `2` here. Otherwise, the test will fail:


https://cirrus-ci.com/task/5359619151757312?logs=ci#L3935


```
test 2024-07-17T05:25:54.632000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:15680
node0 2024-07-17T05:25:54.634738Z (mocktime: 2024-07-17T05:25:54Z) [net] [net.cpp:3764] [CNode] [net] Added connection peer=0
node0 2024-07-17T05:25:54.634776Z (mocktime: 2024-07-17T05:25:54Z) [net] [net.cpp:1814] [Cre
...

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1680464165)
Also, you'll have to move it up by one line. Otherwise it is racy, because sometimes it will be bumped when the message was sent, and sometimes before the raw message was sent.

💬 meglio commented on issue "gettransaction details does not include send to myself balance changes for imported addresses":
(https://github.com/bitcoin/bitcoin/issues/28555#issuecomment-2232511784)
In my case, `gettransaction()` returns zero for the top-level "amount" key sometimes:

```json
{
"result": {
"amount": 0.00000000,
...
```

However, the "details" have non-zero amounts in them.

Can it be related?

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680480182)
Thanks, but actually it doesn't work when running on Windows. Added a new commit with a workaround.

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680480364)
Thanks, taken!

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680480517)
Thanks, taken!