💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946597696)
Hmm, if we are worried that `recv(2)` will return a value less than `-1`, then we would better treat that as an error instead of terminating the whole program. Changed the `switch` to `if/else`, treating all negative values as an error.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946597696)
Hmm, if we are worried that `recv(2)` will return a value less than `-1`, then we would better treat that as an error instead of terminating the whole program. Changed the `switch` to `if/else`, treating all negative values as an error.
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2643062015)
Unfortunately CI won't run on a PR that needs rebase, and this one very non-trivial, so I'll wait for the next #31741 rebase.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2643062015)
Unfortunately CI won't run on a PR that needs rebase, and this one very non-trivial, so I'll wait for the next #31741 rebase.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946649911)
Because later on in `ConnectAndMakeId()` there is logic that will be confused if the default constructed proxy is valid. But it is better to use a `std::optional` to designate "no proxy" instead of default-contructed-and-invalid proxy. Then this assert/Assume is not necessary. Changed.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946649911)
Because later on in `ConnectAndMakeId()` there is logic that will be confused if the default constructed proxy is valid. But it is better to use a `std::optional` to designate "no proxy" instead of default-contructed-and-invalid proxy. Then this assert/Assume is not necessary. Changed.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946658211)
But there is still some common code that would have to be duplicated:
```cpp
{
AssertLockNotHeld(m_connected_mutex);
AssertLockNotHeld(m_unused_i2p_sessions_mutex);
std::unique_ptr<Sock> sock;
std::unique_ptr<i2p::sam::Session> i2p_transient_session;
Assume(!me.IsValid());
if (std::holds_alternative<CService>(to)) {
...
} else {
...
}
if (!sock) {
return std::nullopt;
}
if (!me.IsValid()) {
m
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946658211)
But there is still some common code that would have to be duplicated:
```cpp
{
AssertLockNotHeld(m_connected_mutex);
AssertLockNotHeld(m_unused_i2p_sessions_mutex);
std::unique_ptr<Sock> sock;
std::unique_ptr<i2p::sam::Session> i2p_transient_session;
Assume(!me.IsValid());
if (std::holds_alternative<CService>(to)) {
...
} else {
...
}
if (!sock) {
return std::nullopt;
}
if (!me.IsValid()) {
m
...
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946672405)
Everything after `if (!sock)` could go into a helper function? FinishConnectStuff()
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946672405)
Everything after `if (!sock)` could go into a helper function? FinishConnectStuff()
💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946684778)
(Could still make the `SockMan` destructor `protected` and non-`virtual` if the `CConnman` destructor was made `virtual`, but there is little to gain from it).
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946684778)
(Could still make the `SockMan` destructor `protected` and non-`virtual` if the `CConnman` destructor was made `virtual`, but there is little to gain from it).
💬 yancyribbens commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1946720144)
At first glance this is confusing. It looks like `quot_abs` is being computed differently depending on if `num_high` is negative or not, although it's not apparent why right away. Maybe I will take these fuzz tests for a spin and look at some example values. As a side note, it's interesting that fuzz tests are used to test for program correctness where I thought the main objective for fuzz tests was to find crashes and panics.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1946720144)
At first glance this is confusing. It looks like `quot_abs` is being computed differently depending on if `num_high` is negative or not, although it's not apparent why right away. Maybe I will take these fuzz tests for a spin and look at some example values. As a side note, it's interesting that fuzz tests are used to test for program correctness where I thought the main objective for fuzz tests was to find crashes and panics.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946733390)
I just copied this from `master`:
```cpp
static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50;
```
I guess some empirical testing... This constant is used in two places: in `WaitMany()` and in the sleep if no sockets at all, maybe those two should be separate constants (btw the same is in `master`).
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946733390)
I just copied this from `master`:
```cpp
static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50;
```
I guess some empirical testing... This constant is used in two places: in `WaitMany()` and in the sleep if no sockets at all, maybe those two should be separate constants (btw the same is in `master`).
🤔 marcofleon reviewed a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602102914)
Code review ACK 2a279acd89fa37fec94c66dde7408a3a9627f49a
Confirmed that there's no behavior change. This PR is adding tracking of total tx weight in the orphan map, total announcements, and the total weight of announced orphans for each peer. I ran the updated `txorphan` and `txdownloadman_impl` fuzz tests for ~120 cpu hours each.
Only nit is that `CheckInvariants()` should be outside of the loop in the `txdownloadman_impl` harness.
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602102914)
Code review ACK 2a279acd89fa37fec94c66dde7408a3a9627f49a
Confirmed that there's no behavior change. This PR is adding tracking of total tx weight in the orphan map, total announcements, and the total weight of announced orphans for each peer. I ran the updated `txorphan` and `txdownloadman_impl` fuzz tests for ~120 cpu hours each.
Only nit is that `CheckInvariants()` should be outside of the loop in the `txdownloadman_impl` harness.
👋 hebasto's pull request is ready for review: "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows"
(https://github.com/bitcoin/bitcoin/pull/31158)
(https://github.com/bitcoin/bitcoin/pull/31158)
📝 fanquake opened a pull request: "guix: remove test-security/symbol-check scripts"
(https://github.com/bitcoin/bitcoin/pull/31818)
These scripts are becoming more of nuisance, than a value-add; particularly since we've been building releases using Guix. Adding new (release bin) tests can be harder, because it requires constructing a failing test, which is becoming less easy, e.g trying to disable a feature or protection that has been built into the compiler/toolchain by default.
In the pre-Guix days, these were valuable to sanity-check the environment, because we were pulling that pre-built from Ubuntu, with little contr
...
(https://github.com/bitcoin/bitcoin/pull/31818)
These scripts are becoming more of nuisance, than a value-add; particularly since we've been building releases using Guix. Adding new (release bin) tests can be harder, because it requires constructing a failing test, which is becoming less easy, e.g trying to disable a feature or protection that has been built into the compiler/toolchain by default.
In the pre-Guix days, these were valuable to sanity-check the environment, because we were pulling that pre-built from Ubuntu, with little contr
...
💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2643328203)
@theuni
> Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).
Thank you for your insight!
This PR has been reworked based on your branch. The `inline` variables are avoided.
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2643328203)
@theuni
> Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).
Thank you for your insight!
This PR has been reworked based on your branch. The `inline` variables are avoided.
💬 fanquake commented on pull request "build: CMake security checks workarounds":
(https://github.com/bitcoin/bitcoin/pull/31715#issuecomment-2643328234)
> and I will look at removing them entirely.
Opened a removal in #31818.
(https://github.com/bitcoin/bitcoin/pull/31715#issuecomment-2643328234)
> and I will look at removing them entirely.
Opened a removal in #31818.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946776269)
> if WaitMany() returns false, we would have already waited
No, `false` means an error, e.g. `poll(2)` returning `-1`. A timeout is signaled by a return value of `true` and all `what[].occurred` returned as `0`.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946776269)
> if WaitMany() returns false, we would have already waited
No, `false` means an error, e.g. `poll(2)` returning `-1`. A timeout is signaled by a return value of `true` and all `what[].occurred` returned as `0`.
📝 fanquake opened a pull request: "doc: swap CPPFLAGS for APPEND_CPPFLAGS"
(https://github.com/bitcoin/bitcoin/pull/31819)
`APPEND_CPPFLAGS` will be understood by our CMake, whereas `CPPFLAGS` will not. Attempting what is currently documented will just give:
```bash
CMake Warning:
Ignoring extra path from command line:
"CPPFLAGS=-DDEBUG_LOCKCONTENTION"
```
(https://github.com/bitcoin/bitcoin/pull/31819)
`APPEND_CPPFLAGS` will be understood by our CMake, whereas `CPPFLAGS` will not. Attempting what is currently documented will just give:
```bash
CMake Warning:
Ignoring extra path from command line:
"CPPFLAGS=-DDEBUG_LOCKCONTENTION"
```
👍 hebasto approved a pull request: "doc: swap CPPFLAGS for APPEND_CPPFLAGS"
(https://github.com/bitcoin/bitcoin/pull/31819#pullrequestreview-2602176228)
ACK ea687d202934ee9aa26912cda21993da219cd418.
(https://github.com/bitcoin/bitcoin/pull/31819#pullrequestreview-2602176228)
ACK ea687d202934ee9aa26912cda21993da219cd418.
📝 fanquake opened a pull request: "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in"
(https://github.com/bitcoin/bitcoin/pull/31820)
Follows up from when the `pc.in` was added.
(https://github.com/bitcoin/bitcoin/pull/31820)
Follows up from when the `pc.in` was added.
🚀 glozow merged a pull request: "test: test_inv_block, use mocktime instead of waiting"
(https://github.com/bitcoin/bitcoin/pull/31811)
(https://github.com/bitcoin/bitcoin/pull/31811)
💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2643498506)
Thank you @hebasto! We had a pretty long discussion about this. I think that hebasto's approach is probably fine and would never be an issue in the real world. I'll admit that I'm being overly paranoid and probably wrong about the guarantees that linkers provide...
But still, I'm not alone in my paranoia. For example, Google has [banned non-constexpr inline header variables](https://groups.google.com/a/chromium.org/g/cxx/c/hmyGFD80ocE/m/qVe-hqAVDAAJ) for Chromium out of similar concerns.
[
...
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2643498506)
Thank you @hebasto! We had a pretty long discussion about this. I think that hebasto's approach is probably fine and would never be an issue in the real world. I'll admit that I'm being overly paranoid and probably wrong about the guarantees that linkers provide...
But still, I'm not alone in my paranoia. For example, Google has [banned non-constexpr inline header variables](https://groups.google.com/a/chromium.org/g/cxx/c/hmyGFD80ocE/m/qVe-hqAVDAAJ) for Chromium out of similar concerns.
[
...
💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946784827)
I needed to change this to an arithmetic comparison to make it work because the `wc -l` result includes some white space on my system.
``` suggestion
if (( $(get_file_suffix_count gcda) != 0 )); then
```
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946784827)
I needed to change this to an arithmetic comparison to make it work because the `wc -l` result includes some white space on my system.
``` suggestion
if (( $(get_file_suffix_count gcda) != 0 )); then
```