💬 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

💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2573295321)
I tried testing the asan job, via `time MAKEJOBS="-j17" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh`, on a Fedora dev box, and it fails with the following output:
```bash
+ chown root:root /tmp/tcpdump_unitparallel_eth0
++ tcpdump -n -r /tmp/tcpdump_unitparallel_eth0 --direction=out tcp or udp
reading from file /tmp/tcpdump_unitparallel_eth0, link-type EN10MB (Ethernet), snapshot length 262144
+ out='14:47:32.023429 IP6 1111:1111::4.33262 > 1111:1111::1.53: 9493+ A
...