💬 maflcko commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2568413121)
> 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.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901324742)
nit: The `Collision` here could be `LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, “…”)`.

🤔 ariard reviewed a pull request: "p2p: Fill reconciliation sets (Erlay) attempt 2"
(https://github.com/bitcoin/bitcoin/pull/30116#pullrequestreview-2528318804)
Reviewed up to 4b9f83a (“p2p: Makes transactions available…”), all the code changes.
I’ll check the test coverage.

💬 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_r1901360700)
nano nit:
``` c++
CKey key;
if (!GetPrivKey(/*pos=*/0, arg, key)) return false;
ret = EncodeSecret(key);
return true;
```

🤔 furszy reviewed 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-2528398156)
looking good, only left few nits. No need to tackle them. Will test it properly in the coming days.

💬 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