💬 kevkevinpal commented on pull request "test: remove unused code in `wallet_fundrawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652725607)
> > Not exactly sure why the unused vars were added initially
>
> Not sure if it might worth to add the asserts, it seems to be a leftover from a copy/paste since the functions (in general) are similar.
Ya that makes sense, lgtm
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652725607)
> > Not exactly sure why the unused vars were added initially
>
> Not sure if it might worth to add the asserts, it seems to be a leftover from a copy/paste since the functions (in general) are similar.
Ya that makes sense, lgtm
💬 luke-jr commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1275618446)
This will need `aRules` in `rpc/mining.cpp` updated
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1275618446)
This will need `aRules` in `rpc/mining.cpp` updated
🤔 luke-jr requested changes to a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1548838256)
`-blocksonly` docs need updating
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1548838256)
`-blocksonly` docs need updating
💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275643872)
`node_services` has been discarded here, likely as an oversight trying to rebase it past 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67.
Best approach I see is to export it outward, ensuring callers need to handle the API change.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275643872)
`node_services` has been discarded here, likely as an oversight trying to rebase it past 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67.
Best approach I see is to export it outward, ensuring callers need to handle the API change.
🤔 ajtowns reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1548859239)
utACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe
Various nits only. I think adding some redundant net-category debug logs about trying to make ipv6 connections even on systems with no ipv6 connectivity is fine given it's at about the same rate as there's log entries for the extra block relay only connections anyway.
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1548859239)
utACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe
Various nits only. I think adding some redundant net-category debug logs about trying to make ipv6 connections even on systems with no ipv6 connectivity is fine given it's at about the same rate as there's log entries for the extra block relay only connections anyway.
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275642065)
dev notes says `++i` preferred over `i++`, here and elsewhere
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275642065)
dev notes says `++i` preferred over `i++`, here and elsewhere
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275642506)
Having a map where a `std::array` would be fine is kind of lame... That said, this is the same setup as in addrman_impl.h for `m_network_counts`.
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275642506)
Having a map where a `std::array` would be fine is kind of lame... That said, this is the same setup as in addrman_impl.h for `m_network_counts`.
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275648306)
Could be a `const` member function, if you weren't using `map::operator[]`. That would mean either switching to an array, where you can use `[]` for just a lookup, or doing a manual find:
```c++
if (auto it = m_network_conn_counts.find(net) && it != m_network_conn_counts.end()) {
return it->second > 0;
}
return false;
```
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275648306)
Could be a `const` member function, if you weren't using `map::operator[]`. That would mean either switching to an array, where you can use `[]` for just a lookup, or doing a manual find:
```c++
if (auto it = m_network_conn_counts.find(net) && it != m_network_conn_counts.end()) {
return it->second > 0;
}
return false;
```
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275653315)
Maybe report the `preferred_net` name here as well?
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275653315)
Maybe report the `preferred_net` name here as well?
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275652683)
Isn't `network_peer == true` the same as `preferred_net != std::nullopt` ? Could drop the bool.
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275652683)
Isn't `network_peer == true` the same as `preferred_net != std::nullopt` ? Could drop the bool.
🤔 theStack reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1548892755)
Regarding the stream API discussion, I'd also prefer simplicity over efficiency for now and hence vote for keeping the implementation as-is; the potential advantage of "slightly lower latency when receiving a large P2P message" doesn't seem to outweigh the increased complexity / review power needed, even if it would mostly affect test code. @jamesob made some convincing points for that and I'm also sharing the following sentiment:
> For me, V2 is a very important feature to get deployed. I woul
...
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1548892755)
Regarding the stream API discussion, I'd also prefer simplicity over efficiency for now and hence vote for keeping the implementation as-is; the potential advantage of "slightly lower latency when receiving a large P2P message" doesn't seem to outweigh the increased complexity / review power needed, even if it would mostly affect test code. @jamesob made some convincing points for that and I'm also sharing the following sentiment:
> For me, V2 is a very important feature to get deployed. I woul
...
💬 luke-jr commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1275671827)
This seems like a very serious bug that should have blocked the merge... :|
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1275671827)
This seems like a very serious bug that should have blocked the merge... :|
💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275674549)
>maybe mentioning that this aligns DisconnectNode and the disconnectnode rpc.
Internal APIs really don't seem like they belong here
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275674549)
>maybe mentioning that this aligns DisconnectNode and the disconnectnode rpc.
Internal APIs really don't seem like they belong here
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1652836631)
Updated to remove some unnecessary constructor changes and misleading comments associated with them.
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1652836631)
Updated to remove some unnecessary constructor changes and misleading comments associated with them.
💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677257)
Why not drop this entirely? Seems like a bug specifying an IP won't disconnect all nodes connecting from that IP...
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677257)
Why not drop this entirely? Seems like a bug specifying an IP won't disconnect all nodes connecting from that IP...
💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677709)
In fact, maybe this should be split into a bugfix commit before adding subnet support.
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677709)
In fact, maybe this should be split into a bugfix commit before adding subnet support.
💬 luke-jr commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1652845994)
>Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511.
If it's going to be used for this, it shouldn't be hidden/test-only...
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1652845994)
>Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511.
If it's going to be used for this, it shouldn't be hidden/test-only...
💬 luke-jr commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1275685570)
Is there a reason we shouldn't just support this? Normally it is okay to pass switch options in any order.
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1275685570)
Is there a reason we shouldn't just support this? Normally it is okay to pass switch options in any order.
💬 MarcoFalke commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275760519)
> It's false by default and it has been set up for tests that were already using it.
You are also setting it for tests (all nodes in the test) that only have it set on one node. Not sure about that.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275760519)
> It's false by default and it has been set up for tests that were already using it.
You are also setting it for tests (all nodes in the test) that only have it set on one node. Not sure about that.
👍 MarcoFalke approved a pull request: "test, rpc: invalid sighashtype coverage"
(https://github.com/bitcoin/bitcoin/pull/28166#pullrequestreview-1549053986)
nit: I think it is fine to squash all the sighhashtype commits into one. They all do the same thing, and splitting it just makes review work harder, because at a minimum for each commit there is additional overhead for reviewers to check that a commit doesn't add a backdoor and removes it in the next commit.
(https://github.com/bitcoin/bitcoin/pull/28166#pullrequestreview-1549053986)
nit: I think it is fine to squash all the sighhashtype commits into one. They all do the same thing, and splitting it just makes review work harder, because at a minimum for each commit there is additional overhead for reviewers to check that a commit doesn't add a backdoor and removes it in the next commit.