💬 TheCharlatan commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518001181)
> Logically, I think two BlockTreeEntry objects are equal if they point to the same Block. I've force-pushed a slight doc update to reflect that.
I would contest that - a block tree entry has a bunch of additional properties besides the block it points to that could make an equality operator by block confusing. Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields,
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518001181)
> Logically, I think two BlockTreeEntry objects are equal if they point to the same Block. I've force-pushed a slight doc update to reflect that.
I would contest that - a block tree entry has a bunch of additional properties besides the block it points to that could make an equality operator by block confusing. Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields,
...
🤔 rkrux reviewed a pull request: "wallet: remove redundant sighash calculation in Musig2 signing flow"
(https://github.com/bitcoin/bitcoin/pull/33665#pullrequestreview-3453129469)
> Each commit should leave the code in a valid state, so it would make sense to move https://github.com/bitcoin/bitcoin/commit/000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
Makes sense, done.
(https://github.com/bitcoin/bitcoin/pull/33665#pullrequestreview-3453129469)
> Each commit should leave the code in a valid state, so it would make sense to move https://github.com/bitcoin/bitcoin/commit/000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
Makes sense, done.
💬 rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518072408)
Done
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518072408)
Done
💬 rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068172)
I recall doing it intentionally based on the response I received on a similar suggestion in the MuSig2 signing PR: https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355476052.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068172)
I recall doing it intentionally based on the response I received on a similar suggestion in the MuSig2 signing PR: https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355476052.
💬 rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068751)
`ComputeSchnorrSignatureHash` function of the class `BaseSignatureCreator` had to be made public so that it can be called from the `SignMuSig2` function that makes an implementation in the `DummySignatureCreator` class necessary.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068751)
`ComputeSchnorrSignatureHash` function of the class `BaseSignatureCreator` had to be made public so that it can be called from the `SignMuSig2` function that makes an implementation in the `DummySignatureCreator` class necessary.
✅ maflcko closed an issue: "`test_kernel` fails to build on Ubuntu 22.04"
(https://github.com/bitcoin/bitcoin/issues/33846)
(https://github.com/bitcoin/bitcoin/issues/33846)
💬 stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518136714)
> Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields, but point to the same block?
I'm not sure what that would mean conceptually. If we're using the block tree to store validation status as we do now, that would mean we'd let the user define conflicting validation states about a block in the same index?
My conceptual understanding of a `BlockTreeEntry` is th
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518136714)
> Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields, but point to the same block?
I'm not sure what that would mean conceptually. If we're using the block tree to store validation status as we do now, that would mean we'd let the user define conflicting validation states about a block in the same index?
My conceptual understanding of a `BlockTreeEntry` is th
...
💬 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
...