💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3521439654)
> Since when is this "unused"?

Did some digging, https://github.com/bitcoin/bitcoin/commit/1f0e7ca09c9d7c5787c218156fa5096a1bdf2ea8#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165 inlined `ComputeMerkleRoot` in one case, leaving the `proot` and `pmutated` values unused in `MerkleComputation` (cc: @sipa, @Sjors).
And `BlockWitnessMerkleRoot` was introduced in https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93 where it was already
...

💬 yancyribbens commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#issuecomment-3521446053)
@murchandamus ; would appreciate your input on this.

💬 TheCharlatan commented on pull request "ci: Enable experimental kernel stuff in most CI tasks":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2517943925)
Might we be able to keep this `ON` too, if the windows job also copies over the dll?
```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index d433646292..e2bd3a6940 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -369,0 +370 @@ jobs:
+ ${{ env.BASE_BUILD_DIR }}/bin/*.dll
```

💬 stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2517974921)
The fact that `CBlockIndex` equality is currently defined as pointer equality seems like an implementation detail, tightly coupled with our block tree implementation. Exposing that as part of our public interface seems brittle and unergonomic?

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.

💬 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,
...

🤔 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.

💬 rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(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.

💬 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.

✅ maflcko closed an issue: "`test_kernel` fails to build on Ubuntu 22.04"
(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
...

💬 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`.

💬 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
...

💬 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.

💬 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.

🤔 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.

💬 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
...

👍 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.

👍 TheCharlatan approved a pull request: "kernel: add btck_block_tree_entry_equals"
(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