📝 l0rinc opened a pull request: "Modernize recent `ByteType` usages and simplify read/write functions"
(https://github.com/bitcoin/bitcoin/pull/31601)
Follow-up of https://github.com/bitcoin/bitcoin/pull/31524, applying a few of the suggestions to the codebase, while enabling more trivial reads/writes to use the new `std::byte` interface.

💬 l0rinc commented on pull request "refactor: Allow std::byte in (Read/Write)(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1902054044)
The more I looked at it in this case, the more I actually like it - I pushed a change to https://github.com/bitcoin/bitcoin/pull/31601, let's discuss it there

💬 achow101 commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2569659777)
ACK faf7eac364fb7f421a649b483286ac8681d92b31

🤔 glozow reviewed a pull request: "[28.x] 28.1 backports (final?)"
(https://github.com/bitcoin/bitcoin/pull/31594#pullrequestreview-2529550653)
ACK 1451c56cce3e3df784e1dac969ffb0165df313c7 except for release note

💬 glozow commented on pull request "[28.x] 28.1 backports (final?)":
(https://github.com/bitcoin/bitcoin/pull/31594#discussion_r1902065226)
Should also add hebasto to the credits

👍 vasild approved a pull request: "test: have miner_tests use Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2529550479)
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

Not in this PR, just an observation: given that we have two concepts: "block" and "block template", I find the below pattern a bit confusing:

```
block_template = mining->createNewBlock()
block = block_template->getBlock()
```
because I would expect `createNewBlock()` to create a new block, but apparently it creates a new block template.

💬 vasild commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902065107)
Feel free to ignore: could use `block.vtx[0]` here instead of `MakeTransactionRef(txCoinbase)`:

```suggestion
BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, block.vtx[0]));
```
and reduce the scope of `txCoinbase` to only inside the above `{` `}` block.

💬 fanquake commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2569669491)
@mzumsande thanks for providing the actual context. Given the above, this doesn't seem like a great change.

🚀 achow101 merged a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542)

🚀 achow101 merged a pull request: "build: Use character literals for generated json headers to avoid narrowing"
(https://github.com/bitcoin/bitcoin/pull/31547)

💬 hebasto commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1902082301)
> Is it important this is a Decimal?

I confirm that your patch works.

> ... floating point values weren't defined to be this precise anyway

See @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2529511916).

🤔 sipa reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2497104576)
I wonder if it's possible to avoid keeping track of the announcers in `m_orphans`, as I think it should match `m_txrequest.GetCandidatePeers(orphan_tx)`?

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1881060842)
In commit "[txrequest] GetCandidatePeers"

Would it be possible to add an invocation/comparison with this function in the `src/test/fuzz/txrequest.cpp` fuzz test?

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2569724440)
> I wonder if it's possible to avoid keeping track of the announcers in `m_orphans`, as I think it should match `m_txrequest.GetCandidatePeers(orphan_tx)`?

That's true immediately after receiving the orphan, but we delete the entries from `m_txrequest` once we've added this tx to the orphanage - `m_txrequest` only stores what we haven't successfully downloaded yet. So I think we need to store the announcers somewhere else to remember that information, remove candidates that fail to resolve th
...

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902108458)
Thanks, will consider if I need to retouch. Though I think `txCoinbase` is more clear than `block.vtx[0]` and limiting the scope isn't that important.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569731568)
> because I would expect `createNewBlock()` to create a new block, but apparently it creates a new block template.

Indeed, I could rename the interface method (different PR) to e.g. `newBlockTemplate()`. Though I'd rather avoid the code churn in my pile of PRs :-)

🤔 tdb3 reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2529629807)
Approach ACK

Reviewed code changes.

Syncing from genesis on mainnet (because we're touching `CheckProofOfWorkImpl()`, I don't mind being paranoid, and it happens to be the genesis block anniversary today), so will report back (with ACK if all is well).

Looks like we could add `target` to `getblockchaininfo` and `getchainstates` as well (the top two commits on https://github.com/tdb3/bitcoin/commits/2024/12/gettarget/ add target and associated tests). Could be left for follow up if desir
...

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2569749036)
> >
>
> That's true immediately after receiving the orphan, but we delete the entries from `m_txrequest` once we've added this tx to the orphanage - `m_txrequest` only stores what we haven't successfully downloaded yet.

Right, of course!

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2569803135)
Thank you for the review @stickies-v,

Updated 6c3bc478ca343557d798e2b18b5f99c08079ba9a -> 8e86e77311a2c76fd34b4481f83205b2432b151a ([chainman_flush_destructor_0](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_0) -> [chainman_flush_destructor_1](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_0..chainman_flush_destructor_1))

* Addressed @stickies-v's [commen
...

🤔 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-2529696656)
q: why not store the id belonging to the pubkey with the correct parity bit inside the `SigningProvider` arg at the point where such a structure is loaded? (which most likely happens inside `ParsePubkeyInner()`).