✅ maflcko closed an issue: "depends: `native_libmultiprocess` fails to build on OpenBSD"
(https://github.com/bitcoin/bitcoin/issues/33219)

💬 maflcko commented on issue "depends: `native_libmultiprocess` fails to build on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/33219#issuecomment-3223048498)
Closing for now as "fixed"

💬 maflcko commented on issue "build: indefinite mpgen hang on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33176#issuecomment-3223079751)
On current master this will now fail configure:


```
...
cmake -B build -DENABLE_IPC=ON
...
-- Checking for module 'capnp'
-- Found capnp, version 0.8.0
-- Checking for module 'capnp-rpc'
-- Found capnp-rpc, version 0.8.0
-- Checking for module 'capnp-json'
-- Found capnp-json, version 0.8.0
-- Checking for module 'kj'
-- Found kj, version 0.8.0
-- Checking for module 'kj-async'
-- Found kj-async, version 0.8.0
-- Checking for module 'kj-http'
-- Found kj-http, version 0.8.0
-- Chec
...

💬 maflcko commented on issue "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added":
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3223172462)
The test is re-using "unused" p2p ports after the 6 nodes. On master it is using 4 ports and you are using another 6, so `6+4+6=16`, which is greater than 12, no?

💬 0xB10C commented on issue "tracing: issue running `contrib/tracing/log_utxos.bt`":
(https://github.com/bitcoin/bitcoin/issues/33227#issuecomment-3223275608)
Can not reproduce with bpftrace `v0.22.1`.

Can reproduce with bpftrace `v0.23.2` on both `aarch64` and `x86_64`.

Given the `reg type unsupported for arg#0` error, I assume it is related to how we read the txid (arg0). I looked into it a bit, but haven't been to get it to work.

```
ERROR: Error loading BPF program for BEGIN_1.
Kernel error log:
processed 104 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1


ERROR: Error loading BPF program for usdt__nix_sto
...

💬 Sjors commented on issue "build: indefinite mpgen hang on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33176#issuecomment-3223277365)
I'm still investigating whether Ubuntu can be pursuaded to bump this. I received some replies on https://answers.launchpad.net/ubuntu/+source/capnproto/+question/822317 but haven't gotten around to studying those.

💬 0xB10C commented on issue "tracing: issue running `contrib/tracing/log_utxos.bt`":
(https://github.com/bitcoin/bitcoin/issues/33227#issuecomment-3223326346)
The bpftrace script `connectblock_benchmark.bt` works. It is similar but reads a block hash instead of a txid. However, not the difference between the arg0 of `block_connected` (`8@-2432(%rbp`) and e.g. arg0 of `utxocache::add` (`8@%r15`). Maybe that's the cause for the `reg type unsupported for arg#0` error.


```
$ readelf -n bitcoind
...
stapsdt 0x0000007a NT_STAPSDT (SystemTap probe descriptors)
Provider: validation
Name: block_connected
Location: 0x00000000004b394
...

💬 maflcko commented on issue "build: indefinite mpgen hang on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33176#issuecomment-3223350766)
The current IPC interface is trusted, and I don't think this is going to change after feature-freeze. Also, https://ubuntu.com/security/CVE-2022-46149 specifically says "low priority" and `ignored` for jammy, so I don't think Ubuntu is going to change the package.

Not sure how common Ubuntu Jammy is and how many people compile from source, but https://snapcraft.io/bitcoin-core lists (by frequency):

Ubuntu 24.04
Ubuntu 22.04
Ubuntu 20.04
Ubuntu 25.04

🤔 hodlinator reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3022243737)
Concept ACK 598bee6bd590757565d2564ae86cf46b5eea4399

### Concept

Agree with exploring this avenue of introducing `SockMan` without touching `CConnman`. Given @theuni's comment in https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374, I'm still curious how he would suggest improving it as far as event-loop/send-receive/threading goes.

### Commit approach

Think it's fine so far to pair the commits (copy+modernize, ...). It enables reviewers to review the copying and the
...

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2288381255)
Could we avoid public data in the initial version?
```C++
void InterruptNet() { interruptNet(); }

const std::vector<std::shared_ptr<Sock>>& ListenSockets() const { return m_listen; }
```
Alternatively make them `protected` and expose them in `TestSockMan`.

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2288986758)
Might as well:
```suggestion
std::vector<std::pair<Id, CService>> m_connections GUARDED_BY(m_connections_mutex);

// Received data is written here by the SockMan I/O thread
// and tested by the main thread.
Mutex m_received_mutex;
std::unordered_map<Id, std::vector<uint8_t>> m_received GUARDED_BY(m_received_mutex);
```

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2288991450)
nit:
```suggestion
}
virtual void EventGotEOF(Id id) override {}
virtual void EventGotPermanentReadError(Id id, const std::string& errmsg) override {}
```

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2288996230)
nits:
1. Should be snake_case.
2. Could be put with the block that uses it.

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2289014362)
More modern:
```suggestion
std::shared_ptr<DynSock::Pipes> ConnectClient(std::span<const std::byte> data);
```

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2289081739)
nit: No need to cast to `void*`:
```suggestion
ssize_t bytes_read = pipes->send.GetBytes(actually_received.data(), expected_response_size);
```

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2293246152)
nit:
```suggestion
* Used to generate ids for new connections.
```

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2293235155)
nit: Maybe update/remove year? (Not a copyright lawyer).

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2293305352)
Could assert that various conditions are true:
```suggestion
virtual ~SockMan()
{
assert(!m_thread_socket_handler.joinable()); // Missing call to JoinSocketsThreads()
assert(m_connected.empty()); // Missing call to CloseConnection()
assert(m_listen.empty()); // Missing call to StopListening()
}
```
https://en.cppreference.com/w/cpp/thread/thread/join.html#Postconditions

💬 maflcko commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#discussion_r2300508652)
the todo refers to the value of `m_pay_tx_fee`, not to the hardcoded `0` here. Also, see https://github.com/bitcoin/bitcoin/pull/32138/files

💬 maflcko commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#discussion_r2300510188)
this is just removing magic values in one place and adding them in another. what is the point?