💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1904148406)
Related to [this comment](https://github.com/bitcoin/bitcoin/pull/30391/files#r1904134687), I find the language here confusing as well. A fee rate histogram element, which I would expect to be a feeRate interval/group, is being compared to a feeFrac element, which is just a pair of fee and size.

💬 TheCharlatan commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1904153448)
I don't think the risk to the miner is big enough to warrant holding this up, especially since the timewarp fix is not actually deployed. A lot of things have to align and the miner needs to be doing weird things with setting their timestamp.

👍 TheCharlatan approved a pull request: "Miner: never create a template which exploits the timewarp bug"
(https://github.com/bitcoin/bitcoin/pull/31376#pullrequestreview-2532144755)
ACK 733fa0b0a140fc1e40c644a29953db090baa2890

🚀 fanquake merged a pull request: "lint: Move assertion linter into lint runner"
(https://github.com/bitcoin/bitcoin/pull/31435)

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2573130715)
`48843ec694...bbfc58a0af`: rebase and "If there are only a few tasks that do not work, it may be better to just carve them out, instead of enumerating all the ones that work", see [above](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1896679919).

💬 TheCharlatan commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1904163181)
I think this should also say that the next block miner also has to use their own wall clock time and not the one provided by the template?

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1904166046)
You are right! Flipped the logic to exclude only macOS.

No strong opinion on "tidy" - I added it because those tests should not generate internet traffic. Would drop it if requested.

💬 Sjors commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2573155058)
ACK 733fa0b0a140fc1e40c644a29953db090baa2890

A test would be nice.

After some discussion in https://delvingbitcoin.org/t/timewarp-attack-600-second-grace-period/1326 the main concern I have left, is about miners that ignore the timestamp. This PR has no impact on those miners (because they're not using the timestamp that this PR modifies).

My initial concern was that the eventual soft fork would have a _lower_ `MAX_TIMEWARP`, but this seems very unlikely: https://github.com/bitcoin/bitc
...

💬 Sjors commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1904186696)
I expanded the comment.

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904183854)
Nice, thanks!

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904184387)
Of course, done

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904124892)
Could you elaborate on how we would get missing ancestors beyond parents? We could iterate through the vin of parents we already have, but I don't see how any of those could be missing. And we can't look through the vin of a missing parent.

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904121487)
Yes it's possible (have a comment "The resulting vector may contain duplicate NodeIds"). The cases I can think of, from least likely to most likely:

- the peer announced both txid and wtxid to us (Bitcoin Core wouldn't do this)
- there is a transaction circulating with and without a witness (very rare?)
- the tx is announced by wtxid and we are also trying to fetch it by txid because its child is in our orphanage

So I expect duplicates will be very rare. Also, the worst case is that a No
...

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904206083)
Hm right, I don't think we can assume that it's never empty - I've added an exit case

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904179956)
thanks, done

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904137841)
done

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904211617)
Yes, it is similar. I'm open to code diff suggestions.

I tried a few times to refactor but found some things a bit ugly to work around: we force the `IsWtxid` check to be false since we're requesting parents by txid, and the number of requests is not always 1 (also see https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1901875482). In the future, the function may deviate when we add more per-peer orphan resolution limits and/or another way to do orphan resolution.

💬 glozow commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1904219603)
Ah, @ismaelsadeeq did you mean to call this a feerate *diagram* instead?

@rkrux We've previously represented feerate diagrams as a vector of `FeeFrac`s / chunks
- https://delvingbitcoin.org/t/mempool-incentive-compatibility/553#feerate-diagrams-as-a-way-to-resolve-this-question-3
- #29242

🤔 hodlinator reviewed a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2531853195)
Code review ad80e410b3bf8814cb5b997715ee8cb28f2a2a66

Thanks for incorporating so much of my feedback!

Verified that documented cache configuration output matches PR summary. (Also double-checked that allocated memory budget without `-txindex` and `-blockfilterindex=basic` is unchanged).

Built for 32-bit (i686 centos).

---
ad80e410b3bf8814cb5b997715ee8cb28f2a2a66 - commit message typo: s/not/note

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1903999989)
(Sorry to be correcting my own suggestion, ran out of steam a bit by the time I landed on the approach of using this free function).

- The 2 first asserts could be improved through replacing them with this more complex one.
- `size_t` is always unsigned, it was probably unnecessary to cast the max value in the last assert.

```suggestion
Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21); // Ensure signed bit is unset + enough zeros to shift.
const int64_t bytes{mib << 20
...