🤔 maflcko reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2420377681)
Left a nit. Also, the tests seem to fail when running on each commit individually.

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1832329946)
nit in f1fa6a5dda8f6b640b6377651e6d76df7ff77e14: Not sure about removing the early-return on invalid just to have it trickle through the rest of the code to a debug log statement. As seen in your last force-push, this makes the code more verbose, complex and brittle (easy to get wrong). (https://github.com/bitcoin/bitcoin/compare/28d67cd01c546fa9ce0d98be208ab6a19f1efbb0..f1fa6a5dda8f6b640b6377651e6d76df7ff77e14)

What do you think about adding a small logging lambda function, with `LogInfo("Bl
...

💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2461799300)
> It's not entirely clear to me why BnB should care about whether the excess is going to the recipient or not. I don't think that should change the algorithm at all, so the changes to `SelectCoinsBnB` seem unnecessary.

Imagine this scenario:
- target is 100
- max_excess is 30
- UTXOs: [20, 20, 20, 20, 20, 120]
- fee to spend an input: 4
- minimum spendable: 10

Input set A: [20,20,20,20,20]
Input set B: [120]

Current BnB computes waste as:
Input set A: 20 (best)
Input set
...

👍 dergoegge approved a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238#pullrequestreview-2420537659)
lgtm ACK fa461d7a43aa5a321278c8f734e80fb7aa79bfdb

🚀 fanquake merged a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238)

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461912635)
> I think that the problem warrants a fix in the real code, not in the test.

We can still limit the values to reflect the reality. I don't see how to have a huge `max_pct` in practice. For example, for `GETADDR`, we use `MAX_ADDR_TO_SEND`/`MAX_PCT_ADDR_TO_SEND`. For the RPC `getnodeaddresses` and other usage, its value is always zero.

💬 maflcko commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832478373)
It seems a bit annoying that the Windows build is now duplicated and seems to take 2h each times 2 = 4 CI hours total.

I wonder if this could be avoided (or reduced) by using a cmake multi config build. Putting the fuzz result into a separate folder and then using that in the fuzz_runner should be enough?

🤔 rkrux reviewed a pull request: "util: Narrow scope of args (-color, -version, -conf, -datadir)"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2420533755)
Concept ACK 9e130811a3f68aea77ddeecf667aa8389a2940e8

Thanks for catching issues here, gave me an opportunity to read the app init code a bit more!

Successful make and functional tests. Left couple suggestions in the first 2 commits. Disallowing `noconf` and `nodatadir` conceptually makes sense to me. I tested with these 2 args and the output I received seems okay to me.

```
➜ bitcoin git:(2024/11/invalid_args) ✗ bitcoinclireg -noconf
Error parsing command line arguments: Negating of
...

💬 rkrux commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1832423387)
Good catch. I was confused initially wondering why doesn't color show up on explicitly setting this arg. `in combination` gives an impression that somehow both these args need to be passed together.
```
Only applies to the output of -getinfo.
```

💬 rkrux commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1832430228)
Again great catch. I noticed the difference in the output in master and this branch.

```
➜ bitcoin git:(master) ✗ bitcoinclireg -noversion
Bitcoin Core RPC client version v28.99.0-65b194193661
Copyright (C) 2009-2024 The Bitcoin Core developers

Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.

This is experimental software.
Distrib
...

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832481359)
> It seems a bit annoying that the Windows build is now duplicated and seems to take 2h each times 2 = 4 CI hours total.

I assumed this is a caching issue? This shouldn't be doing anything different from the current windows CI...

💬 fanquake commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832488952)
The majority of the time here is vcpkg compiling Qt modules from source, in the "Generate build system step": https://github.com/bitcoin/bitcoin/actions/runs/11705992609/job/32602049089?pr=31221. That should be getting cached in some way. Maybe push again and see if things are rebuilt?

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832492614)
Re-pushed

💬 fanquake commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832495883)
I would also think we can just turn off everything GUI related off for fuzzing?

💬 maflcko commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832499559)
> I would also think we can just turn off everything GUI related off for fuzzing?

I don't think this is possible while sharing the cache

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832500530)
Looks like it's rebuilding everything again, could it be that it can't store new caches on PRs?

💬 maflcko commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1832502588)
Could this commit be a scripted-diff?

💬 naumenkogs commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1832504252)
Perhaps `CountInFlight` should check against `MAX_PEER_TX_REQUEST_IN_FLIGHT` and `Count` against `MAX_PEER_TX_ANNOUNCEMENTS`, separately?
I think that's what Greg wanted to suggest.

💬 naumenkogs commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2461975409)
ACK https://github.com/bitcoin/bitcoin/commit/5dc94d13d419b8d5e543cb50edeb872335c090e7

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832508570)
> Looks like it's rebuilding everything again, could it be that it can't store new caches on PRs?

The pkg cache is not stored on PRs (see [here](https://github.com/bitcoin/bitcoin/blob/2a0997b3a5dae13e1b42776123bc9a69781d2222/.github/workflows/ci.yml#L212)).

But I think the main problem was that I changed the job name from `win64-native` to `windows-native` which changed the lookup key for the cache (I assume). Re-pushed again.