💬 maflcko commented on pull request "ci: Rewrite test-each-commit as py script":
(https://github.com/bitcoin/bitcoin/pull/32680#issuecomment-2948223714)
Hmm, yeah the boilerplate overhead is a bit unfortunate. I switched to python, which still has downsides such as https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852746032, but it seems manageable here. Also, updated the pull description.

I think the danger of Bash, that it looks harmless and superficially simple and correct, but in many such "harmless" constructs can silently drop a failing return status is reason enough to prefer a slightly more verbose language. Maybe rust is to
...

💬 Muniru0 commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2948296702)

> > [@fanquake](https://github.com/fanquake) why not just updating the the docs to point to the online versions. e.g:
> > **Replace:** `see doc/cjdns.md`
> > With: `see https://github.com/bitcoin/bitcoin/blob/master/doc/cjdns.md`
> > This make's sure that:
> >
> > 1. **Accessibility**: Users can easily access the documentation regardless of how they obtained the binaries.
> > 2. **Up-to-Date Information**: Linking to the online repository ensures that users are directed to the most current ver
...

💬 Muniru0 commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2948298581)
@Zeegaths check I hope you check the comment from @brunoerg before proceeding.

🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2903739719)
Approach ACK 7d301184016a3f59c2e363dff631263cdbe21da0

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2131479604)
Because the context will always be set, ie will never be `nullptr`, then it is better to use a reference here instead of a pointer: `NodeContext& m_context;`

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2131632124)
`std::map` would keep the entries sorted by the key in alphabetical order and lookup time is `O(log(number of elements in the map))` whereas `std::unordered_map` does not maintain an order and has a lookup time of `O(1)`. We do not need any order here, right? This probably is irrelevant since we are only going to put a bunch of entries in this map, but anyway, if there is no reason to use `std::map` I guess we should default to `std::unordered_map`.

(from commit e95c6f5b65 `http: Implement HT
...

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2131642866)
This returns a reference (or a "pointer") to the outside world of an element from the private `m_map` member. That map is not immutable. What happens if the strings it contains get moved around if the map is internally resized when adding new entries? Also there is the `HTTPHeaders::Remove()` method which deletes entries from the map, rendering any returned pointers by `Find()` as dangling.

54a36093f7 `http: using string_view to avoid unnecessary copy in Headers` -- if there is no any measura
...

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2131476990)
Might use `std::chrono::milliseconds` for the type of the argument instead of `int64_t`:

```cpp
HTTPRPCTimer(NodeContext* context, std::function<void()>& func, std::chrono::milliseconds after)
{
context->scheduler->scheduleFromNow(func, after);
```
(in commit 2950597694 `use CScheduler for HTTPRPCTimer`)

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2131490238)
Can simplify the code a few lines below:

```diff
- std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6));
+ std::string_view strUserPass64 = TrimStringView(strAuth.substr(6));
```
(in commit 54a36093f7 `http: using string_view to avoid unnecessary copy in Headers`)

💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2131708208)
But `LockCoin` isn't loading the from the database, which is what makes this call site confusing.

⚠️ Sjors opened an issue: "depends: OpenBSD needs gcc (instruction) for libevent"
(https://github.com/bitcoin/bitcoin/issues/32691)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

The current depends instructions for OpenBSD do not mention `gcc`, which is required to build libevent.

A simple `pkg_add gcc` doesn't cut it either, because that only install `egcc`: https://unix.stackexchange.com/questions/554752/no-gcc-executable-after-pkg-add-gcc-on-openbsd

### Expected behaviour

Explain what the user needs to do to build depends (or change depends).

### Steps to r
...

🤔 hebasto reviewed a pull request: "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)"
(https://github.com/bitcoin/bitcoin/pull/32690#pullrequestreview-2904142213)
Concept ACK 8713e8060d504f561fed705b4aa5af7b96c36e75.

https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15485455235/job/43599018182 looks good.

💬 hebasto commented on issue "depends: OpenBSD needs gcc (instruction) for libevent":
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2948467284)
I'm not sure. The following seems [working](https://github.com/hebasto/bitcoin-core-nightly/blob/2973f9d010f6581a7663d5cea4246ee7f6dc4070/.github/workflows/openbsd.yml#L104):
```yml
prepare: pkg_add bash curl gmake gtar-- cmake git ccache python py3-zmq
```

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2131744550)
The way I understood it is that `LoadLockedCoin` is loading the data into memory and the value (at least parts of it) passed to it in the first argument comes from the database.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2131751750)
Unrelated nit but mentioning because I noticed it now, the `value` is not used in this load func, only `key` is used.

```diff
- [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {
+ [] (CWallet* pwallet, DataStream& key, DataStream&, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {
```

🤔 hebasto reviewed a pull request: "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)"
(https://github.com/bitcoin/bitcoin/pull/32690#pullrequestreview-2904181437)
Why 8713e8060d504f561fed705b4aa5af7b96c36e75 is necessary for libmultiprocess, but not for other packages?

💬 fanquake commented on pull request "[28.x] 28.2rc2":
(https://github.com/bitcoin/bitcoin/pull/32684#issuecomment-2948529722)
Guix Build:
```bash
be30f094366c71bd7251247319dbb0cf1b5674d13e210685328daff1bf855766 guix-build-3b05c498a08e/output/aarch64-linux-gnu/SHA256SUMS.part
1823e9b3d7c97a533ab77540095223bc48759216b9e9fe757f24575f2323680f guix-build-3b05c498a08e/output/aarch64-linux-gnu/bitcoin-3b05c498a08e-aarch64-linux-gnu-debug.tar.gz
8fd4d3a844e845ae13a95d165ae2cd1fea735295c24bc68b3fd28cb91f59c397 guix-build-3b05c498a08e/output/aarch64-linux-gnu/bitcoin-3b05c498a08e-aarch64-linux-gnu.tar.gz
b86f8537ceb5f364
...

🤔 janb84 reviewed a pull request: "ci: Rewrite test-each-commit as py script"
(https://github.com/bitcoin/bitcoin/pull/32680#pullrequestreview-2904243167)
After some contemplating I think the Python code is the better way to go. The Rust code was written a bit verbose and could have been made smaller, but Python seems like the more suitable tool for this task because:

- Its more readable for CI maintenance, more people have python knowledge.
- Python integrates better with existing CI tooling (e.g. is already used in the ci.yml)
- Lower barrier to entry for contributors who need to debug CI issues.

And Python still gives us the advantages
...

💬 Sjors commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2948584686)
I tested this on top of #31802 on a VM running OpenBSD 7.7 (dropping `NO_IPC` for OpenBSB).

Without the patch I get a bunch of errors like:

```
[ 2%] Building CXX object src/kj/CMakeFiles/kj.dir/cidr.c++.o
/root/src/bitcoin/depends/work/build/aarch64-unknown-openbsd7.7/native_capnp/1.1.0-f5dbdc0b2f0/src/kj/cidr.c++:116:71: error: member access into incomplete type 'const struct sockaddr_in6'
otherBits = reinterpret_cast<const struct sockaddr_in6*>(addr)->sin6_addr.s6_addr;
```
...

💬 dergoegge commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2131827333)
nit: Same as above, the Assume can be inline