💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681413085)
If I'm not mistaken, we currently have no means to convert `DEBUG=1` into `-DCMAKE_BUILD_TYPE=Debug`, which means that this line will never be used.

💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1681493872)
In 21b6bf4268e40020523d027e2811402b17b755eb "descriptor: Implement rawnode() partial descriptor": Is this `IsHex` verification necessary? Wouldn't it be verified in `ParseHex`?

Anyway, a test would be nice.

💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1681539455)
In ba99e596a86531cc4ad00826ab24804bc02dc777 "descriptor: Add TRNodeInfo": Does `leaf_version` need to be `int`? https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-7

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1681552971)
done, thanks!

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1681553101)
done, thanks!

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1681554329)
I have now added a single function `CheckHeaderForData` that takes a bool for querying undo data.

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1681554526)
fixed (see below)

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1681554767)
fixed (see below)

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1681555568)
fixed

🤔 stickies-v reviewed a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2183757955)
Approach ACK c3a9c71c7077324bf0aa40f834f7537a14619340

💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1681559119)
nit: I interpret "historical fears" as "the fears are no longer applicable", in which case I think it should just be a constructor and this helper function removed. But, it seems like until C++23 this is indeed an issue (even if the ambiguous overload with the `uint8_t` should prevent compilation), so the current wording is confusing to me.

Suggested alternative:

```
/* uint256 from std::string_view.
* This is not a uint256 constructor to avoid the ambiguity of
* uint256(0) being inte
...

💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1681561583)
In a945bc21d4ad4fd5da68ae783ec295d27f50227a "doc: Add reference for rawnode() and rawleaf()": Wouldn't it be `rawleaf(HEX[,LEAF_VERSION_HEX])`?

💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1681563025)
maybe
```suggestion
- `rawleaf(HEX,[LEAF_VERSION_HEX])`(inside `tr` only): specify raw leaf script and leaf version in HEX encoding. `LEAF_VERSION_HEX` defaults to `c0`
```

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2234010282)
I have now updated the PR with the feedback above.
- split the changes over multiple commits
- adjusted all call sites of `ReadRawBlockFromDisk` / `ReadBlockFromDisk` / `UndoReadFromDisk` from rpc / rest to to check the blockindex before whether we expect to have the (undo) data.
- Introduced a single function `CheckHeaderForData()` to throw the rpc errors that is called by multiple rpcs.
- added some more test coverage

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1681594420)
Thank you for the correction! Applied with some minor adjustments.

🤔 paplorinc reviewed a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2183858096)
Very nice separation into commits, left some questions

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681620128)
nit: typo in commit message: `his allows keeps the secret`,`allows for passing`, `the logic for for applying`, `and the finally move`

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681626093)
nit: would it make sense to use `IsValid()` here?

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681668029)
```suggestion
* expecting a `secp256k1_keypair`. This avoids the need to create a `secp256k1_keypair` object and
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681648449)
Might make the code more uniform (and maybe the diff simpler, if we'd extract it as suggested above) if instead of not making this a const cast, like the rest, we rather added the const to the method's signature:
```patch
diff --git a/src/secp256k1/include/secp256k1_extrakeys.h b/src/secp256k1/include/secp256k1_extrakeys.h
--- a/src/secp256k1/include/secp256k1_extrakeys.h (revision 21d031e7867ef97c07cee691c115d59b3aebfd19)
+++ b/src/secp256k1/include/secp256k1_extrakeys.h (date 1721245901582
...