💬 maflcko commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2573049796)
ACK 28e374f0a7dc1bda1c71e14eb896b2d9d2f23b48 🍭

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 28e374f0a7dc1bda1c71e14eb8
...

💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1904134687)
Thank you for sharing that PR. The term `vFeerateHistogram` in itself is fine and in the #21422 PR a histogram is indeed created.
What I find confusing here is calling a simple vector of feefrac's a histogram. If a histogram will be created using this field later, let's call **that field** a histogram and not this one?

💬 hebasto commented on issue "build: compiler flags in linker flags output":
(https://github.com/bitcoin/bitcoin/issues/31487#issuecomment-2573088492)
> Same for stuff like: `[09:29:46.367] Linker flags .......................... -Wno-error=return-type -Wno-error=maybe-uninitialized -Wno-error=array-bounds` Showing this in "Linker flags" output doesn't make sense.

Replacing `CMAKE_CXX_FLAGS` with `APPEND_CXXFLAGS` to provide those flags will help.

💬 fanquake commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2573089576)
@hebasto Given that your suggestion of

[> If a user wants to override any of these, they should use the CMAKE_CXX_FLAGS_<CONFIG> ](https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2541620375)

doesn't work. What should users do? Are we disallowing `-O3` wholesale? If this is the case, it should probably be documented (with the rationale), and likely warned about at configure time (if they have set `-O3` via `CMAKE_CXX_FLAGS_` or any other way), rather than sliently swapping user
...

💬 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