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

💬 Sjors commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901672336)
I think it's a useful comment.