💬 theStack commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2051723308)
nit: `CheckSignatureEncoding` already checks for empty sig, so the first check isn't needed
```suggestion
if (!CheckSignatureEncoding(sig, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_STRICTENC, nullptr)) {
```
any reason why the verification flags don't also include `SCRIPT_VERIFY_LOW_S`?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2051723308)
nit: `CheckSignatureEncoding` already checks for empty sig, so the first check isn't needed
```suggestion
if (!CheckSignatureEncoding(sig, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_STRICTENC, nullptr)) {
```
any reason why the verification flags don't also include `SCRIPT_VERIFY_LOW_S`?
💬 furszy commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2817260424)
> On Windows, benchmark runs sporadically hung because the descriptor wallet and its companion watch-only wallet were never unloaded, remaining registered in the global WalletContext. This prevented the test harness from cleaning up its temporary directory.
Hmm, haven't dug deeper but this does not seem to be accurate. The `WalletContext` is not global. It is created at the beginning of the benchmark, just after creating the test context (which is the one that calls to the `fs::removal_all` t
...
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2817260424)
> On Windows, benchmark runs sporadically hung because the descriptor wallet and its companion watch-only wallet were never unloaded, remaining registered in the global WalletContext. This prevented the test harness from cleaning up its temporary directory.
Hmm, haven't dug deeper but this does not seem to be accurate. The `WalletContext` is not global. It is created at the beginning of the benchmark, just after creating the test context (which is the one that calls to the `fs::removal_all` t
...
📝 l0rinc opened a pull request: "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`"
(https://github.com/bitcoin/bitcoin/pull/32313)
Split out of https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813590511 since this needed more complicated production code changes.
The changes ensure `cachedCoinsUsage` remains balanced throughout all coin operations and that the sanitizers will catch future violations.
The change was tested with the related fuzz test, and asserted before/after each `cachedCoinsUsage` change (in production code and fuzz) that the calculations are still correct by recalculating it from scratch:
...
(https://github.com/bitcoin/bitcoin/pull/32313)
Split out of https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813590511 since this needed more complicated production code changes.
The changes ensure `cachedCoinsUsage` remains balanced throughout all coin operations and that the sanitizers will catch future violations.
The change was tested with the related fuzz test, and asserted before/after each `cachedCoinsUsage` change (in production code and fuzz) that the calculations are still correct by recalculating it from scratch:
...
💬 andrewtoth commented on pull request "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051775463)
I think this would be simpler if we kept the original behavior of moving the `coin` here as well, and then only increment the `cachedCoinsUsage` if the coin is inserted.
In practice it's not possible to get `!inserted` unless the assume utxo payload was generated incorrectly.
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051775463)
I think this would be simpler if we kept the original behavior of moving the `coin` here as well, and then only increment the `cachedCoinsUsage` if the coin is inserted.
In practice it's not possible to get `!inserted` unless the assume utxo payload was generated incorrectly.
💬 andrewtoth commented on pull request "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051774641)
Would it be simpler to just move this `if` block below the `if` block that throws, instead of pulling the throw out?
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051774641)
Would it be simpler to just move this `if` block below the `if` block that throws, instead of pulling the throw out?
💬 andrewtoth commented on pull request "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051775760)
The entry has an empty coin here, it was just created. So this is just subtracting zero.
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051775760)
The entry has an empty coin here, it was just created. So this is just subtracting zero.
💬 fjahr commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2817298481)
Code change looks good to me. The commit message doesn't seem to say anything about the typo fix though. It should be mentioned there and that the rest of the change is fixing a secondary issue caused by this. You could also split this in two separate commits.
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2817298481)
Code change looks good to me. The commit message doesn't seem to say anything about the typo fix though. It should be mentioned there and that the rest of the change is fixing a secondary issue caused by this. You could also split this in two separate commits.
💬 l0rinc commented on pull request "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051806640)
If you think that's better, I don't mind - done
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051806640)
If you think that's better, I don't mind - done
💬 l0rinc commented on pull request "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051806668)
> In practice it's not possible to get !inserted unless the assume utxo payload was generated incorrectly.
As far as I can see that's not a guarantee, but I've also noted it in the commit message that currently this is the case.
> kept the original behavior of moving the coin here as well
Sure, if you think that's better - changed!
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051806668)
> In practice it's not possible to get !inserted unless the assume utxo payload was generated incorrectly.
As far as I can see that's not a guarantee, but I've also noted it in the commit message that currently this is the case.
> kept the original behavior of moving the coin here as well
Sure, if you think that's better - changed!
💬 l0rinc commented on pull request "RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051806682)
Thanks, I'll add an assert in that case to explain why this branch wasn't symmetric with the other one.
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2051806682)
Thanks, I'll add an assert in that case to explain why this branch wasn't symmetric with the other one.
👋 l0rinc's pull request is ready for review: "coins: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`"
(https://github.com/bitcoin/bitcoin/pull/32313)
(https://github.com/bitcoin/bitcoin/pull/32313)
⚠️ Dustin4444 opened an issue: "new"
(https://github.com/bitcoin/bitcoin/issues/32314)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
> [!IMPORTANT]
>
(https://github.com/bitcoin/bitcoin/issues/32314)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
> [!IMPORTANT]
>
✅ pinheadmz closed an issue: "new"
(https://github.com/bitcoin/bitcoin/issues/32314)
(https://github.com/bitcoin/bitcoin/issues/32314)
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051989121)
nit: `weekday` and `month` can be a `string_view`.
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051989121)
nit: `weekday` and `month` can be a `string_view`.
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051985554)
nit: this comment seems to be incorrect, since `TCP_NODELAY` is only set once by `SockMan`.
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051985554)
nit: this comment seems to be incorrect, since `TCP_NODELAY` is only set once by `SockMan`.
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052027205)
Can we return a `string_view` to the internal buffer?
(preventing allocation & copy)
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052027205)
Can we return a `string_view` to the internal buffer?
(preventing allocation & copy)
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052025637)
Can we return a `string_view` to the internal buffer?
(preventing allocation & copy)
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052025637)
Can we return a `string_view` to the internal buffer?
(preventing allocation & copy)
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052024440)
Maybe it would be better (performance-wise) to create the string after the loop is over?
```c++
std::optional<std::string> LineReader::ReadLine()
{
if (it == end) {
return std::nullopt;
}
auto line_start = it;
size_t count = 0;
while (it != end) {
char c = static_cast<char>(*it);
++it;
++count;
if (c == '\n') break;
if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
}
co
...
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052024440)
Maybe it would be better (performance-wise) to create the string after the loop is over?
```c++
std::optional<std::string> LineReader::ReadLine()
{
if (it == end) {
return std::nullopt;
}
auto line_start = it;
size_t count = 0;
while (it != end) {
char c = static_cast<char>(*it);
++it;
++count;
if (c == '\n') break;
if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
}
co
...
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051998964)
nit: maybe using `std::span` will be simpler?
For example:
```c++
auto span = std::as_bytes(std::span(str));
return {span.begin(), span.end()};
```
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2051998964)
nit: maybe using `std::span` will be simpler?
For example:
```c++
auto span = std::as_bytes(std::span(str));
return {span.begin(), span.end()};
```
👍 rkrux approved a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2780807756)
reACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
```
git range-diff fa16051...fa86190
```
New changes: Updating the commit message, the release docs, functional test name & logs, and unit test comment.
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2780807756)
reACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
```
git range-diff fa16051...fa86190
```
New changes: Updating the commit message, the release docs, functional test name & logs, and unit test comment.