💬 pablomartin4btc commented on pull request "Added test coverage for qt gui#901 console history filter":
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2520038416)
so we don't have to update it every time...
```suggestion
// Copyright (c) 2016-present The Bitcoin Core developers
```
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2520038416)
so we don't have to update it every time...
```suggestion
// Copyright (c) 2016-present The Bitcoin Core developers
```
📝 stickies-v opened a pull request: "kernel: allow null data_directory"
(https://github.com/bitcoin/bitcoin/pull/33867)
An empty path may be represented with a `nullptr`. For example, `std::string_view::data()` may return nullptr.
Removes the `BITCOINKERNEL_ARG_NONNULL` attribute for data_directory, and instead handles such null arguments in the implementation.
Also documents how `BITCOINKERNEL_ARG_NONNULL` should be used.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3454620265
(https://github.com/bitcoin/bitcoin/pull/33867)
An empty path may be represented with a `nullptr`. For example, `std::string_view::data()` may return nullptr.
Removes the `BITCOINKERNEL_ARG_NONNULL` attribute for data_directory, and instead handles such null arguments in the implementation.
Also documents how `BITCOINKERNEL_ARG_NONNULL` should be used.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3454620265
💬 waketraindev commented on pull request "Added test coverage for qt gui#901 console history filter":
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2520040168)
Ah will do, didn't think you meant present literally
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2520040168)
Ah will do, didn't think you meant present literally
💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2520053036)
I thought of eliminating the worst case here (when `pa == pb`, where it has to do the comparison for every byte) with something like:
```C++
return pa != pb &&
std::tie(pa->nChainWork, pb->nSequenceId, pb)
< std::tie(pb->nChainWork, pa->nSequenceId, pa);
```
But the benchmarks indicate that it's not really faster.
<details><summary>benchmark with pointer duplicates</summary>
```C++
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software lice
...
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2520053036)
I thought of eliminating the worst case here (when `pa == pb`, where it has to do the comparison for every byte) with something like:
```C++
return pa != pb &&
std::tie(pa->nChainWork, pb->nSequenceId, pb)
< std::tie(pb->nChainWork, pa->nSequenceId, pa);
```
But the benchmarks indicate that it's not really faster.
<details><summary>benchmark with pointer duplicates</summary>
```C++
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software lice
...
💬 Eunovo commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3524231657)
> 4\. try to connect that block, which will result in the entire chain up to that block being connected by the `msghand` thread. This could block the thread hours, in which no other peer would be processed, so existing peers will get disconnected (ping timeout), while net thread will still continually make new connections (that never complete the version handshake because we are too busy connecting blocks).
The easiest solution to this seems to be to use the ThreadPool from [33689](https://gi
...
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3524231657)
> 4\. try to connect that block, which will result in the entire chain up to that block being connected by the `msghand` thread. This could block the thread hours, in which no other peer would be processed, so existing peers will get disconnected (ping timeout), while net thread will still continually make new connections (that never complete the version handshake because we are too busy connecting blocks).
The easiest solution to this seems to be to use the ThreadPool from [33689](https://gi
...
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520079581)
does this apply to `Flush` as well?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520079581)
does this apply to `Flush` as well?
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520070772)
is this still the right abstraction?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520070772)
is this still the right abstraction?
💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2520101987)
Could you please address them, now that you had to [rebase](https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3510573430) anyway?
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2520101987)
Could you please address them, now that you had to [rebase](https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3510573430) anyway?
💬 hebasto commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2520213223)
I cannot find this change upstream, but it is correct. Otherwise, the `xcb_syslibs` test fails with the following errors:
```
xcb_auth.c:(.text+0x81): undefined reference to `XauGetBestAuthByAddr'
...
xcb_auth.c:(.text+0x292): undefined reference to `XauDisposeAuth'
```
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2520213223)
I cannot find this change upstream, but it is correct. Otherwise, the `xcb_syslibs` test fails with the following errors:
```
xcb_auth.c:(.text+0x81): undefined reference to `XauGetBestAuthByAddr'
...
xcb_auth.c:(.text+0x292): undefined reference to `XauDisposeAuth'
```
👍 hebasto approved a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3456081353)
ACK 79d5f120f4d10b681d3cebad1fcabff3c6f0c8fc modulo a couple of nit above.
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3456081353)
ACK 79d5f120f4d10b681d3cebad1fcabff3c6f0c8fc modulo a couple of nit above.
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2520540275)
I had thought it was still needed, but on closer analysis you're correct, thanks for catching that. Guix still passes `-DCMAKE_SKIP_RPATH=TRUE`, so the post-install security and symbol checks remain on the installed binaries without hard-coding it in the top-level CMake file, and NetBSD keeps its RPATH behavior. I've amended the commit.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2520540275)
I had thought it was still needed, but on closer analysis you're correct, thanks for catching that. Guix still passes `-DCMAKE_SKIP_RPATH=TRUE`, so the post-install security and symbol checks remain on the installed binaries without hard-coding it in the top-level CMake file, and NetBSD keeps its RPATH behavior. I've amended the commit.
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2520732338)
This behavior will conflict with https://github.com/bitcoin/bitcoin/pull/33866 then right? It will abort during tests if `Sync` is called with a raw `CCoinsView`. Should we throw instead and then we'll be able to catch it? Then whichever is merged second can add the catch condition?
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2520732338)
This behavior will conflict with https://github.com/bitcoin/bitcoin/pull/33866 then right? It will abort during tests if `Sync` is called with a raw `CCoinsView`. Should we throw instead and then we'll be able to catch it? Then whichever is merged second can add the catch condition?
💬 andrewtoth commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520746299)
Hmm should we possibly just iterate through the cursor, clearing it? That would make our test code simpler.
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520746299)
Hmm should we possibly just iterate through the cursor, clearing it? That would make our test code simpler.
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3524763254)
> I do think it's worthwhile to unify the format for inputting and and outputting feerates. And I agree it should be configurable, given there are multiple preferred formats.
Yes the idea is to be able to use sat/vB or BTC/kvB in inputting and outputting. This PR is just a PoC on how we could do it for outputting.
> My concern is the idea that the default format might be changed in the future
I don't think the default will ever be changed. I left the door open to it in some comments, b
...
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3524763254)
> I do think it's worthwhile to unify the format for inputting and and outputting feerates. And I agree it should be configurable, given there are multiple preferred formats.
Yes the idea is to be able to use sat/vB or BTC/kvB in inputting and outputting. This PR is just a PoC on how we could do it for outputting.
> My concern is the idea that the default format might be changed in the future
I don't think the default will ever be changed. I left the door open to it in some comments, b
...
💬 ajtowns commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3525328951)
> <vasild> #33565 has no NACKs
To be explicit: NACK. Reviewing bitcoin PRs well is hard, we should not be increasing that burden for, at best, marginal benefits to the author of the PR. For this particular PR, bikeshedding about the names of functions is not a good use of anyone's time here. Further, small PRs that seem to just be a mild refactoring with no (expected) functional effect have repeatedly resulted in meaningful changes; ones that come to mind currently include:
* removing the
...
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3525328951)
> <vasild> #33565 has no NACKs
To be explicit: NACK. Reviewing bitcoin PRs well is hard, we should not be increasing that burden for, at best, marginal benefits to the author of the PR. For this particular PR, bikeshedding about the names of functions is not a good use of anyone's time here. Further, small PRs that seem to just be a mild refactoring with no (expected) functional effect have repeatedly resulted in meaningful changes; ones that come to mind currently include:
* removing the
...
🤔 marcofleon reviewed a pull request: "fuzz: wallet: add target for `MigrateToDescriptor`"
(https://github.com/bitcoin/bitcoin/pull/32624#pullrequestreview-3457317631)
Left some questions below.
Also, [coverage report](https://marcofleon.github.io/coverage/spkm_migration/) after about a day of fuzzing.
(https://github.com/bitcoin/bitcoin/pull/32624#pullrequestreview-3457317631)
Left some questions below.
Also, [coverage report](https://marcofleon.github.io/coverage/spkm_migration/) after about a day of fuzzing.
💬 marcofleon commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521400460)
Adding this and modifying `added_chains` based on it does fix the crash I was seeing before. But shouldn't we save this variable before doing the migration? To make the assertion below actually verify the correctness.
I haven't looked at the migration code in depth, so this might be fine. Just figured if we're checking `added_size` against the result then it's better if it's based on the pre-migration state.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521400460)
Adding this and modifying `added_chains` based on it does fix the crash I was seeing before. But shouldn't we save this variable before doing the migration? To make the assertion below actually verify the correctness.
I haven't looked at the migration code in depth, so this might be fine. Just figured if we're checking `added_size` against the result then it's better if it's based on the pre-migration state.
💬 marcofleon commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521415641)
No null check after this? Doesn't seem to ever return null based on the coverage report, but could still be worth having to avoid potential undefined behavior.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521415641)
No null check after this? Doesn't seem to ever return null based on the coverage report, but could still be worth having to avoid potential undefined behavior.
💬 marcofleon commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521443581)
Is it worth testing other script types here as well? Not super familiar with the wallet code, so disregard if irrelevant.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521443581)
Is it worth testing other script types here as well? Not super familiar with the wallet code, so disregard if irrelevant.
📝 asyncmind0 opened a pull request: "Ecai"
(https://github.com/bitcoin/bitcoin/pull/33868)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33868)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...