💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3551316361)
review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a 🏀

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 7f318e1dd049
...

💬 maflcko commented on pull request "rpc: Add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3551325875)
the commit message is wrong?

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3551348815)
Latest push rebased to resolve conflict with #31734. + Added explicit test coverage of `operator=(Node&&)` (https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3490078075).

💬 Sjors commented on issue "rfc: virtio-vsock for RPC and IPC":
(https://github.com/bitcoin/bitcoin/issues/33897#issuecomment-3551516882)
> it would not really be safe without authentication

And encryption, since we can't use SSL.

💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3551580243)
@plebhash indeed I'm not worried about client-side memory, it's node that could get into memory trouble if there's a long block interval with lots of mempool churn.

> it's extremely unlikely that mempool would sustain sub-second changes for over 2h

In the context of templates we only care about about at the top of the mempool. For the mempool as a whole there can be dozens of transactions per second, see e.g. #28592.

@ryanofsky how would the node go about tracking the memory footprint of clie
...

💬 Sjors commented on pull request "mining: add requestedOutputs field, e.g. for merged mining":
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3551630141)
> is this aimed at Sv2?

Any consumer of the interface, but I mostly have Stratum v2 in mind.

> if it requires a spec change so this can be integrated

As @ajtowns points out in the https://github.com/stratum-mining/sv2-spec/issues/167 thread it's a bit odd that @TheBlueMatt states that merged-mining is "a key goal of Sv2" while the spec doesn't even mention it.

In principle the change here doesn't need a spec change. Miners who want merged-mining will patch _something_, and this just
...

🚀 fanquake merged a pull request: "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026)

💬 fanquake commented on pull request "ci: Consistenly only cache on the default branch":
(https://github.com/bitcoin/bitcoin/pull/33905#issuecomment-3551729334)
cc @willcl-ark @m3dwards

💬 fanquake commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3551737960)
cc @m3dwards @willcl-ark

💬 fanquake commented on issue "depends: fallback server missing Qt downloads":
(https://github.com/bitcoin/bitcoin/issues/33898#issuecomment-3551739190)
cc @hebasto

🤔 maflcko reviewed a pull request: "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3481321005)
forgot to send the nits, basically just a test nit to check the returned size, but just a nit

💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2540945544)
Maybe just de-duplicate with `FromBytes` in the other test file?

💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2540940425)
nit: Here and above in the last commit: Why not check the size is equal to 10 each time?

👍 willcl-ark approved a pull request: "ci: Consistenly only cache on the default branch"
(https://github.com/bitcoin/bitcoin/pull/33905#pullrequestreview-3481757071)
ACK fa411f938e4796cf3806f234c9787b7c79492cc2

We could probably drop `github.event_name != 'pull_request' &&`? Feels like it might be redundant with `github.ref_name == github.event.repository.default_branch`...

📝 hodlinator opened a pull request: "ci: Make the max number of commits tested explicit"
(https://github.com/bitcoin/bitcoin/pull/33909)
Gives less of a false sense of security.

💬 maflcko commented on pull request "ci: Consistenly only cache on the default branch":
(https://github.com/bitcoin/bitcoin/pull/33905#issuecomment-3551842381)
Yeah, I guess even if a pull is opened from a branched named equally to the default branch, the ref_name is rewritten to `<pr_number>/merge`, according to https://docs.github.com/en/actions/reference/workflows-and-actions/contexts#github-context

However, it can't hurt to check twice explicitly, so I'll leave as-is for now :sweat_smile:

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2541346873)
Hmm, looks like this happens because there are still connections between the bitcoin node and the SOCKS5 proxy and then the bitcoin node is restarted, closing the connections. Or rather, the bitcoin node is just establishing a new connection and is interrupted in the middle of the SOCKS5 handshake because of the `self.restart_node()` call.

This is expected and the test succeeds rightfully, however that printout looks odd and scary. Should be handled more gracefully. One way would be to conver
...

💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3551887180)
I have pushed the [commit](https://github.com/bitcoin/bitcoin/pull/30442/commits/bb57dd5e84108126d41cc03a8dba89c0d9f09a39) on top to not invalidate previous ACKs:
```patch
- explicit SaltedOutpointHasher(bool deterministic = false);
+ SaltedOutpointHasher(bool deterministic = false);
```

I would appreciate a re-review @achow101, @ismaelsadeeq, @jonatack, @laanwj, @ryanofsky, @sipa.

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541360330)
7f17cf6d6164460086afebbb0f921a7af4475bf9:

This example uses Makefile variable `CC` and `CXX`, but the section title refers to "Environment Variables". Both approaches work, of course, but the section could be made clearer.

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541379255)
7f17cf6d6164460086afebbb0f921a7af4475bf9

This comes across as a bit discouraging and confusing. When I say “I build depends with Clang”, I’m referring to the host-specific packages. I’m not concerned about which compiler is used for the native tools, as long as they build without issues.