💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1630517256)
NACK. I don't think compiling from source should be used to unintentionally and accidentally fix an unrelated bug.

💬 MarcoFalke commented on pull request "refactor: Move stopafterblockimport option out of blockstorage":
(https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1259500271)
Any reason to change this? Is there any need to remove the named constant?

💬 TheCharlatan commented on pull request "refactor: Move stopafterblockimport option out of blockstorage":
(https://github.com/bitcoin/bitcoin/pull/28053#issuecomment-1630541820)
Updated f87c21c1827fa532749ec17a2f44322378046b93 -> 462390c85f1174b3cf83dfb0f74922049e5cb8af ([blockImportReturn_1](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_1) -> [blockImportReturn_2](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockImportReturn_1..blockImportReturn_2))

* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1259500271), re-introduced
...

👍 MarcoFalke approved a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1523927780)
lgtm ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af 🗝

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 462390c85f1174b
...

💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1259525623)
Yes, that's the idea of "reconsiderable." If you get A again with a different package, you reconsider it. If you get A+B again, you don't. In this set of changes, `TX_LOW_FEE` is used to decide to put something in the reconsiderable cache.

💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1259544521)
This is tracked in https://reviews.llvm.org/D86993

💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1259547933)
Can also be tested with something like on `clang++ -O0` arm64:

```cpp
#include <array>
int main() {
using A = std::array<unsigned char, 33>;
A obj{};
A copy{std::move(obj)};
obj = std::move(obj); // valid, but unspecified state
obj = std::move(copy);
}
```

💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259566308)
I think I was confusing the kernel/node boundaries a bit. You're right that here in the kernel logic that is `validation.cpp`, the documentation and behaviour are what we'd expect.

My concern was around the _node_ implementation of `blockTip()` now potentially calling `StartShutdown()` and returning a `kernel:Interrupted` without leaving any trace. It's not something we'd expect to happen in `InvalidateBlock()` (and it would be behaviour change). I think I was wrong in suggesting we catch/log
...

💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259573661)
You make a lot of good points, and I think I agree with all of them. Thank you.

💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1630664257)
It's a tNACK from me currently @ c3b0f137a9 as I can no longer get this functionality to work in practice on Ubuntu 23.04 with Tor 0.4.7.13.

The send of the data in `vSocks5Init` seems to be failing, meaning the proxy cannot be used: https://github.com/pinheadmz/bitcoin/blob/c3b0f137a9ccd894f2f1d287ccdab26310b3aaf2/src/netbase.cpp#L372-L375

I haven't yet totally ruled out issues with Tor, but I do get responses from the unix socket about it not being an HTTP proxy if I try to send an HTTP
...

💬 recursive-rat4 commented on issue "Creating a Wallet Feature Guidelines Document":
(https://github.com/bitcoin/bitcoin/issues/28062#issuecomment-1630685387)
I'd rather see it more useful with an opposite list of what is in scope, for example:
- The Wallet should improve an user experience when that does not compromise the security
- The Wallet must use the Bitcoin P2P network when an interaction with the outside world is desired
- The Wallet should implement new features that got standardized as BIPs

💬 naumenkogs commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1630685413)
Approach ACK

📝 MarcoFalke opened a pull request: "fuzz: Flatten all FUZZ_TARGET macros into one"
(https://github.com/bitcoin/bitcoin/pull/28065)
The `FUZZ_TARGET` macros have many issues:
* The developer will have to pick the right macro to pass the wanted option.
* Adding a new option requires doubling the number of existing macros in the worst case.

Fix all issues by using only a single macro.

This refactor does not change behavior.

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1259659543)
nit: (This is my fault)

Not really a fan adding more macros, where each new option will cause doubling all existing macros. Currently there are 3, in this pull there are 6, and with the next option we'll have 12 to 16 macros?

At least for the existing options, which only need to be known at runtime, an options struct can be used.

See https://github.com/bitcoin/bitcoin/pull/28065 . Feel free to ignore/NACK.

💬 willcl-ark commented on issue "Bitcoin core hidden services link down/doesnt work":
(https://github.com/bitcoin/bitcoin/issues/28054#issuecomment-1630813890)
Thanks for the report @TNTBOMBOM.

I am going to close this issue here as it's more appropriate for the bitcoincore.org repo, and I've taken the liberty of opening an equivalent issue over there on your behalf: https://github.com/bitcoin-core/bitcoincore.org/issues/985

✅ willcl-ark closed an issue: "Bitcoin core hidden services link down/doesnt work"
(https://github.com/bitcoin/bitcoin/issues/28054)

👍 ryanofsky approved a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1524282976)
Code review ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af. Just has been rebased and is a simpler change after #27607

💬 willcl-ark commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1630852725)
Unrelated to the implementation here, but in general I disagree with most of Luke's comment:

> Port isn't required (the default is 9051).

The way this is currently documented, both host and port should be required:

```

The option is `torcontrol` and should require `host::port` if specified. If we used independant options `torcontrolhost` and `torcontrolport` then we could reasonably have defaults for both, and make both optional, but having _half of a single option_ be optional seem
...

✅ willcl-ark closed a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018)

📝 willcl-ark reopened a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018)
right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :


closes https://github.com/bitcoin/bitcoin/issues/23589
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
firs
...