🤔 stickies-v reviewed a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1526047738)
Concept ACK, reviewed code mechanically and looks good too.

💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1260977018)
I was thinking about suggesting this refactoring in the previous PR, glad to see it happen here. Since there's almost no code reuse between both targets, would prefer splitting it out into 2 smaller functions. The diff below does that, plus:
- use pathlib
- avoid removing and then re-adding the unlimited search target (not sure if having it at the beginning instead of at the end of targets makes a meaningful difference?)
- add some docstrings

<details>
<summary>new code</summary>

```py
...

💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261042551)
Could make this a bit more robust by only considering lines within `namespace NetMsgType`?

```py
if has_p2p_msg:
cmd = ["awk" , "/namespace NetMsgType {/,/} \/\/ namespace NetMsgType/ { print $0 }", str(Path(src_dir) / 'src' / 'protocol.cpp')]
lines = subprocess.run(
cmd,
check=True,
stdout=subprocess.PIPE,
text=True,
).stdout.splitlines()
lines = [l.split('"', 1)[1].split('"')[0] for l in lines if l.
...

💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261092272)
`awk` will probably fail on macOS again? Also, there shouldn't be any risk in being too greedy, because the fuzz target also validates the argument. It seems more risky to be too little greedy.

💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261093422)
Thanks, added your docstrings

💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261099569)
Works fine here (macOS 13.4) but I'm not familiar with its portability, so if that's been an issue in the past then yeah might not be ideal. I agree that being too greedy is not a huge risk but if we can be specific I think that's still preferred. Alternatively, could also use `git grep` with `g_all_net_message_types` like we do for the `rpc` target - didn't see that earlier.

💬 naumenkogs commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1632462419)
Concept ACK

----------------

I was curious whether bumping a sequence number (RBF, then RBF back the target tx) is anything interesting. I think it's ok:
- it doesn't provide any advantage for relay-delay over the regular RBF
- it doesn't look different either (NOTFOUND as opposed to ignoring the GETDATA or whatnot) — might be worth mentioning somewhere that they should look equal?

💬 theuni commented on pull request "guix: Remove librt usage from release binaries":
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1632486009)
Concept ACK.

IIRC we left this because it was harmless as most systems kept a stub for librt. I suppose the situation has now inverted and relying on the stub is deprecated? (Also, it's possible I'm mixed up and thinking of libm or similar)

📝 MarcoFalke opened a pull request: "test: Drop 22.x node from TxindexCompatibilityTest"
(https://github.com/bitcoin/bitcoin/pull/28070)
The node in this test was never really needed, because the compatibility tests shouldn't be used to test previous releases. (The test suite of the previous release should be used for that). So remove it.

Also, other test changes. (See individual commits)

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

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1261190985)
Yeah, looks like you are right. Thanks, done.

💬 hebasto commented on pull request "guix: Remove librt usage from release binaries":
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1632560222)
> Our release binaries currently have a runtime dependency on `librt`. However this is redundant...

Confirming. From https://www.man7.org/linux/man-pages/man3/clock_gettime.3.html:

> Link with `-lrt` (only for glibc versions before 2.17).

💬 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`?