💬 achow101 commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1634547609)
Allowing new types of descriptors only disallows downgrading of wallets that have imported them. Downgrade protection is automatic for such wallets since descriptor wallets were introduced.
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1634547609)
Allowing new types of descriptors only disallows downgrading of wallets that have imported them. Downgrade protection is automatic for such wallets since descriptor wallets were introduced.
💬 hebasto commented on pull request "multiprocess: Add basic spawn and IPC support":
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262872076)
I apologize for revisiting this old PR, but I couldn't resist asking to clarify things for me.
As far as I understand, `interfaces::MakeNodeInit` returns a `std::unique_ptr` that owns an instance of `init::BitcoindInit` or throws an exception. Given this understanding, it seems unclear how the `return exit_status;` statement could ever execute.
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262872076)
I apologize for revisiting this old PR, but I couldn't resist asking to clarify things for me.
As far as I understand, `interfaces::MakeNodeInit` returns a `std::unique_ptr` that owns an instance of `init::BitcoindInit` or throws an exception. Given this understanding, it seems unclear how the `return exit_status;` statement could ever execute.
💬 achow101 commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1634647753)
ACK e7cf8657e1165ea5da3911a9e543837cd8938f97
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1634647753)
ACK e7cf8657e1165ea5da3911a9e543837cd8938f97
🚀 achow101 merged a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411)
(https://github.com/bitcoin/bitcoin/pull/27411)
💬 ryanofsky commented on pull request "multiprocess: Add basic spawn and IPC support":
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262895010)
> As far as I understand, `interfaces::MakeNodeInit` returns a `std::unique_ptr` that owns an instance of `init::BitcoindInit` or throws an exception. Given this understanding, it seems unclear how the `return exit_status;` statement could ever execute.
There are two `interfaces:MakeNodeInit` functions. The one in [`src/init/bitcoind.cpp`](https://github.com/bitcoin/bitcoin/blob/master/src/init/bitcoind.cpp#L41-L44) which is linked into `bitcoind` will leave exit_status unchanged, as you say.
...
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262895010)
> As far as I understand, `interfaces::MakeNodeInit` returns a `std::unique_ptr` that owns an instance of `init::BitcoindInit` or throws an exception. Given this understanding, it seems unclear how the `return exit_status;` statement could ever execute.
There are two `interfaces:MakeNodeInit` functions. The one in [`src/init/bitcoind.cpp`](https://github.com/bitcoin/bitcoin/blob/master/src/init/bitcoind.cpp#L41-L44) which is linked into `bitcoind` will leave exit_status unchanged, as you say.
...
💬 hebasto commented on pull request "multiprocess: Add basic spawn and IPC support":
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262899118)
Right. Thank you.
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262899118)
Right. Thank you.
💬 recursive-rat4 commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634706063)
utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1
`CXXFLAGS` was set in https://github.com/bitcoin/bitcoin/pull/24735 without intention to disable optimizations.
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634706063)
utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1
`CXXFLAGS` was set in https://github.com/bitcoin/bitcoin/pull/24735 without intention to disable optimizations.
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262927938)
Ok adding two commits to handle these two affected areas. For the GUI I've written logic that accommodates the new Proxy features but I'll have to submit a follow-up PR to enable unix socket path configuration from the GUI as a separate element from address:port proxy configuration.
So with this only PR, the user can only set unix sockets outside the GUI, and in the network tab they will see `"Options set in this dialog are overridden by the command line..."` anyway.
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262927938)
Ok adding two commits to handle these two affected areas. For the GUI I've written logic that accommodates the new Proxy features but I'll have to submit a follow-up PR to enable unix socket path configuration from the GUI as a separate element from address:port proxy configuration.
So with this only PR, the user can only set unix sockets outside the GUI, and in the network tab they will see `"Options set in this dialog are overridden by the command line..."` anyway.
💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634714444)
rfm or is something missing here?
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634714444)
rfm or is something missing here?
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262930258)
Ok I'll put this statement after `SetNonBlocking()` but I think the Socks5 stuff should live in a follow-up PR?
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262930258)
Ok I'll put this statement after `SetNonBlocking()` but I think the Socks5 stuff should live in a follow-up PR?
💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262932762)
Thanks, done. Also used C++ 17 structured binding declaration to get the map key-value.
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262932762)
Thanks, done. Also used C++ 17 structured binding declaration to get the map key-value.
💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262934214)
```suggestion
const auto it_ins{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
```
Haven't tracked down which C++20 LWG paper this was. Let me know if someone finds something.
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262934214)
```suggestion
const auto it_ins{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
```
Haven't tracked down which C++20 LWG paper this was. Let me know if someone finds something.
💬 fanquake commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634723100)
> [Use of -O2 and above is not recommended as Memcheck occasionally reports uninitialised-value errors which don't really exist.](https://valgrind.org/docs/manual/quick-start.html)
I guess this doesn't matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.
I think there's another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634723100)
> [Use of -O2 and above is not recommended as Memcheck occasionally reports uninitialised-value errors which don't really exist.](https://valgrind.org/docs/manual/quick-start.html)
I guess this doesn't matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.
I think there's another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.
💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634727453)
> I guess this doesn't matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.
Only for gcc, see https://github.com/bitcoin/bitcoin/issues/27741
> I think there's another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.
Right. Probably not worth to spend time here, with the switch to cmake. But that makes me wonder how cmake handles this.
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634727453)
> I guess this doesn't matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.
Only for gcc, see https://github.com/bitcoin/bitcoin/issues/27741
> I think there's another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.
Right. Probably not worth to spend time here, with the switch to cmake. But that makes me wonder how cmake handles this.
👍 hebasto approved a pull request: "guix: Remove librt usage from release binaries"
(https://github.com/bitcoin/bitcoin/pull/28069#pullrequestreview-1529066233)
ACK 8f6f0d81ee3a9ea582e9c9cf986613da86760098
(https://github.com/bitcoin/bitcoin/pull/28069#pullrequestreview-1529066233)
ACK 8f6f0d81ee3a9ea582e9c9cf986613da86760098
🤔 furszy reviewed a pull request: "[WIP] descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1529085037)
> @furszy I think there is only a downgrade issue, no? If someone were to import a sh(raw(...)) as watch-only address in a descriptor wallet after allowing that, and then downgrades to older software which does not support it.
Yeah good @sipa and @achow101. We are sync then.
My point was more about agreeing that the bug fix should go first, so it can be backported for users of v24 and v25. Preventing them from entering an unrecoverable state after migration.
And then we can move forward
...
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1529085037)
> @furszy I think there is only a downgrade issue, no? If someone were to import a sh(raw(...)) as watch-only address in a descriptor wallet after allowing that, and then downgrades to older software which does not support it.
Yeah good @sipa and @achow101. We are sync then.
My point was more about agreeing that the bug fix should go first, so it can be backported for users of v24 and v25. Preventing them from entering an unrecoverable state after migration.
And then we can move forward
...
💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634760577)
Hello, just wondering if there is something left from my side, or is this ready for review and merging?
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634760577)
Hello, just wondering if there is something left from my side, or is this ready for review and merging?
💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634766029)
>
Bitcoin development is always bottlenecked and all changes no matter how small must thoroughly reviewed for and allowed time for social consensus since every change could potentially cost people real world value. You can work on other issues and/or await more feedback.
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634766029)
>
Bitcoin development is always bottlenecked and all changes no matter how small must thoroughly reviewed for and allowed time for social consensus since every change could potentially cost people real world value. You can work on other issues and/or await more feedback.
💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634768717)
Ahh I see! Thanks for clearing it up :D I was just worried if I needed to change something from my side hehe
Thanks for all the help in this PR everyone, really learnt a great deal from each one of you!
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634768717)
Ahh I see! Thanks for clearing it up :D I was just worried if I needed to change something from my side hehe
Thanks for all the help in this PR everyone, really learnt a great deal from each one of you!
💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1634769028)
> Running this test failed on my device not sure if it's from this PR though.
It looks like you're missing the 25.0 binaries. The test is expecting to find 25.0's bitcoind at `/home/abubakarsadiq/Desktop/bitcoin/releases/v25.0/bin/bitcoind` but doesn't.
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1634769028)
> Running this test failed on my device not sure if it's from this PR though.
It looks like you're missing the 25.0 binaries. The test is expecting to find 25.0's bitcoind at `/home/abubakarsadiq/Desktop/bitcoin/releases/v25.0/bin/bitcoind` but doesn't.