💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1151986544)
`m_connman.GetNodeCount(ConnectionDirection::In)` includes all inbound connections, even ones that do want transactions relayed to them. IIUC, you want this to be "number of all inbound peers that sent fRelay=true" and the second member of the pair should be "number of all non-erlay inbound peers that sent fRelay=true"

💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in ASAN job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1488682494)
Yeah, I suspect that debian:bookworm will be added earlier by google to the compute images. You can ask Sylvestre to add clang-16 to bookworm, I guess?

💬 dergoegge commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1488686338)
Concept ACK

💬 MarcoFalke commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1488712858)
CI should finish green now

💬 apoelstra commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1488767884)
@S3RK I agree that Sjors' PR does what we want (and more), up to the specific import format, but

* It has not been updated in 18 months and now has many conflicts
* It depends on two other unmerged PRs, one of which is marked draft
* It is itself marked draft
* It is 30 commits long and requires significant reviewer attention

Meanwhile this PR is narrow in scope, doesn't even touch the actual wallet code and therefore won't conflict with ongoing mega-refactors, and is currently up to da
...

📝 fanquake opened a pull request: "guix: use python-minimal (3.9)"
(https://github.com/bitcoin/bitcoin/pull/27361)
This further minifies the Guix release build environment.

📝 fanquake opened a pull request: "test: remove `GetRNGState` lsan suppression"
(https://github.com/bitcoin/bitcoin/pull/27362)
I am no-longer seeing this, testing with the native_asan job over `x86_64` (Ubuntu 22.04) and `aarch64` (Fedora 37). Can anyone recreate the false-positive?

Based on #27360 (which works locally; unclear what we are doing with Cirrus), which I need for working aarch64 builds.

💬 dergoegge commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1152094594)
```suggestion
TRACE6(net, evicted_inbound_connection,
```

The logic in `net` exclusively evicts inbound peers and there is more eviction logic in `net_processing` for outbound connections. Maybe adding separate trace points for both makes sense?

📝 fanquake opened a pull request: "ci: use LLVM/clang-16 in native_fuzz (ASAN) job"
(https://github.com/bitcoin/bitcoin/pull/27363)
Similar to #27298.

💬 fanquake commented on pull request "test: remove `GetRNGState` lsan suppression":
(https://github.com/bitcoin/bitcoin/pull/27362#issuecomment-1488816059)
Switched to basing on #27363.

💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1488816966)
native_fuzz split into #27363, as that is not blocked up upstream image issues/decisions.

💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152104910)
Before this change the value of `m_chainman.m_best_header` was synced with the `UpdateBlockAvailability()` call. Now they are not. Is it safe?

💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152127765)
That's unfortunate that we call a logging function while holding a lock. Maybe
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2697,10 +2696,15 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer,
const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers)
{
- if (peer.nUnconnectingHeaders > 0) {
- LogPrint(
...

👍 brunoerg approved a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214)
re-ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152158505)
nm, it will cause double lock.

💬 TheCharlatan commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1488878073)
Concept ACK

The `python-minimal` derivation is defined in https://github.com/guix-mirror/guix/blob/v1.4.0/gnu/packages/python.scm. The `python-minimal` package inherits from the `python-3`, which in turn inherits from `python-2.7`. If I am reading this correctly, the baseline `python` package requires the following input dependencies: `bzip2, expat, gdbm, libffi, sqlite, openssl, readline, zlib, tcl, tk`, while the `python-minimal` package requires `expat, libffi, openssl, zlib`. Seems like a
...

💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152170209)
Yes that is fine because `m_best_header` is not used by `UpdateBlockAvailability` afaict.

📝 fanquake opened a pull request: "ci: set docker run --ulimit to workaround Valgrind assertion"
(https://github.com/bitcoin/bitcoin/pull/27364)
Running the `native_fuzz_with_valgrind_job`, on aarch64 (Fedora 37), I've seen the following:
```bash
Run addr_info_deserialize with args ['valgrind', '--quiet', '--error-exitcode=1', '/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', '/home/fedora/ci_scratch/ci/scratch/qa-assets/fuzz_seed_corpus/addr_info_deserialize']
valgrind: m_libcfile.c:66 (vgPlain_safe_fd): Assertion 'newfd >= VG_(fd_hard_limit)' failed.


valgrind: m_libcfile
...

💬 glozow commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1488942459)
> @glozow want to take a look at the mempool tests?

LGTM, makes sense to generate 1 to clear mempool after subtests. changing who calls `generate` shouldn't impact mempool_package_limits.
I haven't taken the time to verify that this fixes the test intermittent failure.

💬 Tracachang commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1488988174)
@pinheadmz

I have followed all the steps using the bash commands from your answer and I am unable to create a receiving taproot address from the GUI (it does not show bech2m but it shows both base58 (legacy & p2sh-segwit and bech32) if I select the watch_wallet, however if I select the "original_wallet" it appears in GUI all 4 including bech32m.

In any case, from console I can `getnewaddress "test" "bech32m"` and it works but for some reason not on GUI.

💬 TheCharlatan commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1489001205)
ACK d0e571ebb187

```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
53aa12b7fe92d92eaf81a51a8f5d534f92a2f18f0218f29fdf3221d817a53c04 guix-build-d0e571ebb187/output/aarch64-linux-gnu/SHA256SUMS.part
01467530b06b90114561e86968710250e3648fe4aba5c228781ccf9ac72394c2 guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu-debug.tar.gz
fdf6dfc901e371b4e11a7f69eb2ff9b36a3ac393255862bad1dcd45b
...