Bitcoin Core Github
44 subscribers
122K links
Download Telegram
💬 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()`:
...
💬 parsaei889 commented on pull request "Ephemeral Anchors":
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-2200630235)
ندارم
💬 parsaei889 commented on pull request "Ephemeral Anchors":
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-2200630532)
عالی
💬 parsaei889 commented on pull request "Ephemeral Anchors":
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-2200631415)
خوب بود ممنون
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661325008)
Yes, it will return something other than `MempoolAcceptResult::ResultType::VALID`. Would happen as well if a conflicting transaction is mined in the meantime.
💬 pinheadmz commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200639046)
@vasild is it possible that in the intermittent case, some other test running in parallel has a process is listening on port 60000? So we connect OK but fail the i2p handshake?
💬 mzumsande commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200639857)
Looks to me like this is an actual bug that could happen in the real network when making an automatic outbound connection to yourself (where the `opencon` thread instead of a `httpworker` thread opens the connection, whereas the `net` thread accepts connection in both cases), so it's probably not just a ci/test failure?!

Maybe a workable fix would be to take the `PushNodeVersion` call out of `InitializeNode` and call it separately after having added the node to `m_nodes` (which would mean add
...
💬 brunoerg commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200659833)
> @vasild is it possible that in the intermittent case, some other test running in parallel has a process that is listening on port 60000? So we connect OK but fail the i2p handshake?

Yes, it's possible. I just started a server with `python3 -m http.server 60000` and ran the test (from master branch), I got:
```
AssertionError: [node 0] Expected messages "['Error connecting to h3r6bkn46qxftwja53pxiykntegfyfjqtnzbm6iv6r5mungmqgmq.b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not parti
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661339294)
There is some layer separation between `net`/`CConnman` and `net_processing`/`PeerManager` which I am trying not to blur too much. The lower `net` level only knows about connections and things like "I need to open N connections of type X". It does not know anything about transactions, or P2P messages which are handled by the `net_processing` layer.

The constant `NUM_PRIVATE_BROADCAST_PER_TX` (_send a transaction to this many peers_) belongs to the `net_processing` layer. Calling this `NumToOp
...
💬 brunoerg commented on pull request "fuzz: fix ciphertext size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2200709776)
CI failure is unrelated to this PR. It's the same from https://github.com/bitcoin/bitcoin/issues/30368
💬 andrewtoth commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2200722461)
I have the same comment from #28945: Why can we not revert https://github.com/bitcoin/bitcoin/commit/5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 to speed this up instead? I'm not clear why we need to have the `coinsCache` free up all associated memory to use the `PoolAllocator`.
💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2200723434)
Thanks AJ, it's great to get specific and find out what actual problems you are concerned about, before coming to the conclusion that it is too difficult to remove one hard dependency on global shared state, and therefore we should resort to workarounds such as running code in multiple processes, or finding ways to reset the state and coordinate access to it.

> how do we add temporary logging for development in kernel functions that don't have a manager object handy?

The same way you add t
...