💬 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.

💬 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?

💬 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?

💬 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.

💬 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.

💬 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.

💬 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.

👍 hebasto approved a pull request: "guix: Remove librt usage from release binaries"
(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
...

💬 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?

💬 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.

💬 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!

💬 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.

💬 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#discussion_r1262968377)
I don't think the specific keys are relevant.

💬 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#discussion_r1262971716)
I would guess that it was used in the up-downgrade test but was accidentally removed at some point. It isn't necessary anymore either as that test is now automated to go over all of the previous versions so the hardcoded specific wallet versions are no longer necessary.

💬 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#discussion_r1262972014)
Will do if I need to retouch.

💬 achow101 commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1634808459)
If this is ready for review, please remove "[WIP]" from the title.

🤔 furszy reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1529138787)
Concept and light review ACK 31eca93a

💬 furszy commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1262989504)
> Naively, I'd think ProcessNewBlock should just do if (m_interrupt) return false; and the types of races you mention would be avoided.

Hmm, if it arrived to `ProcessNewBlock` is because the node requested the block. So I would try to not discard it, and instead make it pass through `AcceptBlock` so it can be stored. Then quit early before ABC.

🤔 ishaanam reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-1527016849)
Concept ACK

Still working on reviewing this, but here are some initial things. Thanks for breaking it up into twelve commits, it makes it easy to review.