💬 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.
(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.
(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
...
(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.
(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.
(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
(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.
(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 ?
(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
(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
...
(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)
(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.
```
(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;
```
(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;
```
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649113447)
If it happens that `sizeof(in_addr)` is not equal to `RTA_PAYLOAD(rta_gateway)` then I think it is better to not allow this to continue to the `memcpy()` because it could read past the end of the buffer. If we don't want to stop a release/production program for this with an `assert()`, then log + `return std::nullopt;` is maybe better then copying random undefined bytes around and interpreting them as an IP address.
(same for some `Assume()`s in the Apple implementation)
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649113447)
If it happens that `sizeof(in_addr)` is not equal to `RTA_PAYLOAD(rta_gateway)` then I think it is better to not allow this to continue to the `memcpy()` because it could read past the end of the buffer. If we don't want to stop a release/production program for this with an `assert()`, then log + `return std::nullopt;` is maybe better then copying random undefined bytes around and interpreting them as an IP address.
(same for some `Assume()`s in the Apple implementation)
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649171297)
style: `{` should be on the same line as `if` or `for` (also in a few places below).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649171297)
style: `{` should be on the same line as `if` or `for` (also in a few places below).
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649094052)
Now that #30202 has been merged this can be:
```diff
// Create a netlink socket.
- const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
- if (s < 0) {
+ auto sock{CreateSock(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
+ if (!sock) {
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", NetworkErrorString(errno));
return std::nullopt;
}
- Sock sock{static_cast<SOCKET>(s)};
// Send request.
```
and replace `s
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1649094052)
Now that #30202 has been merged this can be:
```diff
// Create a netlink socket.
- const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
- if (s < 0) {
+ auto sock{CreateSock(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
+ if (!sock) {
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", NetworkErrorString(errno));
return std::nullopt;
}
- Sock sock{static_cast<SOCKET>(s)};
// Send request.
```
and replace `s
...
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183111646)
Rebased after #30314 and #30202. Incorporated the (light) transport refactor from #30315 to make sure that didn't break anything.
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183111646)
Rebased after #30314 and #30202. Incorporated the (light) transport refactor from #30315 to make sure that didn't break anything.
💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649237537)
This trips up the address sanitizer:
https://github.com/bitcoin/bitcoin/actions/runs/9616699499/job/26526819621?pr=30315#step:6:4956
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649237537)
This trips up the address sanitizer:
https://github.com/bitcoin/bitcoin/actions/runs/9616699499/job/26526819621?pr=30315#step:6:4956
💬 sipa commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649241912)
Pretty sure you mean `MakeByteSpan(msg)`, not `MakeByteSpan(msg.data())`.
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649241912)
Pretty sure you mean `MakeByteSpan(msg)`, not `MakeByteSpan(msg.data())`.
💬 stickies-v commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183182799)
> 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.
What about a combination of extending the zmq and rest interface and/or using `longpollid`, as suggested e.g. [here](https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587485219)? It seems like that would already improve things significantly, without having to add the encryption and transport layer?
> It needs to do this be
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183182799)
> 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.
What about a combination of extending the zmq and rest interface and/or using `longpollid`, as suggested e.g. [here](https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587485219)? It seems like that would already improve things significantly, without having to add the encryption and transport layer?
> It needs to do this be
...