📝 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`.
💬 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
...
(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
...
💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2200737355)
Code review ACK a05f4ca241045e8a4b295760590805f961c2e5e7.
Just changed psbt error string and fixed a test since the last review.
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2200737355)
Code review ACK a05f4ca241045e8a4b295760590805f961c2e5e7.
Just changed psbt error string and fixed a test since the last review.
👍 pinheadmz approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2152010530)
ACK ffe9b274984a351716348a6c99df19c391bfdc8e
Re-reviewed changes since last ACK, including addition of `tor_port()` in tests which I like.
Also ran my extra test again from https://gist.github.com/pinheadmz/0bea4bf8d8bf9af85f8ae326286d3082
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK ffe9b274984a351716348a6c99df19c391bfdc8e
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaC8jkACgkQ5+KYS2KJ
yT
...
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2152010530)
ACK ffe9b274984a351716348a6c99df19c391bfdc8e
Re-reviewed changes since last ACK, including addition of `tor_port()` in tests which I like.
Also ran my extra test again from https://gist.github.com/pinheadmz/0bea4bf8d8bf9af85f8ae326286d3082
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK ffe9b274984a351716348a6c99df19c391bfdc8e
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaC8jkACgkQ5+KYS2KJ
yT
...
💬 willcl-ark commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2200759744)
Concept ACK.
I was tacking this today (missed this PR as GH search for "sse" yielded no results 🤦🏼♂️ )
First I tried re-implementing @luke-jr previous approach, but refactoring into macros: https://github.com/bitcoin/bitcoin/commit/cdaa9bfd6e63dc675cdd27e6d586f69423ee1748
Whilst this works well-enough, it didn't particularly fix https://github.com/bitcoin/bitcoin/issues/28864 in my opinion, so I tried https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:detect-sse4.
...
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2200759744)
Concept ACK.
I was tacking this today (missed this PR as GH search for "sse" yielded no results 🤦🏼♂️ )
First I tried re-implementing @luke-jr previous approach, but refactoring into macros: https://github.com/bitcoin/bitcoin/commit/cdaa9bfd6e63dc675cdd27e6d586f69423ee1748
Whilst this works well-enough, it didn't particularly fix https://github.com/bitcoin/bitcoin/issues/28864 in my opinion, so I tried https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:detect-sse4.
...