🤔 ryanofsky reviewed a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2225643495)
Thanks for the review!
Updated efdc120795e7c3ce914ee0677c3b69803ea5de1e -> cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 ([`pr/ipc-bind.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.2) -> [`pr/ipc-bind.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-bind.2..pr/ipc-bind.3)) with suggestions
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2225643495)
Thanks for the review!
Updated efdc120795e7c3ce914ee0677c3b69803ea5de1e -> cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 ([`pr/ipc-bind.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.2) -> [`pr/ipc-bind.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-bind.2..pr/ipc-bind.3)) with suggestions
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707371794)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253
> If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don't exist, they will be created. Should that be documented here?
Sure added both things.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707371794)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253
> If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don't exist, they will be created. Should that be documented here?
Sure added both things.
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372358)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253
> Nit: Could be declared `static`?
Sure, added static
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372358)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253
> Nit: Could be declared `static`?
Sure, added static
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372573)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706927164
> Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the "good" value.
I could be missing something, but I think all the functions in this file are returning or throwing as soon as there is an error.
In this ca
...
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372573)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706927164
> Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the "good" value.
I could be missing something, but I think all the functions in this file are returning or throwing as soon as there is an error.
In this ca
...
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707373325)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707288885
> Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.
Thanks, added
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707373325)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707288885
> Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.
Thanks, added
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372017)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707213519
> Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?
Added comment. You are right that gui does not listen on a socket, but `bitcoin-gui` accepts all the same arguments `bitcoin-node` does and more. If `bitcoin-gui` is started and a node processs is no
...
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372017)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707213519
> Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?
Added comment. You are right that gui does not listen on a socket, but `bitcoin-gui` accepts all the same arguments `bitcoin-node` does and more. If `bitcoin-gui` is started and a node processs is no
...
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707373805)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707289963
> Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.
Nice catch, these are not needed. An initial version of this test did use a thread but it's gone and these are no longer necessary.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707373805)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707289963
> Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.
Nice catch, these are not needed. An initial version of this test did use a thread but it's gone and these are no longer necessary.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707536386)
Typo nit: s/if a relative paths are/if relative paths/
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707536386)
Typo nit: s/if a relative paths are/if relative paths/
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707538495)
Ah, that makes sense, I did not think about this function being expanded over time.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707538495)
Ah, that makes sense, I did not think about this function being expanded over time.
💬 davidgumberg commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1707541879)
I think this is right, just wanted to make a note that I believe @mzumsande earlier [comment](https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1655477509) is relevant here. Namely, that it's not necessary to check for freshness here if we cannot have fresh-but-not-dirty entries, and even if we could, I'm not sure why we wouldn't remove them from the cache in this instance. But I think this should be addressed by any revival of #18746.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1707541879)
I think this is right, just wanted to make a note that I believe @mzumsande earlier [comment](https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1655477509) is relevant here. Namely, that it's not necessary to check for freshness here if we cannot have fresh-but-not-dirty entries, and even if we could, I'm not sure why we wouldn't remove them from the cache in this instance. But I think this should be addressed by any revival of #18746.
💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2274027378)
> Is there another crash you were referring to? The first commit message says:
We'd also crash trying to reorg because this scenario is not handled.
I didn't mean an assert with that, just a shutdown with a fatal error. I tested this in a functional test, if I recall correctly the following happened:
After loading the snapshot, I would connect a peer with a higher-work tip branching off below the snapshot height. On master we'd download blocks from this peer, and as soon as we'd have a an
...
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2274027378)
> Is there another crash you were referring to? The first commit message says:
We'd also crash trying to reorg because this scenario is not handled.
I didn't mean an assert with that, just a shutdown with a fatal error. I tested this in a functional test, if I recall correctly the following happened:
After loading the snapshot, I would connect a peer with a higher-work tip branching off below the snapshot height. On master we'd download blocks from this peer, and as soon as we'd have a an
...
💬 boring877 commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707579334)
why not just keep it ? why act like communist and remove things you desire ~
alot of data and infrastructure already on testnet3, why force people your own desires ~
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707579334)
why not just keep it ? why act like communist and remove things you desire ~
alot of data and infrastructure already on testnet3, why force people your own desires ~
💬 boring877 commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707583093)
> @fjahr I guess because I feel it's hard to make concrete promises about something that far out. If we _know_ we will remove testnet3 in v30, we should just deprecate it in v28 already (including a `-deprecatedrpc` requirement), rather than saying it will be deprecated. If not, the best we can do is announce an intent that it will be removed in the future, without concrete timing.
what you mean testnet3 gone ? you seriously wants to delete it~ you can use testnet3 as reference for old script
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707583093)
> @fjahr I guess because I feel it's hard to make concrete promises about something that far out. If we _know_ we will remove testnet3 in v30, we should just deprecate it in v28 already (including a `-deprecatedrpc` requirement), rather than saying it will be deprecated. If not, the best we can do is announce an intent that it will be removed in the future, without concrete timing.
what you mean testnet3 gone ? you seriously wants to delete it~ you can use testnet3 as reference for old script
...
💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707589818)
> what you mean testnet3 gone ? you seriously wants to delete it~ you can use testnet3 as reference for old scripts thats been used ...
No worries, you can continue to use Testnet 3 as long as you desire. It’s just not going to be supported by Bitcoin Core starting with some upcoming version.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707589818)
> what you mean testnet3 gone ? you seriously wants to delete it~ you can use testnet3 as reference for old scripts thats been used ...
No worries, you can continue to use Testnet 3 as long as you desire. It’s just not going to be supported by Bitcoin Core starting with some upcoming version.
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707596947)
> > @fjahr I guess because I feel it's hard to make concrete promises about something that far out. If we _know_ we will remove testnet3 in v30, we should just deprecate it in v28 already (including a `-deprecatedrpc` requirement), rather than saying it will be deprecated. If not, the best we can do is announce an intent that it will be removed in the future, without concrete timing.
>
> what you mean testnet3 gone ? you seriously wants to delete it~ you can use testnet3 as reference for old
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707596947)
> > @fjahr I guess because I feel it's hard to make concrete promises about something that far out. If we _know_ we will remove testnet3 in v30, we should just deprecate it in v28 already (including a `-deprecatedrpc` requirement), rather than saying it will be deprecated. If not, the best we can do is announce an intent that it will be removed in the future, without concrete timing.
>
> what you mean testnet3 gone ? you seriously wants to delete it~ you can use testnet3 as reference for old
...
✅ vostrnad closed a pull request: "Add a `-permitbarepubkey` option"
(https://github.com/bitcoin/bitcoin/pull/29309)
(https://github.com/bitcoin/bitcoin/pull/29309)
💬 ariard commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2274085829)
@petertodd, Yeah it might be too much zeal, yet I’ve been surprised when the option was introduced with `24.x` how much services were still claiming to rely zero-conf acceptance. Same, I’m not aware of any bitcoin industry traffic with real-traffic actually accepting unconfirmed transactions (apart of few LSPs with zero-conf, it’s a paradigm of its own and people running that are more savvy about unconf txn risks due to channel funding flow).
The proposed alternative wording “Bitcoin Core rem
...
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2274085829)
@petertodd, Yeah it might be too much zeal, yet I’ve been surprised when the option was introduced with `24.x` how much services were still claiming to rely zero-conf acceptance. Same, I’m not aware of any bitcoin industry traffic with real-traffic actually accepting unconfirmed transactions (apart of few LSPs with zero-conf, it’s a paradigm of its own and people running that are more savvy about unconf txn risks due to channel funding flow).
The proposed alternative wording “Bitcoin Core rem
...
🤔 ryanofsky reviewed a pull request: "kernel, refactor: return error status on all fatal errors"
(https://github.com/bitcoin/bitcoin/pull/29700#pullrequestreview-2225894783)
Rebased e22926a383223b286fe97a12ea4efaa81414bb41 -> 3ab9a46ac67964c9d5ad54120b680b5eee7f16f0 ([`pr/fatalresult.20`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.20) -> [`pr/fatalresult.21`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.21), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.20-rebase..pr/fatalresult.21)) due to conflicts with #30517 and #30497
(https://github.com/bitcoin/bitcoin/pull/29700#pullrequestreview-2225894783)
Rebased e22926a383223b286fe97a12ea4efaa81414bb41 -> 3ab9a46ac67964c9d5ad54120b680b5eee7f16f0 ([`pr/fatalresult.20`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.20) -> [`pr/fatalresult.21`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.21), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.20-rebase..pr/fatalresult.21)) due to conflicts with #30517 and #30497
💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#discussion_r1707623223)
re: https://github.com/bitcoin/bitcoin/pull/29700#discussion_r1693183422
> Seems fragile to use `util::Result<InterruptResult>`. Wrapping one result into another is fragile, because call-sites may easily forget to unwrap the inner result, especially if the inner result is otherwise `void`. My recommendation would be to treat interruption as `util::Error`, so that callers don't miss it if they check for errors (but forget to check for interruption), but are free to check it, if they inspect th
...
(https://github.com/bitcoin/bitcoin/pull/29700#discussion_r1707623223)
re: https://github.com/bitcoin/bitcoin/pull/29700#discussion_r1693183422
> Seems fragile to use `util::Result<InterruptResult>`. Wrapping one result into another is fragile, because call-sites may easily forget to unwrap the inner result, especially if the inner result is otherwise `void`. My recommendation would be to treat interruption as `util::Error`, so that callers don't miss it if they check for errors (but forget to check for interruption), but are free to check it, if they inspect th
...
💬 Retropex commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-2274112090)
Why closing this? @vostrnad
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-2274112090)
Why closing this? @vostrnad