✅ maflcko closed an issue: "Чудовий початок роботи 👌💪"
(https://github.com/bitcoin/bitcoin/issues/30628)

:lock: fanquake locked an issue: "Чудовий початок роботи 👌💪"
(https://github.com/bitcoin/bitcoin/issues/30628)

👍 maflcko approved a pull request: "chainparams: Handle Testnet4 in GetNetworkForMagic"
(https://github.com/bitcoin/bitcoin/pull/30625#pullrequestreview-2231783071)
review ACK b0ec8716bf27335686471e0ae4c6a34f9a08f33c

Left a style-nit to avoid a copy and to make the code shorter, but only if you re-touch.

💬 maflcko commented on pull request "chainparams: Handle Testnet4 in GetNetworkForMagic":
(https://github.com/bitcoin/bitcoin/pull/30625#discussion_r1712953968)
style-nit: I know you copied this from the surrounding code, but creating a copy is not needed to compare two const references, and if the size of the second range in `std::equal` is different, the result is either wrong or undefined behavior (UB/uninitialized read). See also the notes section (https://en.cppreference.com/w/cpp/algorithm/equal#Notes), which says "... When comparing entire containers for equality, operator== for the corresponding container are usually preferred".

So that'd be:
...

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712955667)
> The cmake -S src/bench -B build command fails.

It seems to me [they're using `ninja`](https://github.com/bitcoin-cash-node/bitcoin-cash-node/blob/master/doc/benchmarking.md?plain=1#L8) for these tasks, i.e. [`ninja bench-bitcoin`](https://github.com/bitcoin-cash-node/bitcoin-cash-node/blob/master/doc/ninja_targets.md?plain=1#L251) instead of raw cmake.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712956905)
> > The cmake -S src/bench -B build command fails.
>
> It seems to me [they're using `ninja`](https://github.com/bitcoin-cash-node/bitcoin-cash-node/blob/master/doc/benchmarking.md?plain=1#L8) for these tasks, i.e. [`ninja bench-bitcoin`](https://github.com/bitcoin-cash-node/bitcoin-cash-node/blob/master/doc/ninja_targets.md?plain=1#L251) instead of raw cmake.

It seems this discussion has gone off topic :)

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712957036)
Didn't realize that, please resolve it in that case

👍 rkrux approved a pull request: "BlockAssembler: return selected packages vsize and feerate"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2231801330)
tACK [f581657](https://github.com/bitcoin/bitcoin/commit/f581657b3a1acd0fc57dc9461d3a7e0835e17a24)

`make, make check` are successful. Left a suggestion.
Thanks for keeping the PR short and focussed.

💬 rkrux commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1712971476)
IMO, depending upon finding **only** the feeRate in the vector of tuples gets rid of some level of robustness in the verification here because it ignores the possibility of duplicate feeRates, and makes the tester/author wary of writing the test in a way that ensures there are no duplicates.
How about **also** checking for the packageSize as well?

💬 fjahr commented on pull request "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files":
(https://github.com/bitcoin/bitcoin/pull/30265#issuecomment-2282736669)
Code review ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d

Also verified manually that the added tests do cover the change of behavior.

💬 Sjors commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2282740748)
```
2024-08-10T19:39:49Z [snapshot] computing UTXO stats for background chainstate to validate snapshot - this could take a few minutes
Killed
```

I think this happened because I used a RAM drive for pruned sync and it ran out of space (a Guix build I ran at the same time failed too). Would be nice to fail more gracefully, but I don't think it's related to the specific snapshot here.

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712974890)
How much rewrite do we need to do here?
I have pushed https://github.com/hebasto/bitcoin/pull/322 to synchronize on the style, let me know how you imagine it.

💬 m3dwards commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2282747080)
ACK 2bca4af323c5b505689c5203e6ddba8becac5dc8

Tested on Debian x86.

With the suppression disabled I got:

```shell
Error: libbitcoin_consensus_a-pubkey.o depends on libbitcoin_util_a-strencodings.o symbol 'std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)'
```

⚠️ maprezentalok opened an issue: ""heapleakdetection" entry in registry for bitcoin-qt.exe"
(https://github.com/bitcoin/bitcoin/issues/30629)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

there is an entry indicating there is a heap leak in bitcoin-qt.exe:

[HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\RADAR\HeapLeakDetection\DiagnosedApplications\bitcoin-qt.exe]
"LastDetectionTime"=hex(b):ce,61,4c,3c,47,bc,da,01

unfortunately no details, also it could be qt related? i hope so :D

### Expected behaviour

expected to free memory as soon as it can be freed

### Steps to reprod
...

📝 hebasto opened a pull request: "doc: Update ccache website link"
(https://github.com/bitcoin/bitcoin/pull/30630)

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1713012418)
> * https://github.com/bitcoin/bitcoin/blob/14093d5d243f6eb9cfef721c80f92848d95032ee/doc/productivity.md?plain=1#L68

See https://github.com/hebasto/bitcoin/pull/323.

👍 tdb3 approved a pull request: "doc: Update ccache website link"
(https://github.com/bitcoin/bitcoin/pull/30630#pullrequestreview-2231883345)
ACK fec74a8bcb29e7cdb1ef21c68c7c8c30c4386ca7
Looks like ccache.samba.org 301 redirects to ccache.dev (checked in browser dev mode).

👍 theStack approved a pull request: "chainparams: Handle Testnet4 in GetNetworkForMagic"
(https://github.com/bitcoin/bitcoin/pull/30625#pullrequestreview-2231901768)
ACK b0ec8716bf27335686471e0ae4c6a34f9a08f33c

👍 tdb3 approved a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2231909317)
ACK 30f701317b4a674fea422766ac3ee179c9c6f554

Light code review. On the PR branch, created a blank legacy wallet, opened in bitcoin-qt. Saw that the migrate option was available and performed the migration.


💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-2282993104)
Thanks, Russ, I made both suggested changes (force pushed).