💬 Mackain commented on pull request "doc: Minor update to doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1779237826)
@jonatack good point! thanks for the suggestion
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1779237826)
@jonatack good point! thanks for the suggestion
🤔 mzumsande reviewed a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334717617)
Concept ACK
I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?).
But even if it was possible to get these messages repeatedly, it doesn't really make sense to retry more than once.
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334717617)
Concept ACK
I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?).
But even if it was possible to get these messages repeatedly, it doesn't really make sense to retry more than once.
💬 mzumsande commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779260055)
This log error is also shown in the non-GUI case, where it doesn't make any sense because the user didn't get the option to rebuild the block database. Also, to be pedantic, it's misleading in the GUI because we didn't abort a rebuild, but aborted instead of trying it. Maybe it's not necessary at all?
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779260055)
This log error is also shown in the non-GUI case, where it doesn't make any sense because the user didn't get the option to rebuild the block database. Also, to be pedantic, it's misleading in the GUI because we didn't abort a rebuild, but aborted instead of trying it. Maybe it's not necessary at all?
💬 mzumsande commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779222847)
nit: might use "retry with do_reindex set". Otherwise it might sounds a bit like "try turning it off and on again".
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779222847)
nit: might use "retry with do_reindex set". Otherwise it might sounds a bit like "try turning it off and on again".
💬 ryanofsky commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2380253538)
> I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?)
I think it wouldn't retry multiple times because the old code sets `do_reindex = true`
https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/init.cpp#L1643
At least that was my interpretation reviewing t
...
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2380253538)
> I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?)
I think it wouldn't retry multiple times because the old code sets `do_reindex = true`
https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/init.cpp#L1643
At least that was my interpretation reviewing t
...
💬 jonatack commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2380259736)
In this context, concept ack with waiting for a more general solution rather than iterative piecemeal changes, as long as achieving it (via cluster mempool) looks reasonably attainable.
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2380259736)
In this context, concept ack with waiting for a more general solution rather than iterative piecemeal changes, as long as achieving it (via cluster mempool) looks reasonably attainable.
💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1779311123)
Since #30921, we can now make these `constexpr string_view`.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1779311123)
Since #30921, we can now make these `constexpr string_view`.
💬 mzumsande commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2380328047)
> At least that was my interpretation reviewing this earlier (could be mistaken)
Yes, you are right. I thought that maybe there could be some scenarios where even with `do_reindex == true` we could maybe get `ChainstateLoadStatus::FAILURE` again, but after looking more closely I don't see how this could be the case.
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2380328047)
> At least that was my interpretation reviewing this earlier (could be mistaken)
Yes, you are right. I thought that maybe there could be some scenarios where even with `do_reindex == true` we could maybe get `ChainstateLoadStatus::FAILURE` again, but after looking more closely I don't see how this could be the case.
📝 RandyMcMillan opened a pull request: "doc/build-osx.md:brew relinking note"
(https://github.com/bitcoin/bitcoin/pull/30993)
(https://github.com/bitcoin/bitcoin/pull/30993)
💬 RandyMcMillan commented on pull request "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS":
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2380384258)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2380384258)
Concept ACK
💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2380400982)
> For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection.
True. Lets dive a bit deeper - what are the reasons to be concerned about somebody inspecting the traffic to my node?
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2380400982)
> For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection.
True. Lets dive a bit deeper - what are the reasons to be concerned about somebody inspecting the traffic to my node?
💬 vasild commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2380402219)
Concept ACK
When `addnode` exits silently I have always found it annoying to have to go to the debug log to find out what happened.
Isn't one of the `addnode` variants asynchronous?
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2380402219)
Concept ACK
When `addnode` exits silently I have always found it annoying to have to go to the debug log to find out what happened.
Isn't one of the `addnode` variants asynchronous?
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2380411313)
`6d0c5247eb...752fbab26a`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2380411313)
`6d0c5247eb...752fbab26a`: rebase due to conflicts
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2380413586)
`489c2477e7...dba7835386`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2380413586)
`489c2477e7...dba7835386`: rebase due to conflicts
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1779377188)
Made it a `constexpr auto` (I don't want non-owning fields) https://github.com/bitcoin/bitcoin/compare/1dd79a32c9c6e0c6cf4ecef00b56972574babced..cb9be6729c4e27bf2d6d703c0d454a22cbcdb6e1
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1779377188)
Made it a `constexpr auto` (I don't want non-owning fields) https://github.com/bitcoin/bitcoin/compare/1dd79a32c9c6e0c6cf4ecef00b56972574babced..cb9be6729c4e27bf2d6d703c0d454a22cbcdb6e1
💬 hebasto commented on pull request "doc/build-osx.md:brew relinking note":
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1779449856)
Is this issue CMake specific?
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1779449856)
Is this issue CMake specific?
📝 itornaza opened a pull request: "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0"
(https://github.com/bitcoin/bitcoin/pull/30994)
This is an attempt to fix the issue https://github.com/bitcoin/bitcoin/issues/30978 by conditionally defining the necessary tools when building on macos and is based on the Solution 3 described in https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2379782629.
(https://github.com/bitcoin/bitcoin/pull/30994)
This is an attempt to fix the issue https://github.com/bitcoin/bitcoin/issues/30978 by conditionally defining the necessary tools when building on macos and is based on the Solution 3 described in https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2379782629.
💬 jonatack commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2380636483)
> make getpeerinfo accept a peer id param to just investigate that one
Sounds useful.
Didn't re-read the discussion, but I'm not sure that no connection should throw, in favor of returning an "error" field (or "errors" object).
@willcl-ark do you plan to update here soon?
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2380636483)
> make getpeerinfo accept a peer id param to just investigate that one
Sounds useful.
Didn't re-read the discussion, but I'm not sure that no connection should throw, in favor of returning an "error" field (or "errors" object).
@willcl-ark do you plan to update here soon?
💬 jonatack commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1779484434)
I might be too focused on the risks rather than benefits, but this is unfortunately only one brigading campaign by one or more prominent bitcoiners on social media away from being potentially adopted by many nodes.
With -onlynet, the network partition risk is in practice opt-in, i.e. chosen by a minority subset for themselves. Whereas here, the network partition risk could be imposed on a future minority of users that are running earlier versions of Bitcoin Core. This seems at odds with main
...
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1779484434)
I might be too focused on the risks rather than benefits, but this is unfortunately only one brigading campaign by one or more prominent bitcoiners on social media away from being potentially adopted by many nodes.
With -onlynet, the network partition risk is in practice opt-in, i.e. chosen by a minority subset for themselves. Whereas here, the network partition risk could be imposed on a future minority of users that are running earlier versions of Bitcoin Core. This seems at odds with main
...
💬 jonatack commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1779484507)
```suggestion
* connections on Tor/I2P/CJDNS can be v1 or v2 connections.
```
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1779484507)
```suggestion
* connections on Tor/I2P/CJDNS can be v1 or v2 connections.
```