💬 meglio commented on issue "signrawtransactionwithwallet fails with signed non-wallet inputs and breaks the existing signatures":
(https://github.com/bitcoin/bitcoin/issues/26385#issuecomment-1488621015)
Am I correct to assume this is still a bug awaiting a fix?

💬 fanquake commented on pull request "ci: use LLVM/clang-16 in ASAN job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1488669908)
> You will probably need to do the same for fuzz asan?

Will add.

> Also, need to edit Cirrus CI to do the same

Can we even do this for native_asan given there is no 2304-lto image listed here: https://cloud.google.com/compute/docs/images/os-details#ubuntu_lts ?

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1151979732)
```suggestion
inbounds_fanouted += pnode->IsInboundConn() && pnode->m_relays_txs && !m_txreconciliation->IsPeerRegistered(pnode->GetId());
```
Right? because other wise this includes erlay peers which we are not flooding to by default.

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