💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946401139)
In 162d0916774c719f1e3749b0d4d204e296ee4ac8 "Add PSBT::GetUniqueID": I think this should check the version number, and then `Assume(version == 0 || tx == std::nullopt)`
The way it's written now suggests `tx` is just a cache that might be stale.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946401139)
In 162d0916774c719f1e3749b0d4d204e296ee4ac8 "Add PSBT::GetUniqueID": I think this should check the version number, and then `Assume(version == 0 || tx == std::nullopt)`
The way it's written now suggests `tx` is just a cache that might be stale.
💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946413388)
In aeb9e44bc5a221553d2010752c76f8f16205551c "Change PSBT::AddInput to take just PSBTInput": I think it's more readable to just check the PSBT version directly.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946413388)
In aeb9e44bc5a221553d2010752c76f8f16205551c "Change PSBT::AddInput to take just PSBTInput": I think it's more readable to just check the PSBT version directly.
💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946386141)
In cef9de00f4c2d4c9c033e7dce2b7ffac2e694a26 "Call CacheUnsignedTxPieces in PSBT constructor": might as well do this in the previous commit. Ditto for the next commit.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946386141)
In cef9de00f4c2d4c9c033e7dce2b7ffac2e694a26 "Call CacheUnsignedTxPieces in PSBT constructor": might as well do this in the previous commit. Ditto for the next commit.
⚠️ jsarenik opened an issue: "Memory-only wallet (for faucets)"
(https://github.com/bitcoin/bitcoin/issues/31816)
### Please describe the feature you'd like to see added.
Currently I do following regularly every couple of days in order to refresh faucet wallets and reclaim megabytes of disk space:
```sh
cd ~/.bitcoin/signet/wallets
bitcoin-cli -signet createwallet name false true
# blank with private-keys enabled
cd name
dt=../listdescriptors-true.txt
A=$(sed 's/"timestamp".*$/"timestamp":"now",/' $dt | jq -rc .descriptors)
bitcoin-cli importdescriptors $A
```
It works well for [me](https://alt.signetfau
...
(https://github.com/bitcoin/bitcoin/issues/31816)
### Please describe the feature you'd like to see added.
Currently I do following regularly every couple of days in order to refresh faucet wallets and reclaim megabytes of disk space:
```sh
cd ~/.bitcoin/signet/wallets
bitcoin-cli -signet createwallet name false true
# blank with private-keys enabled
cd name
dt=../listdescriptors-true.txt
A=$(sed 's/"timestamp".*$/"timestamp":"now",/' $dt | jq -rc .descriptors)
bitcoin-cli importdescriptors $A
```
It works well for [me](https://alt.signetfau
...
💬 maflcko commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642789552)
Interesting. For context, the "speedup" after Jan 12 is likely due to fuzz input invalidation after commit 37af8bfb34d685bfd5d9a9ba6b35b4705e021535. And the "slowdown" after Dec 13th is likely due to adding fuzz inputs after commit https://github.com/bitcoin-core/qa-assets/commit/5c026e2a97b5f447a863734fb1c2566c33738d01.
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642789552)
Interesting. For context, the "speedup" after Jan 12 is likely due to fuzz input invalidation after commit 37af8bfb34d685bfd5d9a9ba6b35b4705e021535. And the "slowdown" after Dec 13th is likely due to adding fuzz inputs after commit https://github.com/bitcoin-core/qa-assets/commit/5c026e2a97b5f447a863734fb1c2566c33738d01.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946505928)
Maybe it is worth doing this: https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952. It is a super good change and normally I would do it, but the reason I didn't is that it will increase the size of this PR which, I am afraid, would turn reviewers away.
I will proceed to other suggestions and give this some thought...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946505928)
Maybe it is worth doing this: https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952. It is a super good change and normally I would do it, but the reason I didn't is that it will increase the size of this PR which, I am afraid, would turn reviewers away.
I will proceed to other suggestions and give this some thought...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946520524)
Done, but used `uint8_t` because `ReceiveMsgBytes()` (in `master`) takes a span of that and conversion is not possible.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946520524)
Done, but used `uint8_t` because `ReceiveMsgBytes()` (in `master`) takes a span of that and conversion is not possible.
💬 maflcko commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946527624)
> conversion is not possible.
It should be possible to convert std::byte* to uint8_t* (and vice-versa). The two are the almost the same anyway (https://en.cppreference.com/w/cpp/types/byte). And a span is just a pointer+size, so conversion between the two span types should also be possible.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946527624)
> conversion is not possible.
It should be possible to convert std::byte* to uint8_t* (and vice-versa). The two are the almost the same anyway (https://en.cppreference.com/w/cpp/types/byte). And a span is just a pointer+size, so conversion between the two span types should also be possible.
💬 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.