💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261202332)
nit: `has_...` is not really useful anymore i guess
```suggestion
if (rpc_target, {}) in targets:
```

👍 stickies-v approved a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1526397826)
crACK fa6824245928dfdbfce75f7e62d646c7e09c9e77

Seems sensible to take a similar approach as in #28015, but I'm not very familiar with fuzzing so my ACK is mostly on the mechanics of the code, which LGTM.

📝 MarcoFalke opened a pull request: "ci: Add missing -O2 to valgrind tasks"
(https://github.com/bitcoin/bitcoin/pull/28071)
Currently the tasks have nothing (`-O0`) set, which makes them slow.

Fix this by falling back to the default (`-O2`).

💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261216419)
thx,fixed

💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#issuecomment-1632577575)
re-crACK fa6245da6061050eb77ad07cd4caf8c596d89dc6

👍 brunoerg approved a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1526435808)
reACK fa6245da6061050eb77ad07cd4caf8c596d89dc6

code lgtm! better with git grep.

targets with rpc and process_message:
```sh
➜ bitcoin-core-dev git:(28066-marco) ✗ ./test/fuzz/test_runner.py corpus process_message rpc -g
2 of 168 detected fuzz target(s) selected: process_message rpc
Generating corpus to corpus
[('process_message', {}), ('rpc', {}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'version'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'verack'}), ('process_message', {
...

⚠️ fanquake opened an issue: "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)"
(https://github.com/bitcoin/bitcoin/issues/28072)
Using [master](357e3f6aa476658aecae7239b4b06d2bc362ff1e), GCC 12.2.1, Valgrind 3.21.0 (system), aarch64, Alpine Linux (musl libc):
```bash
./test/functional/test_runner.py --combinedlogslen=19999 --timeout-factor=12 --jobs=9 --valgrind
...
24/268 - p2p_dns_seeds.py failed, Duration: 9 s
....
node0 2023-07-12T12:59:04.448921Z [init] [noui.cpp:56] [noui_InitMessage] init message: Done loading
node0 2023-07-12T12:59:04.453823Z [addcon] [util/thread.cpp:20] [TraceThread] addcon thread star
...

💬 dergoegge commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632660836)
> which makes them slow

How much faster are they with `-O2`?

💬 MarcoFalke commented on issue "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)":
(https://github.com/bitcoin/bitcoin/issues/28072#issuecomment-1632778239)
To reproduce without tests:

```
# valgrind --gen-suppressions=all --quiet --error-exitcode=1 --suppressions=$PWD/contrib/valgrind.supp --exit-on-first-error=yes ./src/bitcoind -regtest -noconnect -addnode=some.node -printtoconsole=0
==47801== Thread 25 b-addcon:
==47801== Syscall param ppoll(ufds.events) points to uninitialised byte(s)
==47801== at 0x405DCCC: ??? (in /lib/ld-musl-aarch64.so.1)
==47801== Address 0x823b724 is on thread 25's stack
==47801==
{
<insert_a_suppressi
...

💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632780362)
> How much faster are they with -O2?

Yes.

(Locally the functional tests go from `(hasn't finished)` to `19769s (accumulated)`)

💬 dergoegge commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632783352)
> (Locally the functional tests go from (hasn't finished) to 19769s (accumulated))

Any results for the fuzz task? Just to compare it to the speed up observed in #27992

💬 MarcoFalke commented on issue "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)":
(https://github.com/bitcoin/bitcoin/issues/28072#issuecomment-1632784452)
Maybe someone can transform this into a standalone minimal cpp file to submit to valgrind or musl?

💬 hebasto commented on pull request "guix: Remove librt usage from release binaries":
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1632831214)
Guix builds:
```
67078bddd5dc32801b8c916c3bc12f1404da572312f0158a89b9603c1f753969 guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/SHA256SUMS.part
794dd00009860fd67d7e51463ee1c5ea9677dfff1c739dd0b91cf73136deb655 guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/bitcoin-8f6f0d81ee3a-aarch64-linux-gnu-debug.tar.gz
eb9cf3f472ffbc37446fe4d80fe81dc62cf1c28c4d57dd8a7b7176e65487aeeb guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/bitcoin-8f6f0d81ee3a-aarch64-linux-gnu.tar.gz
e775a9e9b23be44b5c7
...

💬 MarcoFalke commented on issue "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)":
(https://github.com/bitcoin/bitcoin/issues/28072#issuecomment-1632844998)
Done:

```
/bitcoin-core # cat /tmp/a.cpp
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>

int main(){
addrinfo ai_hint{};
ai_hint.ai_socktype = SOCK_STREAM;
ai_hint.ai_protocol = IPPROTO_TCP;
ai_hint.ai_family = AF_UNSPEC;
ai_hint.ai_flags = true? AI_ADDRCONFIG : AI_NUMERICHOST;
addrinfo* ai_res{nullptr};
return getaddrinfo("foo.bar", nullptr, &ai_hint, &ai_res);
}
/bitcoin-core # clang++ /tmp/a.cpp -o /tmp/exe && valgrind --gen-s
...

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261403669)
Would be cleaner to write:
```suggestion
if (!Assume(tx)) {
return false;
}
```

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261410424)
(I've mentioned this offline as well)

The locking assumptions around this check are weird because `m_txpackagetracker` has its own internal mutex where as `m_txrequest` is guarded by `cs_main`. There is nothing stopping `m_txpackagetracker->Count()` form returning something different right after this check.

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261405442)
This can be dropped in favor of just checking that the CNodeState exists

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261416426)
Not a big fan of using `Assume` like this because we can't test these conditions but they **can** happen in production (if the caller is doing something wrong).

I'd suggest just returning if these assumptions don't hold.

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261414179)
Imo, it would be nicer to do the orphanage changes separately, including amended functional/unit/fuzz tests.

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261412063)
```suggestion
void PeerManagerImpl::AddOrphanAnnouncer(NodeId nodeid, const CTransactionRef& tx, std::chrono::microseconds current_time)
```