💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011736752)
nit: I had to check the type of `value` to make sure this isn't needlessly copied - to make it slightly simpler to review, consider:
```suggestion
for (const auto& value : multipath->values) {
```

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011851451)
nit: since we're already touching this line we might as well format it

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011759617)
I don't like this third commit (nor the original implementation), we're looping over the splits, assigning a multipath if found, checking if it was already found during previous iterations, setting errors to output parameters and returning early, if we didn't return we assume it was correct, etc - it's too stateful, we're mixing not-strictly-related operations in a single loop, i.e. validation with usage, we should be able to separate the two concerns (it's a tiny loop).

👍 hebasto approved a pull request: "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain"
(https://github.com/bitcoin/bitcoin/pull/31849#pullrequestreview-2713381206)
ACK 963355037fe78eb4fbdda8631ac05a7b07fcec8c, tested on Ubuntu 24.10.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2011902071)
Yes, I believe it should.

🤔 glozow reviewed a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2713564288)
ACK 226d81f8b708ea1c3a39ec58446e291fe7440fdd

Thanks for adding the test! Checked that it'd catch any errors in the RPC code.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012000014)
I like the suggested change, it's cleaner and easier to follow. There's a bit of validation in the second loop as well but I guess there is no way to completely separate the validation from parsing. The alternative would be to parse the multipath nums in the first loop as well but I would prefer this implementation over that.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012003697)
A comment before the first loop would be nice stating `Validate multipath key path specifiers`

🤔 glozow reviewed a pull request: "fuzz: Fix off-by-one in package_rbf target"
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2713587920)
ACK fa5674c264d91eb3a99fa74ace8a1b6be113c0a8

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2751133325)
Re: https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2713105661

In the docs file, I would keep the `3` though. Explicitly stating the count of descriptors it will expand to is helpful.

✅ glozow closed an issue: "Add test coverage for getblocktemplate fees"
(https://github.com/bitcoin/bitcoin/issues/32053)

🚀 glozow merged a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897)

🤔 glozow reviewed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803#pullrequestreview-2713599065)
lgtm ACK a63bae6e281a9d4b0d4d6948550107683cf65d3b

💬 glozow commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r2012014088)
Worth noting this is pegged to the number at which `GatherClusters` halts (which would lead to `!IsReadyToCalculate`)

💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751151893)
Not sure what to make of the Win64 fuzz failure:

```
Assertion failed: DecodeBase64PSBT(psbt, random_string, error) == error.empty(), file
```

https://github.com/bitcoin/bitcoin/actions/runs/14055561051/job/39354164966?pr=32096#step:14:373

cc @maflcko

💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751158862)
> Not sure what to make of the Win64 fuzz failure:

https://github.com/bitcoin/bitcoin/pull/30746#discussion_r2011712656

💬 maflcko commented on pull request "test: replace hardcoded fee with node relay fee based calculation":
(https://github.com/bitcoin/bitcoin/pull/32058#discussion_r2012027710)
```suggestion

```


Actually, `FEE` here isn't a fee rate at all, but an absolute fee.

So it seems fine to just remove the todo and and assert completely.

💬 Sjors commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751192946)
cc @hebasto

🤔 ryanofsky reviewed a pull request: "multiprocess: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2713678101)
Updated 5554f9cb4abf7e1852dd79d2d75e714fa8cbeadd -> 0ec78961915da141b9c68a39ec0ebd5091c13be0 ([`pr/mine.20`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.20) -> [`pr/mine.21`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.21), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.20..pr/mine.21)) adding suggested test fix

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2012057155)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2007217890

Thanks! Applied fix