💬 ryanofsky commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1681394436)
re: https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1681361423

Just to clarify for other reviewers since the names of these macros are confusing: In bitcoin core, `assert` and `Assert` macros behave very differently from the `assert` macro in standard c++. In standard c++ `assert` statements are not executed at all in release builds, while in bitcoin core they are always executed and always cause the execution to abort if they fail. Other c++ libraries that don't to be confusing use
...

🤔 hebasto reviewed a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2183505652)
Tested 22405f6f777e8701cbef0103fcfa06fd9bd109cb.

By default, the `ZeroMQ` project sets `CMAKE_BUILD_TYPE` to `Release`, which implies the `-O3 -DNDEBUG` flags override our `$(host)_release_CXXFLAGS`.

I suggest passing `-DCMAKE_BUILD_TYPE=None` to disable this overriding, which also works with `DEBUG=1`.

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