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

👍 pinheadmz approved a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2151672225)
ACK 60a753b7b38fbe89494f8df66f13a84f28af244b

Tested in an ubuntu docker container with local ipv6 but not external ipv6:

```
# ip address | grep inet
inet 127.0.0.1/8 scope host lo
inet6 ::1/128 scope host
inet 172.17.0.2/16 brd 172.17.255.255 scope global eth0
```

Confirmed the `feature_proxy` test fails without the patch. Added extra logging to confirm the error:

```
getaddrinfo: ::1 error: Address family for hostname not supported
```

With the patch, `getad
...

💬 pinheadmz commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1661217742)
Might consider using `&` here since we're dealing with bitfield flags and not absolute values

💬 pinheadmz commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1661299539)
You could check specifically for `EAI_FAMILY` and then only recheck without `AI_ADDRCONFIG` in that condition. I think the code you have now will always check everything twice, but ignore duplicates?

💬 sipa commented on pull request "util: Use SteadyClock in RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/30372#issuecomment-2200613939)
utACK fa360b047fc578fd855b8f7420d84dc967884f4a

💬 vasild commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017)
Rudimentary idea: insert the entry in `CConnman::m_nodes` just before `PushVersion()`.

```cpp
void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services)
{
...
// void CConnman::NodesAppend() { WITH_LOCK(m_nodes_mutex, m_nodes.push_back(node)); }
m_connman.NodesAppend(node);
if (!node.IsInboundConn()) {
PushNodeVersion(node, *peer);
}
}
```

or, same idea but implement by splitting the `PushNodeVersion()` bit away from `InitializeNode()`:
...