💬 kevkevinpal commented on pull request "test, refactor: Fix MSVC warning C4101 "unreferenced local variable"":
(https://github.com/bitcoin/bitcoin/pull/30464#issuecomment-2232048259)
utACK [44f0878](https://github.com/bitcoin/bitcoin/pull/30464/commits/44f08786f435ed4284d39dc604c2a5fcbde9e602)

lgtm! seems pretty straight forward

🤔 tdb3 reviewed a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457#pullrequestreview-2181557526)
Approach ACK
Thanks for increasing clarity in RPC.
Looks like there's a missing `assert` in `test_getaddressinfo()`.
Left a nit.

💬 tdb3 commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680220509)
Maybe it's worth adding a line for segwit v0 as well? (e.g. witness program not 20 or 32 bytes long)?

also

nit: might increase clarity
```diff
- BECH32_VALID_UNKNOWN_WITNESS = 'bcrt1p424qxxyd0r'
+ BECH32_VALID_UNKNOWN_WITNESS_PROG = 'bcrt1p424qxxyd0r'
```

👍 tdb3 approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2181599489)
Code Review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339
Good addition and the updates tidy things up nicely.

I would also support a bitcoind config parameter for this.

🤔 kevkevinpal reviewed a pull request: "fuzz: add target for `CoinsResult`"
(https://github.com/bitcoin/bitcoin/pull/30461#pullrequestreview-2181679638)
Concept ACK [385ec3f](https://github.com/bitcoin/bitcoin/pull/30461/commits/385ec3f12b6cacfa92aa2bc9ffb0177f3120adca)

I see value in adding fuzz coverage to `CoinsResult` `All`, `Erase`, and `Clear`

💬 kevkevinpal commented on pull request "fuzz: add target for `CoinsResult`":
(https://github.com/bitcoin/bitcoin/pull/30461#discussion_r1680278518)
does it make sense to add another `CallOneOf` with just `coin_result.Clear()` because aren't we forcing the pattern of `Clear()` -> `All()` -> `Size()` and never `Clear()` -> `Size()`

not sure if that is needed, just a thought

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680326042)
If `accumulator` is equal in feerate to `subset`, but smaller in size, it would be correct to either return `accumulator` (as done in the current code) or to continue the loop. Returning `best` in this case would not be correct, as it's possible that a bigger intersection still has a higher feerate.

In general, I prefer returning the first (smallest) acceptable intersection because that is the least amount of iteration work, and it does not hurt quality (if a bigger intersection of higher fee
...

💬 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