💬 dergoegge commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642944794)
Something we could also do is stop using libfuzzer's instrumentation in that task, as it makes things slower too. We aren't using instrumentation in the other fuzz jobs either.
We could still compile with libfuzzer but disable the instrumentation with `-fsanitize-coverage-allowlist=allowlist.txt` where the allow list is empty (i think).
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642944794)
Something we could also do is stop using libfuzzer's instrumentation in that task, as it makes things slower too. We aren't using instrumentation in the other fuzz jobs either.
We could still compile with libfuzzer but disable the instrumentation with `-fsanitize-coverage-allowlist=allowlist.txt` where the allow list is empty (i think).
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946537723)
That would make `addr_to` a pointer, so `*addr_to` would have to be used in the 6 places below. I slightly prefer the current "if holds alternative CService" because it is dump and easy to follow. Leaving it as it is.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946537723)
That would make `addr_to` a pointer, so `*addr_to` would have to be used in the 6 places below. I slightly prefer the current "if holds alternative CService" because it is dump and easy to follow. Leaving it as it is.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946544583)
Yeah, `EventNewConnectionAccepted()` is generic used also for non-I2P connections and the flip will not do anything on I2P addresses.
> But maybe you should add a Assume(IsI2P(conn.peer));
In which function?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946544583)
Yeah, `EventNewConnectionAccepted()` is generic used also for non-I2P connections and the flip will not do anything on I2P addresses.
> But maybe you should add a Assume(IsI2P(conn.peer));
In which function?
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946547841)
Removed!
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946547841)
Removed!
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946550609)
Done.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946550609)
Done.
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946557003)
I think in ThreadI2PAccept() right before it calls `NewSockAccepted()`
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946557003)
I think in ThreadI2PAccept() right before it calls `NewSockAccepted()`
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946563897)
Done.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946563897)
Done.
⚠️ giahuy98 opened an issue: "GetRandBytes() Hangs on Samsung Galaxy S25 and OnePlus 13"
(https://github.com/bitcoin/bitcoin/issues/31817)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
We have been using Bitcoin Core natively in the Nunchuk Bitcoin wallet, available on both desktop and mobile. On Samsung Galaxy S25 and OnePlus 13 (both running Android 15 with the SM8750-AB Snapdragon 8 Elite 3nm processor), the app fails to initialize due to an issue in `GetRandBytes()`.
The problem occurs when `GetRandBytes()` calls `GetRNDRRS()`, resulting in an infinite loop without
...
(https://github.com/bitcoin/bitcoin/issues/31817)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
We have been using Bitcoin Core natively in the Nunchuk Bitcoin wallet, available on both desktop and mobile. On Samsung Galaxy S25 and OnePlus 13 (both running Android 15 with the SM8750-AB Snapdragon 8 Elite 3nm processor), the app fails to initialize due to an issue in `GetRandBytes()`.
The problem occurs when `GetRandBytes()` calls `GetRNDRRS()`, resulting in an infinite loop without
...
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2643024263)
I implemented both the BIP155 NetworkID approach (https://github.com/0xB10C/bitcoin/commit/72133f356f359db818bc9eb2194fdbfa783a5d2c) and the Network as string approach from https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1555936795 (https://github.com/0xB10C/bitcoin/commit/39102d4ae166e34ee4b76581c1431184c486d8d9).
The BIP155 approach is a bit more involved with making `GetBIP155Network()` public, introducing a `ConnectedThroughBIP155Network()` helper, and passing `0` (or somethin
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2643024263)
I implemented both the BIP155 NetworkID approach (https://github.com/0xB10C/bitcoin/commit/72133f356f359db818bc9eb2194fdbfa783a5d2c) and the Network as string approach from https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1555936795 (https://github.com/0xB10C/bitcoin/commit/39102d4ae166e34ee4b76581c1431184c486d8d9).
The BIP155 approach is a bit more involved with making `GetBIP155Network()` public, introducing a `ConnectedThroughBIP155Network()` helper, and passing `0` (or somethin
...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946583867)
Hmm, is not straight forward. From all callers of `MarkAsDisconnectAndCloseConnection()`:
* 3 unconditional `LogDebug(BCLog::NET`
* 1 no log
* 2 conditional `LogDebug(BCLog::NET` based on `fDisconnect`
So it is not like all the callers do the same thing. Maybe it is ok to consolidate those into one behavior, but would be a functional change that is not the purpose of this PR. Leaving it as it is, but sounds like something to explore as a followup.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946583867)
Hmm, is not straight forward. From all callers of `MarkAsDisconnectAndCloseConnection()`:
* 3 unconditional `LogDebug(BCLog::NET`
* 1 no log
* 2 conditional `LogDebug(BCLog::NET` based on `fDisconnect`
So it is not like all the callers do the same thing. Maybe it is ok to consolidate those into one behavior, but would be a functional change that is not the purpose of this PR. Leaving it as it is, but sounds like something to explore as a followup.
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2643056228)
I included the commit from #31815 here so it gets more runs.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2643056228)
I included the commit from #31815 here so it gets more runs.
💬 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.