💬 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)
👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2132676568)
Code review ACK 6a5408dd7da4c31283cb4f632b3a7aa692a53ca3. This is a really nice improvement over existing shell scripts, with a surprisingly simple implementation. I left some suggestions, but none are important and this looks good to merge in its current form.
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2132676568)
Code review ACK 6a5408dd7da4c31283cb4f632b3a7aa692a53ca3. This is a really nice improvement over existing shell scripts, with a surprisingly simple implementation. I left some suggestions, but none are important and this looks good to merge in its current form.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648974658)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
I don't think it's sufficient to call `connman.SetNetworkActive()` manually in this function, because the InvalidateBlock and ReconsiderBlock functions called here can both throw exceptions and leave the network disabled after this RPC call if there is an error. Would suggest making a NetworkDisable disable_network{connman}; RAII class that disables the network in its constructor and enables it in
...
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648974658)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
I don't think it's sufficient to call `connman.SetNetworkActive()` manually in this function, because the InvalidateBlock and ReconsiderBlock functions called here can both throw exceptions and leave the network disabled after this RPC call if there is an error. Would suggest making a NetworkDisable disable_network{connman}; RAII class that disables the network in its constructor and enables it in
...
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649320665)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Maybe say "generate it yourself from another node" to be clear you need a different node to generate it.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649320665)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Maybe say "generate it yourself from another node" to be clear you need a different node to generate it.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648975246)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
s/disc/disk/
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648975246)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
s/disc/disk/
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649333656)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
For a followup, it might be convenient if height value defaulted to the last height value which could actually be loaded as a snapshot. It could also accept a "latest" string value in case the user does want to generate the latest snapshot.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649333656)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
For a followup, it might be convenient if height value defaulted to the last height value which could actually be loaded as a snapshot. It could also accept a "latest" string value in case the user does want to generate the latest snapshot.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649335464)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
It seems racy to unlock cs_main before calling CreateUTXOSnapshot. Not sure if there is a way to implement this in a race-free way without looping though. Could be something to follow up on later.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649335464)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
It seems racy to unlock cs_main before calling CreateUTXOSnapshot. Not sure if there is a way to implement this in a race-free way without looping though. Could be something to follow up on later.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649328350)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Is it true that no pruning will take place between the snapshot and the tip? I haven't experimented but it looks like GetPruneRange will only return a minimum prune height for the snapshot chainstate, not the background chainstate.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649328350)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Is it true that no pruning will take place between the snapshot and the tip? I haven't experimented but it looks like GetPruneRange will only return a minimum prune height for the snapshot chainstate, not the background chainstate.