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

💬 maflcko commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#issuecomment-3223533891)
Thanks, but the value of purely LLM generated "vibe" pull requests is limited, because:

* The "author" does not understand the changes and can not reply to code review feedback.
* There are more than 300 pull requests (most written by real humans) waiting for review.
* Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.

My recommendati
...

✅ maflcko closed a pull request: "wallet: Replace fee magic numbers with named constants"
(https://github.com/bitcoin/bitcoin/pull/33254)

💬 maflcko commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3223540909)
I think this was fixed and can be closed?

✅ maflcko closed a pull request: "doc: add Linux GUI runtime instructions to doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/33197)

💬 maflcko commented on pull request "doc: add Linux GUI runtime instructions to doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/33197#issuecomment-3223543034)
Closing for now, as this was fixed already.

💬 ryanofsky commented on issue "build: indefinite mpgen hang on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33176#issuecomment-3223640756)
From https://ubuntu.com/security/CVE-2022-46149 it seems to say ubuntu has looked at the issue and ignoring it in LTS releases ("Ignored" value in Status table), because the fix changes [changes the ABI](https://github.com/capnproto/capnproto/compare/v0.8.0..v0.8.1#diff-22b74c4b9a5b4d48aece4e5b3b4e73a0a680be156c8921c1d135c1c1cac3d8d1R1235) and could maybe break applications compiled against the 0.8.0 version of the headers but using the 0.8.1 runtime library (IIUC, I'm not exactly sure what they
...

⚠️ fanquake opened an issue: "ci: tidy job is producing output"
(https://github.com/bitcoin/bitcoin/issues/33256)
master (6ca6f3b37b992591726bd13b494369bee3bd6468) https://cirrus-ci.com/task/5658586602274816:
```bash
[17:17:19.943] [108/715][22.0s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/echo.capnp.proxy-server.c++
[17:17:34.032] /usr/include/kj/async-inl.h:609:37: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorRes
...