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

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680154549)
The function is just for testing, so it doesn't matter much, but sure, added.

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

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680155259)
Fair enough, but style-wise I still like to have a comment on each function. I've expanded the comment a bit here to point out it's for testing.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680157731)
It's not clear to me what you're asking for. Are you asking what I changed to address this, or saying that it's still an issue, or saying that more explanatory comments are needed?

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

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2231992542)
> Ok, I am done, but I think it is easier to review this pull request.

OK - I'm working on addressing the feedback, plus adding more test coverage for the different types of errors. Will update soon!

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