👍 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
...
✅ achow101 closed a pull request: "Ecai"
(https://github.com/bitcoin/bitcoin/pull/33868)
(https://github.com/bitcoin/bitcoin/pull/33868)
💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2521976766)
No, but only in the same sense that it's not possible to split an 8MWU template into two <= 4MWU blocks, so the fact that with that change you couldn't include 3x 2.6 MWU txns seems more of a feature than a bug.
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2521976766)
No, but only in the same sense that it's not possible to split an 8MWU template into two <= 4MWU blocks, so the fact that with that change you couldn't include 3x 2.6 MWU txns seems more of a feature than a bug.
👍 rkrux approved a pull request: "scripted-diff: fix leftover references to `policy/fees.h`"
(https://github.com/bitcoin/bitcoin/pull/33864#pullrequestreview-3457992495)
lgtm ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
(https://github.com/bitcoin/bitcoin/pull/33864#pullrequestreview-3457992495)
lgtm ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2522131622)
I don't think this change is right. The path can not be null or empty, because that will fail later on with:
```
unknown location(0): fatal error: in "fs_tests/fsbridge_stem": std::filesystem::__cxx11::filesystem_error: filesystem error: cannot make absolute path: Invalid argument []
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2522131622)
I don't think this change is right. The path can not be null or empty, because that will fail later on with:
```
unknown location(0): fatal error: in "fs_tests/fsbridge_stem": std::filesystem::__cxx11::filesystem_error: filesystem error: cannot make absolute path: Invalid argument []
💬 alexanderwiederin commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3526237850)
@stickies-v, what is the _real_ benefit over a hash-based equality check?
I wonder if we can avoid committing to a pointer-based check here given it's a non-trivial decision that is hard to reverse. The performance gain is negligible from what I understand.
What do you think?
(https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3526237850)
@stickies-v, what is the _real_ benefit over a hash-based equality check?
I wonder if we can avoid committing to a pointer-based check here given it's a non-trivial decision that is hard to reverse. The performance gain is negligible from what I understand.
What do you think?
💬 maflcko commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2522177400)
> How about keeping the checking, but testing it without going through the c++ wrapper?
Sure, that'll be my suggestion above: "Side-step the C++ wrapper and directly call the C code with a raw C-span from this test."
Anything should be fine, and the only problem I see with that would be that we won't be able to enable hardened C in the future. What can be achieved with `std::span` and library hardening in C++, can be achieved in C via `__counted_by` and `-fbounds-safety`.
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2522177400)
> How about keeping the checking, but testing it without going through the c++ wrapper?
Sure, that'll be my suggestion above: "Side-step the C++ wrapper and directly call the C code with a raw C-span from this test."
Anything should be fine, and the only problem I see with that would be that we won't be able to enable hardened C in the future. What can be achieved with `std::span` and library hardening in C++, can be achieved in C via `__counted_by` and `-fbounds-safety`.
📝 maflcko opened a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869)
Otherwise, compilation with GCC-15+ will warn about it:
```
src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```
Follow-up to https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482
Can be tested via `git archive --output=/tmp/a.tar HEAD`
(https://github.com/bitcoin/bitcoin/pull/33869)
Otherwise, compilation with GCC-15+ will warn about it:
```
src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```
Follow-up to https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482
Can be tested via `git archive --output=/tmp/a.tar HEAD`
💬 maflcko commented on pull request "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC":
(https://github.com/bitcoin/bitcoin/pull/32420#discussion_r2522490088)
/**\n+ * Whether to include and OP_0 as a dummy extraNonce in the template's coinbase\n+ */\n -> "and" -> "an" [Correct article: "an OP_0" (singular noun) makes the sentence grammatical and clear.]
(https://github.com/bitcoin/bitcoin/pull/32420#discussion_r2522490088)
/**\n+ * Whether to include and OP_0 as a dummy extraNonce in the template's coinbase\n+ */\n -> "and" -> "an" [Correct article: "an OP_0" (singular noun) makes the sentence grammatical and clear.]
💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2435927466)
llm nits:
assumetxo -> assumeutxo [spelling mistake in comments; inconsistent with the rest of the code/documentation]
assumetxo -> assumeutxo [same misspelling repeated elsewhere in the added enum comments]
contains database -> contains a database [missing article makes the sentence ungrammatical / harder to parse]
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2435927466)
llm nits:
assumetxo -> assumeutxo [spelling mistake in comments; inconsistent with the rest of the code/documentation]
assumetxo -> assumeutxo [same misspelling repeated elsewhere in the added enum comments]
contains database -> contains a database [missing article makes the sentence ungrammatical / harder to parse]