💬 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.
👋 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`.