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

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904274621)
Ah, by recursing through transactions in the orphanage I meant.

Imagine a sequence A->B->C (A spends on output of B, B spends an output of C). We have received A and B already (in orphanage), but C is still missing. An announcement from a new peer for transaction A can reasonably be treated as an announcement for C from that peer.

👍 ryanofsky approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2532359577)
Code review ACK 06ff9429d32943aa4debec38ea105c8af4283aec. It seems like a improvement to me overall if binaries in the build directory have the same layout as binaries in releases. I'm not sure current implementation of this is ideal but it seems like a a step in the right direction.

Having binaries built in consistent locations that match install locations should make it is easier to find them, make scripts and wrappers simpler, and avoid bugs that only happen in developer builds but not fin
...

🤔 jarolrod reviewed a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2532365008)
ACK fa029a78780fd00e1d3ce1bebb81a95857bfbb94

> Does macOS auto-update or provide pop-ups for minor releases?

Yeah, depending on the users preference for auto-updates. See ["Automatic Updates"](https://support.apple.com/guide/mac-help/software-update-settings-on-mac-mchla7037245/15.0/mac/15.0)

🚀 fanquake merged a pull request: "util: Add missing types in make_secure_unique"
(https://github.com/bitcoin/bitcoin/pull/31464)

💬 i-am-yuvi commented on pull request "#31403 follow-up":
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573352968)
> ACK [a3f5573](https://github.com/bitcoin/bitcoin/commit/a3f5573ae3aa5edbe92d0388f36586ad02214de6)
>
> First commit follows recommendations by myself with improvements by maflcko in already merged PR.
>
> Passed subset of functional tests locally.
>
> Might squeeze a little bit more information into the title?
>
> "#31403 follow-up" -> "qa: Improve framework.generate* enforcement (#31403 follow-up)"
>
> See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-t
...

💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2573398883)
Thanks for the review @i-am-yuvi!

> I haven't tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!

So I learned that it's likely this
https://github.com/bitcoin/bitcoin/blob/41a2ce9b7d7354c633c104bab5653f1f60eb2068/src/test/fuzz/util.cpp#L278-L289
that is preventing the `load_external_block_file` target from crashing on a mac.

📝 hebasto opened a pull request: "depends, NetBSD: Fix `bdb` package compilation with GCC-14"
(https://github.com/bitcoin/bitcoin/pull/31613)
On NetBSD 10, compilation of the `bdb` package with GCC-14 fails with the following error:
```
$ gmake -C depends bdb CC=/usr/pkg/gcc14/bin/gcc
<snip>
./libtool --mode=compile /usr/pkg/gcc14/bin/gcc -c -I. -I../dist/./.. -I/home/hebasto/dev/bitcoin/depends/x86_64-unknown-netbsd10.0/include -D_XOPEN_SOURCE=600 -pipe -std=c11 -O2 -Wno-error=implicit-function-declaration -Wno-error=format-security -Wno-error=implicit-int ../dist/./../env/env_config.c
libtool: compile: /usr/pkg/gcc14/bin/g
...