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

🤔 l0rinc reviewed a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2528790297)
Thanks for improving these!
Since we can’t usually change the behavior, it’s crucial to share tribal knowledge.
I have a few suggestions for reducing duplication, simplifying sentence structure, and applying these changes to similar places.
However, I understand if you prefer to limit the scope

💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901633014)
I had to read this several times (didn't understand what a "least-significant-first base" is at first), consider the following:
```suggestion
/** Integer with 32-bit digits, least-significant first. */
```

💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901648055)
Could we be more compact here?
Personally I'm not a fan of long documentation - though the reason usually is that if we need to document something heavily it's a sign of poorly written code - not something we can really change here.

💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901655760)
documenting `txid` as "it's the txid hash" is a bit redundant and confusing.
In https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L135 (and a few other places) the description is `"The transaction hash in hex"`.
Could we unify the rest as well to something simple like
```suggestion
{RPCResult::Type::STR_HEX, "txid", "transaction id hash (without witness) shown in byte-reversed hex"},
```

💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901612593)
```suggestion
* This means, for example, that ArithToUint256(num).GetHex() can be used to
* display an arith_uint256 num value as a number, because
```

💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901643256)
According to Wiki `little endian` is preferred with a dash: https://en.wikipedia.org/wiki/Endianness

But the part I find confusing is rather expressing 3 concepts in a single sentence - can we break it up a bit?

```suggestion
/** Get the hash of the wtxids of these transactions, treating each wtxid hash
* as a little-endian number and sorting them in ascending numeric order.
* This is equivalent to reversing the bytes of each wtxid hash and sorting them lexicographically.
*/
```

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669070)
That makes sense, added an explicit check above the `waitTipChanged()` call.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669119)
I dropped this comment.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669167)
Ok, I dropped them for the commit and PR description and updated the code comment.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669229)
Renamed.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669273)
A failing `BOOST_REQUIRE` produces an easier to debug failure than dereferencing nullptr, so it's always a good one to add.

I updated the test to do this after every `miner->createNewBlock()` call.

Similarly `BOOST_REQUIRE` stops the test immedidately, so I replaced a few `BOOST_CHECK` calls with it.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569034227)
Rebased and addressed feedback.