🤔 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
```
(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
(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:
(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.
(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
...
(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
(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?
(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
(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()`:
...
(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)
ندارم
(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)
عالی
(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)
خوب بود ممنون
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-2200631415)
خوب بود ممنون
📝 fanquake locked a pull request: "Ephemeral Anchors"
(https://github.com/bitcoin/bitcoin/pull/29001)
Depends on https://github.com/bitcoin/bitcoin/pull/28948 and https://github.com/bitcoin/bitcoin/pull/28984
Replaces https://github.com/bitcoin/bitcoin/pull/26403 to refresh the conversation.
BIP text here: https://github.com/instagibbs/bips/blob/ephemeral_anchor/bip-ephemeralanchors.mediawiki
Example usage:
https://github.com/instagibbs/bolts/commits/zero_fee_commitment
https://github.com/instagibbs/lightning/commits/commit_zero_fees
TODO:
1) figure out what precisely to do in a r
...
(https://github.com/bitcoin/bitcoin/pull/29001)
Depends on https://github.com/bitcoin/bitcoin/pull/28948 and https://github.com/bitcoin/bitcoin/pull/28984
Replaces https://github.com/bitcoin/bitcoin/pull/26403 to refresh the conversation.
BIP text here: https://github.com/instagibbs/bips/blob/ephemeral_anchor/bip-ephemeralanchors.mediawiki
Example usage:
https://github.com/instagibbs/bolts/commits/zero_fee_commitment
https://github.com/instagibbs/lightning/commits/commit_zero_fees
TODO:
1) figure out what precisely to do in a r
...
💬 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.
(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?
(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
...
(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
...
(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
...
(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
(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`.
(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`.