💬 furszy commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1901358754)
nit:
```c++
return key.IsValid();
```

🤔 ryanofsky reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2528244641)
Code review 7f34802fd726b259f7df6f605e1d488faf251127. I got pretty lost here and only reviewed about half of it so far, but overall I think the changes look good and make a lot of sense.

I left some suggestions below which I think could make code clearer, but they are not important so feel free to ignore any you disagree with.

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901278842)
In commit "Add DeriveTarget() to pow.h" (2f549b547207839ab666ac2b8b02811ff0990214)

I don't really like the approach here of duplicating the `CheckProofOfWorkImpl` code and having a comment saying a future refactor could deduplicate it. I think it could avoid confusion and bugs later on to just split the `CheckProofOfWorkImpl` function into two parts now in a way that makes it obvious behavior is not changing. Would suggest:

```diff
--- a/src/pow.cpp
+++ b/src/pow.cpp
@@ -143,7 +143,7 @@
...

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901345161)
In commit "rpc: gettarget" (0c56689e425684c0c5b2058c1a47f293ac7032e6)

It's not right to involve the `ser_uint256` function here, since converting the target `int` to a string should not involve bitcoin serialization. This can just use python string formatting `return f"{target:064x}"`

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901323476)
In commit "test: add nBits coverage" (ecb18b3405e4a61f872d03f667c167fb7c4a52cf)

I think it would be good to make this more descriptive. Maybe add "Compact representation of the block difficulty target, which the block hash interpreted as a little-endian number must not be greater than. This is hexadecimal string containing 32-bit number with the compact representation described in the arith_uint256 class of the difficulty target."

---

I also think it's really unfortunate the RPC is retu
...

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901357898)
In commit "test: add nBits coverage" (ecb18b3405e4a61f872d03f667c167fb7c4a52cf)

`return f"{nbits:08x}"` would be a more direct conversion from number to string and would make the python code more consistent with the c++ code.

📝 ryanofsky opened a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596)
This is a documentation-only change following up on suggestions made in the #30526 review.

Motivation for this change is that I was recently reviewing #31583, which reminded me how confusing the arithmetic blob code was and made me want to write better comments.

💬 ryanofsky commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901379737)
I don't think it's necessary to change this, but the principle here is to avoid having unnecessary template functions and unnecessary template parameters when it is possible to use concepts directly. In this case there's no point in defining a type `B` to stand in for `ByteType` and then using `B` one time when you can just use `ByteType` directly.

I don't think it would be possible to write a clang-tidy plugin to check for these cases because the check would need to scan the codebase and ens
...

💬 sipa commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901384725)
@ryanofsky I don't think that's right; they're all just shorthand notations for the next thing:

* `void func(ByteType auto b)`
* `template<ByteType T> void func(T b)`
* `template<typename T> requires ByteType<T> void func(T b)`

👍 theStack approved a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2528694272)
Concept and code-review ACK 116ed1d543641006f74bf089c23714259b6d4904

commit message / PR description micro-nit: haven't seen the term "evenness" before in this context, I think we usually call it "parity bit" (as in [BIP-341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-10))

💬 hebasto commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2568839311)
> > many functional tests fail due to `float` numbers internal representation.
> > A typical error looks as follows:
>
> It would be good to explain this a bit better and provide a `--tracerpc` log, or similar.

Sure! I've updated the PR description with the `--tracerpc` log.

💬 Sjors commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#issuecomment-2568880002)
ACK 13464c0c44645e0ed88d9d72c3ad697dca800e10

📝 hebasto opened a pull request: "ci: Switch to latest macOS and Windows images"
(https://github.com/bitcoin/bitcoin/pull/31597)
This PR updates the macOS and Windows images to their latest versions, in line with our usual practice.

Additionally, the Xcode version has been updated to 16.2.

For more details regarding these images, please refer to:
- https://github.com/actions/runner-images/issues/10686
- https://github.com/actions/runner-images/issues/11228

💬 dergoegge commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2568948899)
@reardencode @0xBEEFCAF3 @EthanHeilman Perhaps this is interesting for you all to review as it is relevant to testing your soft-fork proposals (i.e. #29247, #29269).

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901630874)
Taken

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901630902)
Taken

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901631082)
Ok, I used "nBits: compact representation of the block difficulty target".

To avoid confusion, I introduced a new commit 45e2edde903e71fbb085860af781f5ac65095579 that adds a "target" field to `getblockheader` and `getblock` (as well as their REST equivalents).

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901631250)
Done in 6accee18442f79fd2f2796027371a70e3757d825.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2568971908)
Thanks for the review @ryanofsky. I took your suggestion to split `CheckProofOfWorkImpl`. Can you add the Consensus label to this PR?

@tdb3 I took your two patches and one suggestion

👍 hebasto approved a pull request: "ci: Enable DEBUG=1 for one GCC-12 build to catch 117966 regressions"
(https://github.com/bitcoin/bitcoin/pull/31522#pullrequestreview-2528826222)
ACK fa1d84702bf4c14d55c51b9ab4cd30cdbbc5ad44, tested locally by reverting https://github.com/bitcoin/bitcoin/commit/fa9e0489f57968945d54ef56b275f51540f3e5e4 back and observing a compiler error.