🤔 vasild reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1465241616)
Approach ACK df76eb535bbd215da17cee8e0f480af9e94e9e70
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1465241616)
Approach ACK df76eb535bbd215da17cee8e0f480af9e94e9e70
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1219678326)
`GetFullOutboundAndManualCount()` will iterate on all nodes, and is called from `ForEachNode()` which iterates on all nodes. This makes it `O(number of nodes ^ 2)`. This will also do a recursive lock on `m_nodes_mutex` and there is an ongoing effort to eradicate recursive locks from the code base.
So, what about collecting the stats (a network->count map) just once, before the `ForEachNode()` call?
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1219678326)
`GetFullOutboundAndManualCount()` will iterate on all nodes, and is called from `ForEachNode()` which iterates on all nodes. This makes it `O(number of nodes ^ 2)`. This will also do a recursive lock on `m_nodes_mutex` and there is an ongoing effort to eradicate recursive locks from the code base.
So, what about collecting the stats (a network->count map) just once, before the `ForEachNode()` call?
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221338648)
This test seems correct but is a bit obscure as it juggles with `vNodes[max_outbound_full_relay - 2]` and `vNodes.back()`. Here it does not check the value of `vNodes[max_outbound_full_relay - 1]->fDisconnect` (it is supposed to be `true`, left unmodified from the previous test).
Maybe it would help a bit if:
* before the newly added test snippet set `vNodes[max_outbound_full_relay - 1]->fDisconnect` to `false`, so that all of them are `false` then the new test starts.
* `back()` is not use
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221338648)
This test seems correct but is a bit obscure as it juggles with `vNodes[max_outbound_full_relay - 2]` and `vNodes.back()`. Here it does not check the value of `vNodes[max_outbound_full_relay - 1]->fDisconnect` (it is supposed to be `true`, left unmodified from the previous test).
Maybe it would help a bit if:
* before the newly added test snippet set `vNodes[max_outbound_full_relay - 1]->fDisconnect` to `false`, so that all of them are `false` then the new test starts.
* `back()` is not use
...
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221406171)
There is no need to collect more than one element in `preferred_networks`, the previous loop can be stopped as soon as a match is found and the shuffling can be done on the order in which networks are iterated (untested):
```cpp
std::optional<Network> CConnman::MaybePickPreferredNetwork()
{
std::array<Network, 5> nets{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
Shuffle(nets.begin(), nets.end(), FastRandomContext());
auto peers_per_net = GetFullOutboundAndManualCounts(
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221406171)
There is no need to collect more than one element in `preferred_networks`, the previous loop can be stopped as soon as a match is found and the shuffling can be done on the order in which networks are iterated (untested):
```cpp
std::optional<Network> CConnman::MaybePickPreferredNetwork()
{
std::array<Network, 5> nets{NET_IPV4, NET_IPV6, NET_ONION, NET_I2P, NET_CJDNS};
Shuffle(nets.begin(), nets.end(), FastRandomContext());
auto peers_per_net = GetFullOutboundAndManualCounts(
...
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221348635)
The return value and the optional output argument are redundant. Better to return `std::optional` and take no arguments:
```cpp
std::optional<Network> MaybePickPreferredNetwork()
```
(would have to be called outside of `else if (...` which I find clearer).
Or drop the optional:
```cpp
bool MaybePickPreferredNetwork(Network& network);
```
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221348635)
The return value and the optional output argument are redundant. Better to return `std::optional` and take no arguments:
```cpp
std::optional<Network> MaybePickPreferredNetwork()
```
(would have to be called outside of `else if (...` which I find clearer).
Or drop the optional:
```cpp
bool MaybePickPreferredNetwork(Network& network);
```
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221384118)
`GetFullOutboundAndManualCount(net)` is called for each network and each call iterates all nodes, collecting stats just for the given network, ignoring the others. It can collect all the stats with one iteration (network->count map) and be called just once, before this loop (vs for each netowork).
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221384118)
`GetFullOutboundAndManualCount(net)` is called for each network and each call iterates all nodes, collecting stats just for the given network, ignoring the others. It can collect all the stats with one iteration (network->count map) and be called just once, before this loop (vs for each netowork).
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221421939)
The way this is implemented would give clearnet 2x chance of being returned over another network with 0 peers. Is this the intention?
For example: let all of IPv4, IPv6 and Tor be reachable and there are 0 peers to each of those networks. Clearnet (IPv4 or IPv6) will be returned in 67% of the cases and Tor in 33%. Should that not be clearnet 50%, Tor 50%?
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1221421939)
The way this is implemented would give clearnet 2x chance of being returned over another network with 0 peers. Is this the intention?
For example: let all of IPv4, IPv6 and Tor be reachable and there are 0 peers to each of those networks. Clearnet (IPv4 or IPv6) will be returned in 67% of the cases and Tor in 33%. Should that not be clearnet 50%, Tor 50%?
⚠️ Brotcrunsher opened an issue: "Alternative read only paths for -blocksdir"
(https://github.com/bitcoin/bitcoin/issues/27833)
### Please describe the feature you'd like to see added.
The Blockchain is growing in size over the years. It would be nice to be able to store it accross multiple hard drives and specify additional blocksdirs that are meant for validation in full nodes. When the client writes a file, it is still written to the -blocksdir, always. When reading a file, it is first looked up in the -blocksdir. When it's not present there, it is looked up in the additional blocksdirs, in order as they are specifie
...
(https://github.com/bitcoin/bitcoin/issues/27833)
### Please describe the feature you'd like to see added.
The Blockchain is growing in size over the years. It would be nice to be able to store it accross multiple hard drives and specify additional blocksdirs that are meant for validation in full nodes. When the client writes a file, it is still written to the -blocksdir, always. When reading a file, it is first looked up in the -blocksdir. When it's not present there, it is looked up in the additional blocksdirs, in order as they are specifie
...
💬 ryanofsky commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#discussion_r1221463118)
Either way seems fine. I suggested it for a followup because it isn't directly related to using a consistent type for transaction sizes. If you will make a follow up PR, you could consider dropping the second commit from this PR to avoid churn, since the second commit 93daff4b4577a07994b57218df9bb83bed7bf743 also isn't directly related to tx sizes. But whatever you want to do is fine.
(https://github.com/bitcoin/bitcoin/pull/23962#discussion_r1221463118)
Either way seems fine. I suggested it for a followup because it isn't directly related to using a consistent type for transaction sizes. If you will make a follow up PR, you could consider dropping the second commit from this PR to avoid churn, since the second commit 93daff4b4577a07994b57218df9bb83bed7bf743 also isn't directly related to tx sizes. But whatever you want to do is fine.
💬 MarcoFalke commented on issue "Alternative read only paths for -blocksdir":
(https://github.com/bitcoin/bitcoin/issues/27833#issuecomment-1580690654)
Not sure if it is worth it to add code for a rarely (or singly) used edge case. As you mention there is RAID. Also, it is possible to purchase a new storage media large enough. Finally, you can use `-prune`.
(https://github.com/bitcoin/bitcoin/issues/27833#issuecomment-1580690654)
Not sure if it is worth it to add code for a rarely (or singly) used edge case. As you mention there is RAID. Also, it is possible to purchase a new storage media large enough. Finally, you can use `-prune`.
💬 petertodd commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221483218)
NACK This wording is misleading. OP_Return outputs only contain data if they contain something in addition to the OP_Return opcode.
A better approach here if we are touching this opcode could be to redefine `-datacarriersize` to be size *in-addition-too* the OP_Return opcode that is allowed. Thus `-datacarriersize=0` would allow 0 bytes in addition to the 1 byte used by the OP_Return opcode.
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221483218)
NACK This wording is misleading. OP_Return outputs only contain data if they contain something in addition to the OP_Return opcode.
A better approach here if we are touching this opcode could be to redefine `-datacarriersize` to be size *in-addition-too* the OP_Return opcode that is allowed. Thus `-datacarriersize=0` would allow 0 bytes in addition to the 1 byte used by the OP_Return opcode.
💬 MarcoFalke commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221497178)
My goal is to document the current behavior and then behavior changes can be done in a follow-up change. Happy to drop this commit or change to a better wording to describe the current behavior, if you have any suggestions.
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221497178)
My goal is to document the current behavior and then behavior changes can be done in a follow-up change. Happy to drop this commit or change to a better wording to describe the current behavior, if you have any suggestions.
🚀 fanquake merged a pull request: "ci: enable AArch64 target in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27824)
(https://github.com/bitcoin/bitcoin/pull/27824)
💬 petertodd commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221516504)
Since you are updating this, you should change the doc text to make clear that the current behavior is a mistake that needs fixing rathre than continue to incorrectly call them data carrier scriptPubKey's.
But it'd be less code churn to just fix the behavior in one shot. We have an ACK on https://github.com/bitcoin/bitcoin/pull/27261, so there is an argument to fix the bare OP_Return case. Rather than merging https://github.com/bitcoin/bitcoin/pull/27261 as-is I could make a pull-req that sim
...
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221516504)
Since you are updating this, you should change the doc text to make clear that the current behavior is a mistake that needs fixing rathre than continue to incorrectly call them data carrier scriptPubKey's.
But it'd be less code churn to just fix the behavior in one shot. We have an ACK on https://github.com/bitcoin/bitcoin/pull/27261, so there is an argument to fix the bare OP_Return case. Rather than merging https://github.com/bitcoin/bitcoin/pull/27261 as-is I could make a pull-req that sim
...
💬 MarcoFalke commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221522467)
> Fixing that error would also allow -datacarrier to be dropped entirely, as it is redundant: -datacarriersize=0 prohibits data carrying outputs just fine.
I think `-datacarrier` can be dropped regardless of whether the behavior is changed. And this would have been my review comment in https://github.com/bitcoin/bitcoin/pull/27261 :)
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221522467)
> Fixing that error would also allow -datacarrier to be dropped entirely, as it is redundant: -datacarriersize=0 prohibits data carrying outputs just fine.
I think `-datacarrier` can be dropped regardless of whether the behavior is changed. And this would have been my review comment in https://github.com/bitcoin/bitcoin/pull/27261 :)
💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1580738352)
Needs rebase?
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1580738352)
Needs rebase?
💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1580740902)
Thanks, I pushed your branch (rebased)
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1580740902)
Thanks, I pushed your branch (rebased)
💬 fanquake commented on issue "Alternative read only paths for -blocksdir":
(https://github.com/bitcoin/bitcoin/issues/27833#issuecomment-1580742109)
Agree with Marco. You can use something like LVM to combine your drives, and then point to a single blocksdir. There's no need for us to introduce complexity to mange this into Bitcoin Core.
(https://github.com/bitcoin/bitcoin/issues/27833#issuecomment-1580742109)
Agree with Marco. You can use something like LVM to combine your drives, and then point to a single blocksdir. There's no need for us to introduce complexity to mange this into Bitcoin Core.
✅ fanquake closed an issue: "Alternative read only paths for -blocksdir"
(https://github.com/bitcoin/bitcoin/issues/27833)
(https://github.com/bitcoin/bitcoin/issues/27833)
💬 ryanofsky commented on pull request "kernel: Remove shutdown globals from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869)
Thanks for following up and incorporating the feedback!
On the topic of how to move forward with these changes, I think we should see if we can find a way to avoid making temporary changes to validation code that are not desirable for the long term. Specifically:
- Having `interrupt_on_fatal_error` and `interrupt_on_condition` ChainStateManager options which are complicated (and very confusing even to me!)
- Giving ChainStateManager a mutable reference to the interrupt object instead of
...
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869)
Thanks for following up and incorporating the feedback!
On the topic of how to move forward with these changes, I think we should see if we can find a way to avoid making temporary changes to validation code that are not desirable for the long term. Specifically:
- Having `interrupt_on_fatal_error` and `interrupt_on_condition` ChainStateManager options which are complicated (and very confusing even to me!)
- Giving ChainStateManager a mutable reference to the interrupt object instead of
...