💬 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
...
💬 sipa commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183201374)
> But if the bitcoind interfaces are lacking for sv2tpd to be a separate project, I think we should first exhaust the possibilities of improving the interfaces before just incorporating (part of) the downstream project into Bitcoin Core.
FWIW, my view is that indeed the current Bitcoin Core interfaces are inadequate for implementing template providers, and while it's probably possible to make a number of improvements to existing ones to improve the situation, the result will either be far fro
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183201374)
> But if the bitcoind interfaces are lacking for sv2tpd to be a separate project, I think we should first exhaust the possibilities of improving the interfaces before just incorporating (part of) the downstream project into Bitcoin Core.
FWIW, my view is that indeed the current Bitcoin Core interfaces are inadequate for implementing template providers, and while it's probably possible to make a number of improvements to existing ones to improve the situation, the result will either be far fro
...
💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649293172)
I did! Did I? Should be fixed now.
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649293172)
I did! Did I? Should be fixed now.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183273298)
> I'm not sure I understand this. This PR is not an sv2 discussion, so if my understanding is wrong we don't need to talk about it here, but are you saying the pool will approve a proposed block template without having/keeping all the transactions? That seems undesired?
The [Job Declaration Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md) has a param `async_mining_allowed` which, if set, allows miners to immediately submit work even before the tem
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183273298)
> I'm not sure I understand this. This PR is not an sv2 discussion, so if my understanding is wrong we don't need to talk about it here, but are you saying the pool will approve a proposed block template without having/keeping all the transactions? That seems undesired?
The [Job Declaration Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md) has a param `async_mining_allowed` which, if set, allows miners to immediately submit work even before the tem
...
👍 theuni approved a pull request: "[26.x] upnp: fix build with miniupnpc 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30319#pullrequestreview-2133269679)
ACK 10413ac46c07d3a45dc9d71818f59ffdb1129032
(https://github.com/bitcoin/bitcoin/pull/30319#pullrequestreview-2133269679)
ACK 10413ac46c07d3a45dc9d71818f59ffdb1129032
💬 luke-jr commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1649343879)
This should probably reflect the OS bitcoind was compiled for
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1649343879)
This should probably reflect the OS bitcoind was compiled for
💬 luke-jr commented on pull request "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core"":
(https://github.com/bitcoin/bitcoin/pull/30308#issuecomment-2183325152)
>It can be changed just for consistency, and to prevent similar issues in future when devs copy-paste code.
Changing it would *break* that test.
I don't think there's anything more to do here at the moment. (CI is a good idea, but seems suited to a different PR)
(https://github.com/bitcoin/bitcoin/pull/30308#issuecomment-2183325152)
>It can be changed just for consistency, and to prevent similar issues in future when devs copy-paste code.
Changing it would *break* that test.
I don't think there's anything more to do here at the moment. (CI is a good idea, but seems suited to a different PR)