👍 hodlinator approved a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3566738644)
re-ACK fa874c9f050500dc521ef8d9c81b043687f0dcc7

Thanks for grabbing `swap`!

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610026288)
How come this method overload is not calling itself recursively?

Tested adding `BOOST_CHECK_EQUAL((Expected<void, int>{Unexpected{1}}.error()), 1);` to the tests to confirm it doesn't but don't understand why.

Is `this` in some kind of `&`-mode, making it call the method above?

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610039385)
nit: Might be nice to have these assert before failing due to an exception being thrown inside of `noexcept`.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610108777)
Yeah, finding a solution that works on all platforms would be even better. Don't think it's a case of the file missing, but rather that Windows forbids renaming parent directories when a process holds a file handle to a file.

Maybe we could rename files inside of the directory matching `blk*.dat` to `blk*.dat.bkp`? Haven't tested yet.

👍 rkrux approved a pull request: "test: Log IP of download server in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34045#pullrequestreview-3566844824)
tACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610120563)
I think it's slightly better as a general principle, to make test failures more intelligible. But it's very unlikely that anyone would even locally tweak the code being tested here so that it fails.

💬 stickies-v commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2610132975)
> Also left `include` in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds

I'm not sure that's feasible without also adding a `-deprecatedrpc` option to `cli` (unless that already exists), which I don't think is worth it? Afaik we don't support disjoint `bitcoind` and `bitcoin-cli` versions, and I think `bitcoin-cli` is more for manual usage anyway at which point just typing "debug" instead of "include" is trivial? The diff I added passes all tests and also adds
...

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610166839)
> Is `this` in some kind of `&`-mode, making it call the method above?

It is not `this`, but `(*this)`, see https://eel.is/c++draft/expr.prim#id.general-2

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610168017)
Why? Just seems more code for no reason?

📝 l0rinc opened a pull request: "fuzz: exercise `ComputeMerkleRoot` without `mutated` parameter"
(https://github.com/bitcoin/bitcoin/pull/34050)
The `mutated` parameter in `ComputeMerkleRoot` unlocks a different path that was always exercised in the fuzz test.
Adjusted to be fuzzer to pass `nullptr` as well to make sure that path is also tested: https://github.com/bitcoin/bitcoin/blob/24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94/src/consensus/merkle.cpp#L49-L53

Follow-up to https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589073735

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2610289485)
Thanks, added in https://github.com/bitcoin/bitcoin/pull/34050

💬 Raimo33 commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#issuecomment-3641566317)
Code Review ACK 031ee0b22c4ee4f04f66cb8f0be534b404dbe73c

The new benchmark is more realistic

💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2610315019)
please add braces to multiline loops

💬 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