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

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904134914)
Would be good to check against negative `-dbcache` values as well.

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904029488)
Good to have this for user supplied data instead of triggering asserts!

nit: Might be able to skip one cast:
```suggestion
if (static_cast<uint64_t>(db_cache << 20) > std::numeric_limits<size_t>::max()) {
```

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904015194)
nit: Could deserve a brief comment?
```C++
//! Guards against truncation of values before converting.
constexpr size_t MiBToBytes(int64_t mib)
```

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904214972)
nit: Simpler to say 4GiB?

I know 32-bit Windows has issues using more than 2GB unless one sets `/LARGEADDRESSAWARE`, not sure whether we do that. See https://learn.microsoft.com/en-us/windows/win32/memory/memory-limits-for-windows-releases

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904193756)
nit: Could use more modern `LogInfo()` instead.

📝 vasild opened a pull request: "ci: build msan's libc with _LIBCPP_ABI_BOUNDED_*"
(https://github.com/bitcoin/bitcoin/pull/31612)
For the task `MSan, depends (Cirrus CI)` we build a custom libc++ for which we already use `-DLIBCXX_HARDENING_MODE=debug`. Compile it also with `_LIBCPP_ABI_BOUNDED_*` to enable further checks.

Docs at: https://libcxx.llvm.org/Hardening.html#abi-options

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin
...

💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2573250107)
`7daae2cf96...39a1326488`: drop some changes and move them into a separate PR: https://github.com/bitcoin/bitcoin/pull/31612, suggested by @maflcko