💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200413138)
@glozow We both noticed this independently. @TheCharlatan Had it queued up as part of his next batch of changes (not yet PR'd), but I didn't know that when I PR'd this one. He's since agreed that it makes sense to merge this on its own.

#30141 is unrelated, he's just waiting for that to be merged before PR'ing his next batch of kernel changes.

💬 ishaanam commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661201441)
Done

💬 ishaanam commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661201934)
Done, thanks

💬 glozow commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200424459)
> https://github.com/bitcoin/bitcoin/pull/30141 is unrelated, he's just waiting for that to be merged before PR'ing his next batch of kernel changes.

I can see that they aren't related, just trying to clarify which one to merge first because the PRs conflict. I figured this would be an ok place to ask that since it's the same people. Apologies, I wasn't trying to imply that they're related at all, my previous comment was to provide an explanation for why I was asking which one to prioritize.

📝 maflcko opened a pull request: "fuzz: Mutate -max_len= during generation phase"
(https://github.com/bitcoin/bitcoin/pull/30371)
This revives https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1927243781, because it helps to find https://github.com/bitcoin/bitcoin/issues/30367, which failed to be found for more than a year on any of the existing fuzz servers.

Locally, I can find the bug with `-max_len=84 -use_value_profile=1` on a single thread on a laptop. The reason is likely that a smaller max_len results in a faster fuzzing speed (iterations). As a side-effect it may also be effective at reducing the size o
...

💬 furszy commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661216674)
can now be respent without manually abandoning the transactions "when the parent transaction is dropped from the mempool"?

👍 theuni approved a pull request: "ci: Clear unused /msan/llvm-project"
(https://github.com/bitcoin/bitcoin/pull/30369#pullrequestreview-2151691321)
utACK fa6beb8cfcabd58273f15e03f781016ac610e788

Could delete the sdk tarball as well.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2200478173)
Rebased after the merge of #30237.

📝 maflcko opened a pull request: "util: Use SteadyClock in RandAddSeedPerfmon"
(https://github.com/bitcoin/bitcoin/pull/30372)
`GetTime` is mockable in tests and system-changeable in production. This should be fine and not lead to issues, but using `SteadyClock` is more correct in this context to do an expensive task only so often.

💬 vasild commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200493378)
> I'm not 100% convinced of the change in this PR

Right. This looks quite bizarre to me. `Session::Hello()` creates the socket and it shouldn't be able to proceed since it shouldn't be able connect to `127.0.0.1:60000`. However, from the log message `Missing RESULT=` it seems it managed to call `SendRequestAndGetReply(*sock, "HELLO VERSION MIN=3.1 MAX=3.1");`, write to that socket, go a reply and is upset because the reply is missing `RESULT=`, and the reply is the same as the request :raised
...

💬 maflcko commented on pull request "ci: Clear unused /msan/llvm-project":
(https://github.com/bitcoin/bitcoin/pull/30369#issuecomment-2200506998)
> Could delete the sdk tarball as well.

IIRC it is only `77M` and a volume (https://github.com/bitcoin/bitcoin/blob/0bd2bd1efb4b88d7f9eb9a1203560d69e07d97c3/ci/test/02_run_container.sh#L30), so it should only consume the `77M` once per machine, regardless of whether an image is re-generated or not.

💬 maflcko commented on pull request "ci: Clear unused /msan/llvm-project":
(https://github.com/bitcoin/bitcoin/pull/30369#issuecomment-2200511677)
For me, msan is the largest image locally so far:

```
podman image ls 'localhost/ci_*'
REPOSITORY TAG IMAGE ID CREATED SIZE
localhost/ci_native_asan latest 1ae3884f0bd3 6 days ago 1.81 GB
localhost/ci_native_valgrind latest fa2461c0e0d5 2 weeks ago 1.36 GB
localhost/ci_native_fuzz_msan latest 682198747e18 2 weeks ago 6.02 GB
localhost/ci_native_fuzz_valgr
...

💬 dergoegge commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2200523989)
I don't know how to evaluate if this is a good idea but I'm also not using the test runner, so wouldn't mind. Also, I don't think oss-fuzz is doing this, so how come they found the bug but we didn't?

💬 vasild commented on pull request "test: p2p: check that connecting to ourself leads to disconnect":
(https://github.com/bitcoin/bitcoin/pull/30362#issuecomment-2200524871)
So this tiny new test revealed a race and a bug in the existent code, excellent! :heart:

💬 fjahr commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2200535832)
CI failure is unrelated (#30368).

🚀 fanquake merged a pull request: "ci: Clear unused /msan/llvm-project"
(https://github.com/bitcoin/bitcoin/pull/30369)

🤔 hebasto reviewed a pull request: "ci: Clear unused /msan/llvm-project"
(https://github.com/bitcoin/bitcoin/pull/30369#pullrequestreview-2151784097)
ACK fa6beb8cfcabd58273f15e03f781016ac610e788

https://cirrus-ci.com/task/5273350220546048:
```
+ du -sh /msan/llvm-project
2.1G /msan/llvm-project
+ rm -rf /msan/llvm-project
```

💬 ishaanam commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661292675)
Done

💬 vasild commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200583405)
Calling `InitializeNode()` after acquiring `m_nodes_mutex` has the potential to cause a deadlock because of acquiring `cs_main` and `m_nodes_mutex` in different order in different parts of the code:

1. Existent code: `PeerManagerImpl::NewPoWValidBlock()` acquires `cs_main` and calls `m_connman.ForEachNode()` which acquires `m_nodes_mutex`.
2. New code: `CConnman::OpenNetworkConnection()` acquires `m_nodex_mutex` and calls `InitializeNode()` which acquires `cs_main`

:bomb:

📝 brunoerg opened a pull request: "fuzz: fix ciphertext size in `crypter`"
(https://github.com/bitcoin/bitcoin/pull/30373)
Fixes #30251

This PR sets a max length for cyphertext in `ConsumeRandomLengthByteVector`. I set 64, should be enough, I couldn't see anything greater than 48 in practice tbh.