🤔 vasild reviewed a pull request: "rpc: add 'getnetmsgstats' RPC"
(https://github.com/bitcoin/bitcoin/pull/28926#pullrequestreview-1765066743)
Concept ACK

I have applied the suggestions below in [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats) plus some other simplifications. Feel free to pick them.

Thanks!

💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415634706)
These might as well be private. It would be good to completely hide this to/from index from the outside world because it can be misused in a dangerous way. Then provide a way to call a provided function for each element:

```cpp
void TrafficStats::ForEach(std::function<void(TrafficStats::Direction dir,
Network net,
ConnectionType con,
const std::string&
...

💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415625631)
This method is not needed. It is simpler and suffices to explicitly initialize the members of `MsgStat` to zero:

```cpp
std::atomic_uint64_t byte_count{0}; //!< Number of bytes transferred.
std::atomic_uint64_t msg_count{0}; //!< Number of messages transferred.
```

💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415601693)
Commit d3b34b82af33909eaeffe8d596a6c70951dc844c `net: move NET_MESSAGE_TYPE_OTHER` is not needed and can be dropped if `messageTypeFromIndex()` and `messageTypeToIndex()` standalone functions are made private methods in the `NetStats` class and their usage limited to within the class. That is desirable anyway because they are only relevant for an index in an array that is defined in a particular way and is (should be) private within `NetStats`.

💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1417286331)
This shouldn't be necessary. It means that the previous test did not leave the environment as it was when it started. That is the newly added `test_getnetmsgstats`. It is good to be able to run the tests in isolation and that they do not rely on the leftovers from previous tests. For example if:

```
self.test_connection_count()
self.test_getpeerinfo()
self.test_getnettotals()
self.test_getnetmsgstats()
self.test_getnetworkinfo()
sel
...

💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415682660)
The code already collects the traffic data for the per peer stats. To avoid confusion and ease maintenance it would be nice if the per peer and global traffic data is collected at the same place. This is where the data is collected in `master` for the per peer stats:

Received:
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/net.cpp#L663
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/net.cpp#L669-L674

Sent:
https:
...

💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1417282448)
I realized that this can be done in a simpler way - if the stats are put in a map and `""` used for the aggregated dimensions. See commit b7c56affe9 `rpc: make it possible to aggregate the result in getnetmsgstats` from [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats).

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1842914664)
[clusterfuzz-testcase-bitdeque-5480558045822976.bin.txt](https://github.com/bitcoin/bitcoin/files/13583557/clusterfuzz-testcase-bitdeque-5480558045822976.bin.txt)

📝 dergoegge opened a pull request: "wip: Split fuzz binary"
(https://github.com/bitcoin/bitcoin/pull/29010)
Closes #28971

* Split each harness into it's own file
* Support compiling individual harness through `CPPFLAGS="-DFUZZ_HARNESS=<harness name>"`
* Build individual binaries

The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).

TODOs:
- [ ] Introduce option for building individual binaries
- [ ] Deal with `test/fuzz/{tx_pool, tx_package_eval, deserialize}.cpp`
- [ ] One harness per
...

💬 dergoegge commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1842946104)
cc @maflcko

turns out we can actually keep the map if we just don't add all the harness functions into it

💬 dergoegge commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1842958846)
Should we do the same for `process_message`? would need to change it to use `ProcessMessagesOnce` as well.

👍 fanquake approved a pull request: "ci: Use Ubuntu 24.04 Noble for asan,tsan,tidy,fuzz"
(https://github.com/bitcoin/bitcoin/pull/28992#pullrequestreview-1767679627)
ACK fad2392c5861a88a87cb8a03d2fc9773e178feb8

🚀 fanquake merged a pull request: "ci: Use Ubuntu 24.04 Noble for asan,tsan,tidy,fuzz"
(https://github.com/bitcoin/bitcoin/pull/28992)

💬 dergoegge commented on pull request "fuzz: rule-out too deep derivation paths in descriptor parsing targets":
(https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1842974001)
Could you add the exception for the `scriptpubkeyman` harness as well?

https://github.com/bitcoin/bitcoin/blob/d85491404352c8b20bd7c317c129dca0763aead2/src/wallet/test/fuzz/scriptpubkeyman.cpp#L54-L56

📝 fanquake opened a pull request: "[26.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/29011)
Backports for 26.x. Currently:
* https://github.com/bitcoin/bitcoin/pull/28992

💬 fanquake commented on pull request "ci: Use Ubuntu 24.04 Noble for asan,tsan,tidy,fuzz":
(https://github.com/bitcoin/bitcoin/pull/28992#issuecomment-1842974602)
Will be backported in #29011.

💬 brunoerg commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417402704)
In 964e1a8faec984df2799171bc2a9268a3765fbca: Any specific reason to increase the number of added addresses here?

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1417404342)
Tracepoints are OS level hooks that are not part of the software by default. Logging is about keeping the history of events that occurred along the software lifetime. This does not only involve errors, it also involves information about the soft activity.
Unlike logging, which can be enabled on-the-fly by simply calling an RPC command, users cannot enable tracepoints on their node without recompiling the software with a specific flag. Which requires compiling knowledge, forces the user to shutd
...

💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417424421)
This was lowered to 1 tried and 1 new in https://github.com/bitcoin/bitcoin/pull/23084 as more than one address meant the test could sporadically fail if there's a collision.

@stratospher, I think, if you want to make this clearer, you could have this commit be a revert of https://github.com/bitcoin/bitcoin/commit/5825b34783545f9470d5ab94b87c918980715675 or note it in the commit message.

💬 brunoerg commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417428585)
Thanks, @0xB10C. An explanation in the commit message would be valuable.