💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1842735340)
> Can we get rid of linker warnings:
I assume this is a combination of `-mmacosx-version-min`, `-Wl,-platform_version` and maybe cctools. Note that the warnings have also already been appearing in our security checks/tests, for some time. This should dissapear when we switch to lld, so it might not be worth making more changes now.
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1842735340)
> Can we get rid of linker warnings:
I assume this is a combination of `-mmacosx-version-min`, `-Wl,-platform_version` and maybe cctools. Note that the warnings have also already been appearing in our security checks/tests, for some time. This should dissapear when we switch to lld, so it might not be worth making more changes now.
💬 naumenkogs commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1417175382)
Assume `fDisconnect` could be useful here too. Just in case `ForEachNode` gets updated.
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1417175382)
Assume `fDisconnect` could be useful here too. Just in case `ForEachNode` gets updated.
💬 naumenkogs commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1417178196)
Not sure why anyone would do `maxconnections=1` in practice, but then if their firewall allows to find only one blocksonly node (and they are not in blocksonly), the behavior might be confusing: they will be frequently disconencted from this peer.
Perhaps, this could be logged if maxconnections=1
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1417178196)
Not sure why anyone would do `maxconnections=1` in practice, but then if their firewall allows to find only one blocksonly node (and they are not in blocksonly), the behavior might be confusing: they will be frequently disconencted from this peer.
Perhaps, this could be logged if maxconnections=1
📝 maflcko opened a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.
Fix this by detecting them in the fuzz target.
Can be tested by introducing a bug such as:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::ato
...
(https://github.com/bitcoin/bitcoin/pull/29009)
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.
Fix this by detecting them in the fuzz target.
Can be tested by introducing a bug such as:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::ato
...
💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842754101)
NACK. There's no universal way to filter all present and future datacarrying styles. This invites an endless cat and mouse game inside very critical code paths.
It seems better if you just keep this patch for Knots, or alternatively, come up with a generic design to filter transactions. E.g. by loading rules from a file in some template language.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842754101)
NACK. There's no universal way to filter all present and future datacarrying styles. This invites an endless cat and mouse game inside very critical code paths.
It seems better if you just keep this patch for Knots, or alternatively, come up with a generic design to filter transactions. E.g. by loading rules from a file in some template language.
💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842759347)
Just to set expectations, I might also NACK such a generic arbitrary filtering system, given the can of worms it would open, but I'd have to think about that more.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842759347)
Just to set expectations, I might also NACK such a generic arbitrary filtering system, given the can of worms it would open, but I'd have to think about that more.
🤔 furszy reviewed a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009#pullrequestreview-1767421817)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29009#pullrequestreview-1767421817)
Concept ACK
💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1842790794)
Fixed up capnp as well. Discussed this a bit with @theuni yesterday, and it seems to most straightforward to just keep using `/lib` (also discussed above) given thats how depends was built to work, and it's not clear why changing things to exist in various (depending on the system and it's configuration) GNU dirs is any better (for now). If we actually switched all packages in depends to using CMake, maybe we could revisit this (also assuming they all themselves use GNUInstallDirs).
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1842790794)
Fixed up capnp as well. Discussed this a bit with @theuni yesterday, and it seems to most straightforward to just keep using `/lib` (also discussed above) given thats how depends was built to work, and it's not clear why changing things to exist in various (depending on the system and it's configuration) GNU dirs is any better (for now). If we actually switched all packages in depends to using CMake, maybe we could revisit this (also assuming they all themselves use GNUInstallDirs).
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1842795172)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1842795172)
Concept ACK
💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1417239636)
> Is this workaround still needed with https://github.com/bitcoin/bitcoin/pull/28856? That PR seems to drop the android workaround, so maybe switching both of these libraries to cmake would just fix the problem automatically.
Yes, the switch to CMake in #28856 didn't fix this issue.
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1417239636)
> Is this workaround still needed with https://github.com/bitcoin/bitcoin/pull/28856? That PR seems to drop the android workaround, so maybe switching both of these libraries to cmake would just fix the problem automatically.
Yes, the switch to CMake in #28856 didn't fix this issue.
💬 naumenkogs commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1417250008)
ea3138b7b189158db80bfd1f9d9dd17e7ef52f3f
might as well jump `pindex` forward here, so that the second condition is satisfied?
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1417250008)
ea3138b7b189158db80bfd1f9d9dd17e7ef52f3f
might as well jump `pindex` forward here, so that the second condition is satisfied?
💬 aureleoules commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1842819914)
> Maybe something was being cached, as this seems to have updated now.
Yes it may take some time for the sonarcloud worker to finish, as well as sonarcloud to refresh the results.
I'll improve the UX for that 😅
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1842819914)
> Maybe something was being cached, as this seems to have updated now.
Yes it may take some time for the sonarcloud worker to finish, as well as sonarcloud to refresh the results.
I'll improve the UX for that 😅
💬 naumenkogs commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1842833890)
The code looks alright, but i'd still rather have [this](https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930) addressed first. It looks like even #28170 doesn't directly handle this, but rather prepare the codebase for another PR handling it?
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1842833890)
The code looks alright, but i'd still rather have [this](https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930) addressed first. It looks like even #28170 doesn't directly handle this, but rather prepare the codebase for another PR handling it?
💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842866482)
I would NACK that as you are creating infrastructure for things like OFAC censorship.
On December 6, 2023 1:21:15 PM GMT+01:00, Sjors Provoost ***@***.***> wrote:
>Just to set expectations, I might also NACK such a generic arbitrary filtering system, given the can of worms it would open, but I'd have to think about that more.
>
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842866482)
I would NACK that as you are creating infrastructure for things like OFAC censorship.
On December 6, 2023 1:21:15 PM GMT+01:00, Sjors Provoost ***@***.***> wrote:
>Just to set expectations, I might also NACK such a generic arbitrary filtering system, given the can of worms it would open, but I'd have to think about that more.
>
🤔 vasild reviewed a pull request: "rpc: add 'getnetmsgstats' RPC"
(https://github.com/bitcoin/bitcoin/pull/28926#pullrequestreview-1765066743)
Concept ACK
I have applied the suggestions below in [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats) plus some other simplifications. Feel free to pick them.
Thanks!
(https://github.com/bitcoin/bitcoin/pull/28926#pullrequestreview-1765066743)
Concept ACK
I have applied the suggestions below in [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats) plus some other simplifications. Feel free to pick them.
Thanks!
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415634706)
These might as well be private. It would be good to completely hide this to/from index from the outside world because it can be misused in a dangerous way. Then provide a way to call a provided function for each element:
```cpp
void TrafficStats::ForEach(std::function<void(TrafficStats::Direction dir,
Network net,
ConnectionType con,
const std::string&
...
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415634706)
These might as well be private. It would be good to completely hide this to/from index from the outside world because it can be misused in a dangerous way. Then provide a way to call a provided function for each element:
```cpp
void TrafficStats::ForEach(std::function<void(TrafficStats::Direction dir,
Network net,
ConnectionType con,
const std::string&
...
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415625631)
This method is not needed. It is simpler and suffices to explicitly initialize the members of `MsgStat` to zero:
```cpp
std::atomic_uint64_t byte_count{0}; //!< Number of bytes transferred.
std::atomic_uint64_t msg_count{0}; //!< Number of messages transferred.
```
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415625631)
This method is not needed. It is simpler and suffices to explicitly initialize the members of `MsgStat` to zero:
```cpp
std::atomic_uint64_t byte_count{0}; //!< Number of bytes transferred.
std::atomic_uint64_t msg_count{0}; //!< Number of messages transferred.
```
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415601693)
Commit d3b34b82af33909eaeffe8d596a6c70951dc844c `net: move NET_MESSAGE_TYPE_OTHER` is not needed and can be dropped if `messageTypeFromIndex()` and `messageTypeToIndex()` standalone functions are made private methods in the `NetStats` class and their usage limited to within the class. That is desirable anyway because they are only relevant for an index in an array that is defined in a particular way and is (should be) private within `NetStats`.
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415601693)
Commit d3b34b82af33909eaeffe8d596a6c70951dc844c `net: move NET_MESSAGE_TYPE_OTHER` is not needed and can be dropped if `messageTypeFromIndex()` and `messageTypeToIndex()` standalone functions are made private methods in the `NetStats` class and their usage limited to within the class. That is desirable anyway because they are only relevant for an index in an array that is defined in a particular way and is (should be) private within `NetStats`.
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1417286331)
This shouldn't be necessary. It means that the previous test did not leave the environment as it was when it started. That is the newly added `test_getnetmsgstats`. It is good to be able to run the tests in isolation and that they do not rely on the leftovers from previous tests. For example if:
```
self.test_connection_count()
self.test_getpeerinfo()
self.test_getnettotals()
self.test_getnetmsgstats()
self.test_getnetworkinfo()
sel
...
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1417286331)
This shouldn't be necessary. It means that the previous test did not leave the environment as it was when it started. That is the newly added `test_getnetmsgstats`. It is good to be able to run the tests in isolation and that they do not rely on the leftovers from previous tests. For example if:
```
self.test_connection_count()
self.test_getpeerinfo()
self.test_getnettotals()
self.test_getnetmsgstats()
self.test_getnetworkinfo()
sel
...
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415682660)
The code already collects the traffic data for the per peer stats. To avoid confusion and ease maintenance it would be nice if the per peer and global traffic data is collected at the same place. This is where the data is collected in `master` for the per peer stats:
Received:
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/net.cpp#L663
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/net.cpp#L669-L674
Sent:
https:
...
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415682660)
The code already collects the traffic data for the per peer stats. To avoid confusion and ease maintenance it would be nice if the per peer and global traffic data is collected at the same place. This is where the data is collected in `master` for the per peer stats:
Received:
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/net.cpp#L663
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/net.cpp#L669-L674
Sent:
https:
...