💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610320129)
when can `GetInputWeight` return `nullptr`? can we exercise that path as well?

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610314187)
this is just duplicating the actual implementation, not very useful

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610343832)
`HasSelectedOrder()` and `GetSelectionPos()` from header are still untested

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610327468)
Given the `std::optional`'s `operator==` overload, wouldn't this suffice
```suggestion
assert(scripts.first == script);
```
?

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610349656)
doesn't this overwrite the value change by `coin_control.Select`?

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610343108)
`Select` mutates `m_selection_pos` - we could do selection separately

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610346634)
you could leave these to untangle coverage from context

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610297526)
Please resolve the comment

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610306247)
`SetScriptWitness` is already void:

```suggestion
coin_control.Select(out_point).SetScriptWitness(script_wit);
```

💬 l0rinc commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2610361286)
Deleted the manual removal, thanks!

💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2610422861)
I don't understand this either. There is no obfuscation, so the two are the same either way?

I'd prefer to just keep the `<<` in high level code, unless there is a reason not to.

💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#issuecomment-3641734759)
it doesn't help that you rebase on a Knots base - the first few commits are already merged on Core, please fix that.

🤔 maflcko reviewed a pull request: "bench: run `FindByte` across block-sized buffer"
(https://github.com/bitcoin/bitcoin/pull/34046#pullrequestreview-3567263151)
lgtm 464ee9e26f7851ff27c2d18f807ae64c8dd0fc62, but I'd prefer to not use the low-level buffer writing questionable "optimization"

💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2610442547)
you could inline hit to the if condition

💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2610445121)
why extract `end` if we're getting rid of it in the next commit?

💬 l0rinc commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2610448127)
the vector serializes differently, we don't need the size prefix, it would make the assertion at the end a bit less intuitive

💬 Raimo33 commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2610450968)
messier imo

💬 Raimo33 commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2610453046)
I've thought about it as well. but it makes it easier to review. smaller diff

💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2610458005)
how so, how is it easier to review multiple conflicting changes? Same for the `it_start` change - it's completely removed in a next commit.

💬 l0rinc commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#discussion_r2610463571)
Yes, it's an unused argument - it's clearer to remove the fake single-instance-abstraction

💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2610467129)
To use span-serialization, you can just type `std::span{bla}`, see also `git grep '<< std::span{'`.

I don't think it makes sense to use write_buffer for places where span-serialization is wanted. Otherwise, this leads to inconsistent code, such as:

```cpp
file << int_val;
file.write_buffer(vec_val);
file << int_other_val;
```

Also, this mutating the buffer (possibly) makes it doubly confusing.