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

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681661927)
```suggestion
* - Otherwise: tweak the internal key with H_TapTweak(pubkey || *merkle_root)
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681636478)
maybe it would simplify the diff if we extracted the keypair.data() here
```C++
KeyPair keypair = ComputeKeyPair();
if (!keypair.IsValid()) return false;
const secp256k1_keypair *keypairData = reinterpret_cast<const secp256k1_keypair *>(keypair.data());
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681662778)
```suggestion
* (this is used for key path spending with a specific
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681669713)
nit: typo in commit message: `Instead of calling ... instead invalidate`