💬 mzumsande commented on issue "Assertion pindexPrev && pindexPrev == chainstate.m_chain.Tip() when running regtest":
(https://github.com/bitcoin/bitcoin/issues/31562#issuecomment-2569582744)
> Thx alot! so is this expected to reach the next release cycle? which version will it be included in?

Looks like it will already be fixed in 28.1 (backport PR #31594) which should be released soon.

💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1902019482)
> It is a vector of package fee frac's, which can be used to build a histogram.

Building a histogram seems to be one of the use cases of this field. I don't feel it should be called a histogram only because of this. Is there any other reason as well?
I hope I'm not missing any context here.

🤔 mzumsande reviewed a pull request: "qa: Ensure consistent use of decimals instead of floats"
(https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2529511916)
See https://github.com/python/cpython/issues/128005 (long discussion, skip to [this post](https://github.com/python/cpython/issues/128005#issuecomment-2547582533) to save time) for more background on this.
This is NetBSD-specific, and apparently caused by a pkgsrc patch which caused `sys.float_repr_style` to be "legacy". So the question is if this qualifies as a bug in NetBSD or not and whether we need to accomodate it by moving to Decimals.

💬 achow101 commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#issuecomment-2569631398)
Pulled in @theStack's test

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1902048035)
I think you are, I'm not introducing this term in this PR
You can see https://github.com/bitcoin/bitcoin/pull/21422 for context

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