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

💬 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_r1832539197)
This will create a stale ccache with fuzz blobs, which will then be picked up the normal build, no?

💬 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_r1832543950)
Hm, should we have a separate ccache for each job type? I assume, there is no way to merge the ccache from the two

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

hmm... you make a good point

💬 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_r1832585089)
> have a separate ccache for each job type

Done

💬 rkrux commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2462108426)
> Though I don't completely understand why this comment "ok to not have a config file" (and subsequent flow) is allowed

My guess it is applicable when the daemon is run without a conf because this portion lies in `common/`.

👍 vasild approved a pull request: "fuzz: fix bad alloc in connman target"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2420857067)
ACK 81f26e9225838a1f0e0293c84f875efab95d9724

💬 vasild commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1832608263)
It would be good extend the `GetAddr()` doc to something like the following. And then no need to cap to 200 here.

```diff
--- i/src/addrman.h
+++ w/src/addrman.h
@@ -163,13 +163,13 @@ public:
std::pair<CAddress, NodeSeconds> Select(bool new_only = false, const std::unordered_set<Network>& networks = {}) const;

/**
* Return all or many randomly selected addresses, optionally by network.
*
* @param[in] max_addresses Maximum number of addresses to return (
...

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1832614510)
The cap to 200 is just to facilitate exploring valid ranges and some exceeds.

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1832614816)
Going to extend the doc. Thanks.

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1832616826)
Done.