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

💬 vasild commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461986596)
In the future the callers of `AddrManImpl::GetAddr_()` can change, or new callers can be added. It is good to ensure that it does not misbehave or crash the program for some input.

I understand that this is not an universal argument and should be applied with reason, with limits. For example, the following is beyond reason: "`std::vector::reserve()` is going to crash the program for some input - if asked to allocate 1000TB, lets fix it!".

IMO `AddrManImpl::GetAddr_()` capping `max_pct` is
...

💬 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_r1832515709)
"Cache restored successfully" 🎉. "Generate build system" only took 4min now. Resolving.

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1832518874)
The fix for double-validation seems good.
--------------
Do you mind sharing the code of your attempt (assuming it compiles :))? I wanna take a look, but would be useful for archival reasons anyway.

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461998783)
> IMO AddrManImpl::GetAddr_() capping max_pct is reasonable. It already caps max_addresses. And then we can ensure it does by throwing any random numbers at it and making sure it does not break.

I agree. I can address it. I'm thinking about If `max_pct` is the maximum percentage of addresses to return. We could add an assert in `GetAddr` that this number is not greater than 100 and change the targets to use values from 0 to 100. I doesn't make sense to try to get 10000000% of the addresses, f
...

💬 vasild commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2462009631)
Yeah, that too. Then the test should indeed not call it with >100. If you go this way, then it should be clearly documented in:

```cpp
/**
* Return all or many randomly selected addresses, optionally by network.
*
* @param[in] max_addresses Maximum number of addresses to return (0 = all).
* @param[in] max_pct Maximum percentage of addresses to return (0 = all).
```

Somehow I feel it is safer to treat values >100 as 100 and document that instead of cras
...

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2462011095)
> Somehow I feel it is safer to treat values >100 as 100 and document that instead of crashing with an assert (or bad alloc).

Sure, I will address this.

💬 marcofleon commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1832534828)
Could be misunderstanding here, but wouldn't `m_txrequest.Count(peer)` always be greater than (or equal?) to `m_txrequest.CountInFlight(peer)`, so the additional assert would be redundant?

💬 marcofleon commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1832535338)
Update comment for the new assert?

💬 instagibbs commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2462023522)
Assuming we implemented everything correctly yes!

On Thu, Nov 7, 2024, 6:42 AM marcofleon ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/test/fuzz/txdownloadman.cpp
> <https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1832534828>:
>
> > @@ -287,7 +287,7 @@ static void CheckInvariants(const node::TxDownloadManagerImpl& txdownload_impl,
> // We should never have more than the maximum in-flight requests out fo
...

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2462024757)
Force-pushed adding the cap of `max_pct` in `GetAddresses`, changed the PR description as well. Thanks, @vasild.