💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2183003317)
utACK da205dda14d7e71fe0567944117362c1b820ba8f

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649141459)
639be6df22800d2e2a170c1af74d51951597834c: this `namespace` is outdated, since this isn't part of the `node` anymore.

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2183005331)
@vasild thank you for your review, all comments addressed.

👍 hebasto approved a pull request: "ci: add option for running tests without volume"
(https://github.com/bitcoin/bitcoin/pull/30310#pullrequestreview-2132950784)
ACK da205dda14d7e71fe0567944117362c1b820ba8f.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1649143746)
Without this change, when there's no ARM machine, it remains pending forever.

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2183009446)
With bigger cache size:

```shell
ccache version 4.9.1
Cacheable calls: 770 / 782 (98.47%)
Hits: 769 / 770 (99.87%)
Direct: 769 / 769 (100.0%)
Preprocessed: 0 / 769 ( 0.00%)
Misses: 1 / 770 ( 0.13%)
Uncacheable calls: 12 / 782 ( 1.53%)
Local storage:
Cache size (GB): 0.2 / 0.2 (96.47%)
Hits: 769 / 770 (99.87%)
Misses: 1 / 770 ( 0.13%)
```

💬 mzumsande commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2183037276)
> I actually thought we did this already some time ago and got confused about that, see [#29996 (comment)](https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1646164004) Is this what you had in mind as well @mzumsande ?

I opened #30320 with my suggestion - I think we have to use `m_best_header` for this and can't use the active chain which only has blocks that were connected (and therefore fully available at that point).

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1649164186)
It was working but I agree handling the error is safer.

Also I was wrong that it would need to handle the specific "address family for hostname not supported" error.

Implemented your suggestion of just returning from the lambda.

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2183040402)
Thanks for your review @vasild, should have addresses all of your comments.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183041904)
> upgrading the existing RPC method, or adding a new one that's minimal in scope and not SV2 opinionated

I don't think our RPC is suitable for this, because it's inherently poll based. We want to push out new block templates as soon as something better is found.

Another major change compared to the `getblocktemplate` RPC is that the TP hold on to templates for a while (currently until after the next block is found, though could be shorter). It needs to do this because solutions for custom
...

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2183053147)
ASan job has gone from 79 minutes down to 46 minutes with cache.

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1649181039)
Thanks fixed.

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1649181380)
Also added a test for this

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1649181913)
Fixed, and also add a test case for it.

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1649184476)
leaving this as is, as I suspect there might be other places that needs to be replaced also.
this and others can come in a separate PR ?

🤔 ismaelsadeeq reviewed a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2133019836)
Approach ACK

💬 paplorinc commented on pull request "WIP Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1649187336)
@fanquake, is it possible that [this way of benchmarking](https://github.com/bitcoin/bitcoin/commit/619d5691c20bee5d08be2ce85aafa2cb570dbca4#diff-722a565cc61ff748b555e7ed62c0affd13618f67c693524edc351c7b4bf20247R67) isn't actually representative?
I tried comparing code that was obviously faster according to the benchmark by running the before and after code explicitly many times in a test and measuring total time - and the new version seems to reflect it more accurately.
I tried it with `ankerl
...

🤔 vasild reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2132874534)
Posting review midway, up to 237792601fb911cf2e5abebc9226d63f4cd35cec (incl)

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649097270)
```suggestion
// Whether to use the first 4 or 16 bytes from request.dst_data.
```

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649167543)
nit, can drop the parameter name for an unused parameter and the `void` cast:
```suggestion
static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t)
{
return std::nullopt;
```