🤔 BrandonOdiwuor reviewed a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871591979)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871591979)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49
💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2913060959)
@pinheadmz, that's the first thing I though of, but unfortunately the header is as mutable as it gets (see e.g. `while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) ++block.nNonce;`), so it would be dangerous to cache the hash, given that all fields are accessible from the outside and there's no simple way to invalidate the pre-computed hash, unless we expose them all to getter/setters which would invalidate the hash on change (and which would slow down `nNonce` expl
...
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2913060959)
@pinheadmz, that's the first thing I though of, but unfortunately the header is as mutable as it gets (see e.g. `while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) ++block.nNonce;`), so it would be dangerous to cache the hash, given that all fields are accessible from the outside and there's no simple way to invalidate the pre-computed hash, unless we expose them all to getter/setters which would invalidate the hash on change (and which would slow down `nNonce` expl
...
💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2109545287)
I didn't want to make it that verbose, people can check the source code if they see a failure here :)
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2109545287)
I didn't want to make it that verbose, people can check the source code if they see a failure here :)
💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913073569)
@andrewtoth, if you've closed it on purpose, do you want to explain the reason? Is it because you agree with @maflcko that making it a built-in functionality might be simpler? I'm also leaning in that direction, especially after https://github.com/bitcoin/bitcoin/pull/31144, where obfuscation should become a lot cheaper.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913073569)
@andrewtoth, if you've closed it on purpose, do you want to explain the reason? Is it because you agree with @maflcko that making it a built-in functionality might be simpler? I'm also leaning in that direction, especially after https://github.com/bitcoin/bitcoin/pull/31144, where obfuscation should become a lot cheaper.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913209698)
A bigger reason to do this in bitcoind is that it may be able to run in the background and lead to a better user experience.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913209698)
A bigger reason to do this in bitcoind is that it may be able to run in the background and lead to a better user experience.
💬 luke-jr commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2913210853)
Is this actually intended behaviour?
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2913210853)
Is this actually intended behaviour?
🤔 hebasto reviewed a pull request: "deps: Bump lief to 0.16.5"
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2871819728)
> > I think it'd be worth investigating any issues with 0.16.5, rather than deferring.
>
> Sounds good, marking the PR as draft again while I investigate.
>
> I've bisected to this commit in `lief`: [lief-project/LIEF@f23ced2](https://github.com/lief-project/LIEF/commit/f23ced2f4ffc170d0a6f40ff4a1bee575e3447cf)
>
> Seems related to scikit-build 0.10 [changing](https://iscinumpy.dev/post/scikit-build-core-0-10/#other-changes) `cmake.targets` to `build.targets`, and defining `build.target
...
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2871819728)
> > I think it'd be worth investigating any issues with 0.16.5, rather than deferring.
>
> Sounds good, marking the PR as draft again while I investigate.
>
> I've bisected to this commit in `lief`: [lief-project/LIEF@f23ced2](https://github.com/lief-project/LIEF/commit/f23ced2f4ffc170d0a6f40ff4a1bee575e3447cf)
>
> Seems related to scikit-build 0.10 [changing](https://iscinumpy.dev/post/scikit-build-core-0-10/#other-changes) `cmake.targets` to `build.targets`, and defining `build.target
...
💬 hebasto commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109683378)
Is `python-tomli` really necessary?
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109683378)
Is `python-tomli` really necessary?
💬 hebasto commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109682830)
```suggestion
lief==0.16.5 \
```
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109682830)
```suggestion
lief==0.16.5 \
```
👍 hebasto approved a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871843422)
re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f, only a [comment](https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775) has been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934).
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871843422)
re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f, only a [comment](https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775) has been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934).
💬 l0rinc commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#issuecomment-2913287986)
ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
(https://github.com/bitcoin/bitcoin/pull/32623#issuecomment-2913287986)
ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
🚀 hebasto merged a pull request: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439)
(https://github.com/bitcoin/bitcoin/pull/32439)
💬 hebasto commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2109748436)
Guix's `cmake-minimal` remains at version [3.24.2](https://codeberg.org/guix/guix/src/commit/b868ae741e989d56dbaf8dd481c66a1c00e45e25/gnu/packages/cmake.scm#L179).
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2109748436)
Guix's `cmake-minimal` remains at version [3.24.2](https://codeberg.org/guix/guix/src/commit/b868ae741e989d56dbaf8dd481c66a1c00e45e25/gnu/packages/cmake.scm#L179).
👍 pinheadmz approved a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871920816)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Built and tested on macos/arm64 and reviewed code. This is a simple optimization that deduplicates a call to `Block.GetHash()` when reading a block from disk by `BlockManager`. The hash is checked against the expected result from a `CBlockIndex`. Covered by a new unit test.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
-----BEGIN PGP SIGNATURE-
...
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871920816)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Built and tested on macos/arm64 and reviewed code. This is a simple optimization that deduplicates a call to `Block.GetHash()` when reading a block from disk by `BlockManager`. The hash is checked against the expected result from a `CBlockIndex`. Covered by a new unit test.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
-----BEGIN PGP SIGNATURE-
...
🤔 theStack reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2871820534)
Reviewed thoroughly up to ff7476ec32f4dbee07bf04169e741e2aea5a9ab7, looks good to me (with one refactoring suggestion below). Still need to go again through the parsing code as there is a lot going on with all the conditions on what is allowed and what not w.r.t. derivations.
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2871820534)
Reviewed thoroughly up to ff7476ec32f4dbee07bf04169e741e2aea5a9ab7, looks good to me (with one refactoring suggestion below). Still need to go again through the parsing code as there is a lot going on with all the conditions on what is allowed and what not w.r.t. derivations.
💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2109683327)
in commit ff7476ec32f4dbee07bf04169e741e2aea5a9ab7: can't this be simplified to
```suggestion
out += pubkey->ToString(type);
```
? It's a bit confusing that there are two `StringType` enum classes within different classes, (`PubkeyProvider::StringType` and `DescriptorImpl::StringType`), but the code here seems to deal only with the former. I guess whether the diff makes sense or not depends on if it's possible in the future that the default arguments for pubkey providers `ToStrin
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2109683327)
in commit ff7476ec32f4dbee07bf04169e741e2aea5a9ab7: can't this be simplified to
```suggestion
out += pubkey->ToString(type);
```
? It's a bit confusing that there are two `StringType` enum classes within different classes, (`PubkeyProvider::StringType` and `DescriptorImpl::StringType`), but the code here seems to deal only with the former. I guess whether the diff makes sense or not depends on if it's possible in the future that the default arguments for pubkey providers `ToStrin
...
💬 luke-jr commented on pull request "gui: Add a menu action to restore then migrate a legacy wallet":
(https://github.com/bitcoin-core/gui/pull/877#issuecomment-2913426259)
>by adding a menu item in the "Migrate Wallet" menu
IMO this is bad UX. Users will be looking to restore the wallet, and the details of how it does that shouldn't require unintuitive steps.
(https://github.com/bitcoin-core/gui/pull/877#issuecomment-2913426259)
>by adding a menu item in the "Migrate Wallet" menu
IMO this is bad UX. Users will be looking to restore the wallet, and the details of how it does that shouldn't require unintuitive steps.
💬 maflcko commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2913466098)
Can be turned into draft while the CI is red?
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2913466098)
Can be turned into draft while the CI is red?
🤔 rkrux reviewed a pull request: "wallet: Always set descriptor cache upgraded flag for new wallets"
(https://github.com/bitcoin/bitcoin/pull/32597#pullrequestreview-2872012392)
Concept ACK, code review 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a
> Newly created wallets will always have an upgraded descriptor cache, so set those.
Besides the logical accuracy, is there any other advantage of doing this that is used in #32489? Probably can mention it in the PR description.
(https://github.com/bitcoin/bitcoin/pull/32597#pullrequestreview-2872012392)
Concept ACK, code review 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a
> Newly created wallets will always have an upgraded descriptor cache, so set those.
Besides the logical accuracy, is there any other advantage of doing this that is used in #32489? Probably can mention it in the PR description.
💬 rkrux commented on pull request "wallet: Always set descriptor cache upgraded flag for new wallets":
(https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2109834391)
I don't suppose it's a big issue but while going through the creation flow it seems odd that this flag is set prior to even the setup of the SPKMs. I have not tried it but from a code flow perspective a call to `UpgradeDescriptorCache` can come right after `SetupDescriptorScriptPubKeyMans` that will set both the cache keys in the database along with this flag.
(https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2109834391)
I don't suppose it's a big issue but while going through the creation flow it seems odd that this flag is set prior to even the setup of the SPKMs. I have not tried it but from a code flow perspective a call to `UpgradeDescriptorCache` can come right after `SetupDescriptorScriptPubKeyMans` that will set both the cache keys in the database along with this flag.