💬 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:
...
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1417282448)
I realized that this can be done in a simpler way - if the stats are put in a map and `""` used for the aggregated dimensions. See commit b7c56affe9 `rpc: make it possible to aggregate the result in getnetmsgstats` from [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats).
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1417282448)
I realized that this can be done in a simpler way - if the stats are put in a map and `""` used for the aggregated dimensions. See commit b7c56affe9 `rpc: make it possible to aggregate the result in getnetmsgstats` from [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats).
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1842914664)
[clusterfuzz-testcase-bitdeque-5480558045822976.bin.txt](https://github.com/bitcoin/bitcoin/files/13583557/clusterfuzz-testcase-bitdeque-5480558045822976.bin.txt)
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1842914664)
[clusterfuzz-testcase-bitdeque-5480558045822976.bin.txt](https://github.com/bitcoin/bitcoin/files/13583557/clusterfuzz-testcase-bitdeque-5480558045822976.bin.txt)
📝 dergoegge opened a pull request: "wip: Split fuzz binary"
(https://github.com/bitcoin/bitcoin/pull/29010)
Closes #28971
* Split each harness into it's own file
* Support compiling individual harness through `CPPFLAGS="-DFUZZ_HARNESS=<harness name>"`
* Build individual binaries
The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).
TODOs:
- [ ] Introduce option for building individual binaries
- [ ] Deal with `test/fuzz/{tx_pool, tx_package_eval, deserialize}.cpp`
- [ ] One harness per
...
(https://github.com/bitcoin/bitcoin/pull/29010)
Closes #28971
* Split each harness into it's own file
* Support compiling individual harness through `CPPFLAGS="-DFUZZ_HARNESS=<harness name>"`
* Build individual binaries
The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).
TODOs:
- [ ] Introduce option for building individual binaries
- [ ] Deal with `test/fuzz/{tx_pool, tx_package_eval, deserialize}.cpp`
- [ ] One harness per
...
💬 dergoegge commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1842946104)
cc @maflcko
turns out we can actually keep the map if we just don't add all the harness functions into it
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1842946104)
cc @maflcko
turns out we can actually keep the map if we just don't add all the harness functions into it
💬 dergoegge commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1842958846)
Should we do the same for `process_message`? would need to change it to use `ProcessMessagesOnce` as well.
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1842958846)
Should we do the same for `process_message`? would need to change it to use `ProcessMessagesOnce` as well.