💬 hebasto commented on pull request "guix: Update `python-lief` package to 0.13.2":
(https://github.com/bitcoin/bitcoin/pull/27813#issuecomment-1607382638)
Guix builds:
```
018716ffdc0c60b1dfa27f95d1fb4e03d0e37f50cfc325ee35d200039b3f419e guix-build-529c92e837b2/output/aarch64-linux-gnu/SHA256SUMS.part
bb6e74d4806879560e7b6e50bce4b47b383ed5d763e6d25feac64b0c944cc036 guix-build-529c92e837b2/output/aarch64-linux-gnu/bitcoin-529c92e837b2-aarch64-linux-gnu-debug.tar.gz
2e6fc404211c94bc9a0265a783fabcd3c0bfddb9a87322b612185489ef6264d9 guix-build-529c92e837b2/output/aarch64-linux-gnu/bitcoin-529c92e837b2-aarch64-linux-gnu.tar.gz
b0f0d9e2e4466cc301c
...

💬 hebasto commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607389698)
> the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp?rgh-link-date=2023-05-16T15%3A52%3A00Z#L1021).

However, [Xcode 13 Release Notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-13-release-notes) state:
> All programs and dylibs built with a deployment target of macOS 12 or iOS 15 or later now use the chained fixups format. This uses different load commands and
...

💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#discussion_r1242138308)
The `-X` option is still needed as it was suggested [earlier](https://github.com/bitcoin/bitcoin/pull/27099#discussion_r1240844088):
```suggestion
zip -X -r $@ $(APP_DIST_DIR)
```

💬 hebasto commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607427931)
It would be nice if someone confirms that Guix binaries run on macOS 11 BigSur.

💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242157733)
In f9db8fce:

Missed to set the error here.
```suggestion
if (err.empty()) err = e.what();
```

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242173358)
Hmm, just realized that this is marked as resolved but is not resolved in the code. I am not sure, did you intentionally leave it as it is in the code now or did you forget to drop the optional like I suggested above?

💬 TheCharlatan commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607505539)
Guix build
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
109ed45706b8cbdc897b6c51efa0247f8e8f1e6bb35c57e69f30093b62189a51 guix-build-3df60704661c/output/aarch64-linux-gnu/SHA256SUMS.part
4e28463f1c074c7c121abd5fbad89aa2269c5978829ccd4898d33a21eedfeb4f guix-build-3df60704661c/output/aarch64-linux-gnu/bitcoin-3df60704661c-aarch64-linux-gnu-debug.tar.gz
f20d074f0529202274a40f0530f79f601f5c575f1cd0addc099418719982485e
...

🤔 vasild reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1498508181)
Approach ACK 564bf851cccd34e859f23b6e6b00debebba9a3c2

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242180428)
style: use clang-format, this should be:

```cpp
bool IsManualOrFullOutboundConn() const
{
switch (m_conn_type) {
case ConnectionType::INBOUND:
...
```

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242190047)
unrelated whitespace change

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242196931)
I suggested `std::array` in another comment as a way to workaround the lack of mutex protection. Now that this is protected by a mutex and no need to tweak or workaround concurrency issues, I think the neatest option is:

```suggestion
std::unordered_map<Network, size_t> m_network_conn_counts GUARDED_BY(m_nodes_mutex) = {};
```

The `std::array` usage adds the requirement for sequential values of `NET_*` that are without gaps and start from `0` and have `NET_MAX`. That makes the code m
...

💬 hebasto commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607542766)
Guix (self-signed) `bitcoin-qt` tested on macOS Ventura 13.4.1 (Apple M1).

📝 MarcoFalke opened a pull request: " ci: Start with clean env "
(https://github.com/bitcoin/bitcoin/pull/27976)
Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564529747

💬 Xekyo commented on pull request "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee":
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607580462)
> Fixes #27913, although this does bring up a larger question of how we should handle such large feerates in fuzzing.

My understanding is that we should fuzz as broadly as possible, but our fuzzers should probably catch before overflows of `CAmount` occur. If I didn’t err on my napkin math that still allows us to do full block transactions at 92 ₿ per vB (9,223,372,036 ṩ/vB). So… maybe we just limit feerates to 9 billion ṩ/vB in fuzzers?

💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1607633650)
If I read correctly it happened _after_ IBD.

I guess `-debug=net` would useful here, if you can ever reproduce it. It might give us a hint which message we were about to react to.

As an aside, since you configured the wallet, be careful loading descriptor wallets on master until #27915 is resolved.

💬 Xekyo commented on pull request "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee":
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607637305)
We use the `fee_a × vsize_b > fee_b × vsize_a` trick in a bunch of places. On the other hand, fee is calculated from `vsize × feerate`. So, in fuzzing based on a feerate input, we should limit the feerate to prevent integer overflows with `int64_t` to `max_feerate = int64_t_max / (max_vsize²)`?

If we assume that our code does not have to deal with transactions greater than 100_000 vB:

`9_223_372_036_854_775_807÷(100_000²) = 922_337_203_685 [ṩ/kvB] ≈ 922_337_203 [ṩ/vB] ≈ 9_223 ₿/kvB`

If
...

💬 Sjors commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1607638095)
> While reviewing it, discovered that descriptors containing a key with an origin are actually incorrect.

Glad the tests were useful :-)

> The tests must pass in v25 with these commits.

Thanks, I was thinking of testing that as well, but couldn't figure out how to do so trivially with my own commit.

💬 Sjors commented on issue "wallets created on master get corrupted when processed with v25":
(https://github.com/bitcoin/bitcoin/issues/27915#issuecomment-1607644227)
I'll review the apostropocalypse fix soon, thanks.

💬 MarcoFalke commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1607655279)
> I guess -debug=net would useful here, if you can ever reproduce it. It might give us a hint which message we were about to react to.

If you do this, make sure to add additional logging (maybe `nBytes`, `data.size()`, and `node.nSendOffset` after the `Send()`?) in the function that asserted, because there is none.

💬 Sjors commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1607656508)
@kristapsk I removed two of the three hashboards and set the target consumption to 100W. It's practice it's more like 130, but that's fine, and not too noisy (mine came with BrainsOS+ installed)