👍 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()`).

💬 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_r1902153333)
Just as a fun fact: this function could be reduced to a single line:
```c++
bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
{
return m_xonly ? arg.GetKeyByXOnly(XOnlyPubKey(m_pubkey), key) :
arg.GetKey(m_pubkey.GetID(), key);
}
```

💬 tdb3 commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902173650)
We repeat this pattern throughout the file.
Unless I'm missing something, seems like a good opportunity to de-duplicate with something like:

```c++
CBlock CreateAndGetBlock(const node::BlockCreateOptions& options)
{
auto mining{MakeMining()};
BOOST_REQUIRE(mining);
std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
BOOST_REQUIRE(block_template);
return block_template->getBlock();
}
```

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

Nice updates. Left a minor non-blocking comment.

💬 laanwj commented on pull request "[28.x] 28.1 backports and final changes":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2569868533)
> Are these significant enough to warrant a rc3 or can we go straight to final?

The only change here that could trigger a new RC is the change in `src/rpc/mining.cpp`, which affects the release binary, but as that's contained to a test-only RPC i don't think it's worth rolling another rc for. The rest are test changes.

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569873452)
Thank you for the review @mzumsande,

Updated f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 -> 182945e946ac6a92daad012d6579e5441b9641cc ([blockmanDB_4](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_4) -> [blockmanDB_5](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_4..blockmanDB_5))

* Addressed @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977), added documentatio
...