💬 Sjors commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3521838314)
Concept ACK
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
This was merely test code up until recently when I needed it for the mining interface. That happened in #30955 by moving `BlockMerkleBranch`, `MerkleComputation` and `ComputeMerkleBranch` from test code (back) to `merkle.{h,cpp}`.
At the time I didn't check for unused code paths. The mining interface doesn't need `mutated` or `proot`.
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3521838314)
Concept ACK
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
This was merely test code up until recently when I needed it for the mining interface. That happened in #30955 by moving `BlockMerkleBranch`, `MerkleComputation` and `ComputeMerkleBranch` from test code (back) to `merkle.{h,cpp}`.
At the time I didn't check for unused code paths. The mining interface doesn't need `mutated` or `proot`.
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3521842506)
> @sstone Question -- In LDK, when we confirm transactions, we require the transaction's index-in-block to be provided, for short channel ID construction. I'm curious how this API works for you without this information?
This index would be used to find if and how channels have been spent (after a restart for example) and would be much more efficient that walking down the blockchain from the tip.
To compute the short channel Id we simply call `getrawtransaction` which gives us the block ha
...
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3521842506)
> @sstone Question -- In LDK, when we confirm transactions, we require the transaction's index-in-block to be provided, for short channel ID construction. I'm curious how this API works for you without this information?
This index would be used to find if and how channels have been spent (after a restart for example) and would be much more efficient that walking down the blockchain from the tip.
To compute the short channel Id we simply call `getrawtransaction` which gives us the block ha
...
💬 Sjors commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-3521861054)
Past reviewers may be interested in the cleanup followup #33805.
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-3521861054)
Past reviewers may be interested in the cleanup followup #33805.
💬 hebasto commented on pull request "build: Bump g++ minimum supported version to 12":
(https://github.com/bitcoin/bitcoin/pull/33842#discussion_r2518278339)
Good to see this test gone, as it was annoying to set [`SeCreateSymbolicLinkPrivilege`](https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-10/security/threat-protection/security-policy-settings/create-symbolic-links) to make it pass when building natively on Windows.
(https://github.com/bitcoin/bitcoin/pull/33842#discussion_r2518278339)
Good to see this test gone, as it was annoying to set [`SeCreateSymbolicLinkPrivilege`](https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-10/security/threat-protection/security-policy-settings/create-symbolic-links) to make it pass when building natively on Windows.
🤔 Sjors reviewed a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3452502265)
Was in the middle of a review, but had to leave. Some comments.
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3452502265)
Was in the middle of a review, but had to leave. Some comments.
💬 Sjors commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2517601078)
In 23f8b6035ff6538134be3d3df8a3ab8ba47f86c0 _descriptor: ToPrivateString() pass if at least 1 priv key exists_: the variable name `any_success` had me confused. If you have to retouch, it would be good to document its meaning or rename it.
```cpp
// If a public string is requested, fail if no subscript returns
a public string. If a private string is requested, fail if no
subscript returns a private string.
```
Seems like `any_success` was suggested here: https://github.com/bitcoin/b
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2517601078)
In 23f8b6035ff6538134be3d3df8a3ab8ba47f86c0 _descriptor: ToPrivateString() pass if at least 1 priv key exists_: the variable name `any_success` had me confused. If you have to retouch, it would be good to document its meaning or rename it.
```cpp
// If a public string is requested, fail if no subscript returns
a public string. If a private string is requested, fail if no
subscript returns a private string.
```
Seems like `any_success` was suggested here: https://github.com/bitcoin/b
...
👍 laanwj approved a pull request: "kernel: Allow null arguments for serialized data"
(https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3453468849)
code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
- "kernel: Allow null arguments for serialized data" -- Allows nullptr arguments for three functions that accept spans. Also checks for incorrect usage of nullptr pointers, and returns nullptr (as documented) if so. Some test code was added, as well as moved.
- checked that these are the only three functions that this is relevant for.
- "ci: Enable experimental kernel stuff in ASan task" -- CI only commit to improve test coverage.
(https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3453468849)
code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
- "kernel: Allow null arguments for serialized data" -- Allows nullptr arguments for three functions that accept spans. Also checks for incorrect usage of nullptr pointers, and returns nullptr (as documented) if so. Some test code was added, as well as moved.
- checked that these are the only three functions that this is relevant for.
- "ci: Enable experimental kernel stuff in ASan task" -- CI only commit to improve test coverage.
👍 TheCharlatan approved a pull request: "kernel: add btck_block_tree_entry_equals"
(https://github.com/bitcoin/bitcoin/pull/33855#pullrequestreview-3453482721)
ACK 096924d39d644acc826cbffd39bb34038ecee6cd
(https://github.com/bitcoin/bitcoin/pull/33855#pullrequestreview-3453482721)
ACK 096924d39d644acc826cbffd39bb34038ecee6cd
👍 TheCharlatan approved a pull request: "ci: Enable experimental kernel stuff in most CI tasks"
(https://github.com/bitcoin/bitcoin/pull/33824#pullrequestreview-3453499963)
Nice, ACK fae83611b8ef358ea7aca7070fd7e82dc06f9755
(https://github.com/bitcoin/bitcoin/pull/33824#pullrequestreview-3453499963)
Nice, ACK fae83611b8ef358ea7aca7070fd7e82dc06f9755
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3521999252)
Rebased a5a8bff198f9f641c10e159fa3cc51757d8f69f6 -> ef1f96134098e73b47bfc988d878422917ceac1d ([blocktreestore_7](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_7) -> [blocktreestore_8](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_7..blocktreestore_8))
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3521999252)
Rebased a5a8bff198f9f641c10e159fa3cc51757d8f69f6 -> ef1f96134098e73b47bfc988d878422917ceac1d ([blocktreestore_7](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_7) -> [blocktreestore_8](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_7..blocktreestore_8))
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3522030114)
Fixed the linking issue with `qdbusviewer`, by just disabling building that tool.
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3522030114)
Fixed the linking issue with `qdbusviewer`, by just disabling building that tool.
💬 mmortal03 commented on issue "Option to use dark theme for Windows":
(https://github.com/bitcoin-core/gui/issues/378#issuecomment-3522060989)
> The issue has been resolved in [bitcoin/bitcoin#30997](https://github.com/bitcoin/bitcoin/pull/30997), as Qt 6.7.3 allows an app to use the dark system palette on Windows.
>
> Compare screenshots:
Based on my testing, this only works on Windows 11 currently.
To get it working on Windows 10, it would need the GUI to use one of the other Qt styles, other than what they have it default to for Windows 10, which is called the "windowsvista" style. See here: https://forum.qt.io/topic/163222/dark-
...
(https://github.com/bitcoin-core/gui/issues/378#issuecomment-3522060989)
> The issue has been resolved in [bitcoin/bitcoin#30997](https://github.com/bitcoin/bitcoin/pull/30997), as Qt 6.7.3 allows an app to use the dark system palette on Windows.
>
> Compare screenshots:
Based on my testing, this only works on Windows 11 currently.
To get it working on Windows 10, it would need the GUI to use one of the other Qt styles, other than what they have it default to for Windows 10, which is called the "windowsvista" style. See here: https://forum.qt.io/topic/163222/dark-
...
💬 janb84 commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3522064022)
Same got build failure (2x) at 758da5a5732433807f1d9c8f043de934982218ab
```sh
/ 'build' phasenote: keeping build directory `/tmp/guix-build-gcc-cross-x86_64-w64-mingw32-14.3.0.drv-0'
builder for `/gnu/store/akfhrz7dsfj133vqrdr79ipdhgvc5352-gcc-cross-x86_64-w64-mingw32-14.3.0.drv' failed with 1
build of /gnu/store/akfhrz7dsfj133vqrdr79ipdhgvc5352-gcc-cross-x86_64-w64-mingw32-14.3.0.drv failed
View build log at '/var/log/guix/drvs/ak/fhrz7dsfj133vqrdr79ipdhgvc5352-gcc-cross-x86_64-w64-mingw
...
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3522064022)
Same got build failure (2x) at 758da5a5732433807f1d9c8f043de934982218ab
```sh
/ 'build' phasenote: keeping build directory `/tmp/guix-build-gcc-cross-x86_64-w64-mingw32-14.3.0.drv-0'
builder for `/gnu/store/akfhrz7dsfj133vqrdr79ipdhgvc5352-gcc-cross-x86_64-w64-mingw32-14.3.0.drv' failed with 1
build of /gnu/store/akfhrz7dsfj133vqrdr79ipdhgvc5352-gcc-cross-x86_64-w64-mingw32-14.3.0.drv failed
View build log at '/var/log/guix/drvs/ak/fhrz7dsfj133vqrdr79ipdhgvc5352-gcc-cross-x86_64-w64-mingw
...
👋 fanquake's pull request is ready for review: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537)
(https://github.com/bitcoin/bitcoin/pull/33537)
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3522077989)
I think this is now reviewable / testable. I've updated our Guix symbol check to remove all `libxcb*` entries. The remaining dynamic Qt deps are Freetype, Fontconfig and the dynamic loader.
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3522077989)
I think this is now reviewable / testable. I've updated our Guix symbol check to remove all `libxcb*` entries. The remaining dynamic Qt deps are Freetype, Fontconfig and the dynamic loader.
🤔 fjahr reviewed a pull request: "init: Require explicit -asmap filename"
(https://github.com/bitcoin/bitcoin/pull/33770#pullrequestreview-3453538980)
Approach ACK
While not everyone I asked has gotten back to me yet, almost all responders did already use an explicit file path and the few that had used the option of the default file had similar sentiment to @sipa.
The file should probably also be dropped from `files.md` then since there is nothing special about that name/location anymore, e.g. https://github.com/fjahr/bitcoin/commit/e0283bb8c133c8cae19c48c8d78cfe95dab664ee
(https://github.com/bitcoin/bitcoin/pull/33770#pullrequestreview-3453538980)
Approach ACK
While not everyone I asked has gotten back to me yet, almost all responders did already use an explicit file path and the few that had used the option of the default file had similar sentiment to @sipa.
The file should probably also be dropped from `files.md` then since there is nothing special about that name/location anymore, e.g. https://github.com/fjahr/bitcoin/commit/e0283bb8c133c8cae19c48c8d78cfe95dab664ee
💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518375913)
Effectively the arg is not allowed anymore without a path to a file, so I think it would make sense to return an explicit error here which gives the user that feedback. The current error for the empty `-asmap` is "Could not find asmap file /path/to/datadir" which suggests there still is a default location where the file (without a name) could not be found.
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518375913)
Effectively the arg is not allowed anymore without a path to a file, so I think it would make sense to return an explicit error here which gives the user that feedback. The current error for the empty `-asmap` is "Could not find asmap file /path/to/datadir" which suggests there still is a default location where the file (without a name) could not be found.
💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518380662)
nit: I was going to say that the unspecified file and the missing file test could be deduplicated since they are currently similar but I think the better change would be to have an explicit error message for the empty `-asmap` arg.
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518380662)
nit: I was going to say that the unspecified file and the missing file test could be deduplicated since they are currently similar but I think the better change would be to have an explicit error message for the empty `-asmap` arg.
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2518433916)
double-checked test that it would go through properly funded (works):
```
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index 4b2c89f3d9..10e3239a33 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -346,4 +346,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert_raises_rpc_error(-26, "insufficient feerate: does not improve feerate diagram", self.nodes[0].sendrawtransaction, tx_replacement['hex'])
+
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2518433916)
double-checked test that it would go through properly funded (works):
```
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index 4b2c89f3d9..10e3239a33 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -346,4 +346,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert_raises_rpc_error(-26, "insufficient feerate: does not improve feerate diagram", self.nodes[0].sendrawtransaction, tx_replacement['hex'])
+
...
💬 laanwj commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3522121392)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
i personally haven't trusted luke-jr to run important bitcoin core infrastructure since he hijacked the Transifex for Knots, removing the other maintainer's admin access. He could potentially use the DNS seed in some way to further Knots, or in a way harmful to this project. Or might already be doing so.
Recent developments, furthering bad blood between the projects, haven't improved my sentiment on this.
In terms of risk minimization, it mak
...
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3522121392)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
i personally haven't trusted luke-jr to run important bitcoin core infrastructure since he hijacked the Transifex for Knots, removing the other maintainer's admin access. He could potentially use the DNS seed in some way to further Knots, or in a way harmful to this project. Or might already be doing so.
Recent developments, furthering bad blood between the projects, haven't improved my sentiment on this.
In terms of risk minimization, it mak
...