💬 stickies-v commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1385269044)
When changing `options` into an rvalue `std::vector<std::string>&& options={}`, can simplify this to:

```suggestion
std::set<std::string> args(std::move(options.begin()), std::move(options.end()));
```

💬 stickies-v commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1387077532)
Could also use a `util::Result<void>` return type to avoid the out-parameter?

<details>
<summary>git diff on 6be2b82e1c</summary>

```diff
diff --git a/src/common/args.cpp b/src/common/args.cpp
index b9454d3814..f7509b3e7b 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -15,6 +15,7 @@
#include <util/check.h>
#include <util/fs.h>
#include <util/fs_helpers.h>
+#include <util/result.h>
#include <util/strencodings.h>

#ifdef WIN32
@@ -358,27 +359,25 @@ std::opt
...

💬 stickies-v commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1387084187)
nit: list initialization, here and in a few other places
```suggestion
auto command_options{m_available_args.find(OptionsCategory::COMMAND_OPTIONS)};
```

💬 pinheadmz commented on issue "Apparently CJDNS network does not work with Tor on mainnet.":
(https://github.com/bitcoin/bitcoin/issues/24450#issuecomment-1802697256)


I could work on this. I think it would be cool to support all networks with full privacy (meaning ipv4 and ipv6 routed over Tor). Onion goes over the SOCKS5 and I2P has its own proxy server (SAM), CJDNS is just a little trickier.

> Maybe we should consider something like `-proxy=1.2.3.4:5678=ipv4` for a more fine-grained control?

I think it's either something like this, or another option like `-cjdnsproxy` (default true if `-proxy` is set)

👍 luke-jr approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1721315158)
utACK after squash

💬 hebasto commented on pull request "doc: update docs for `CHECK_ATOMIC` macro":
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1802728334)
Not directly related to this PR patch, but...

At some point in the future, the Bitcoin Core project will drop support for i686 platforms. It looks unreasonable to run the modern Bitcoin Core on CPUs without instruction sets for h/w optimizations etc. And the market is full of cheap alternatives.

By what criteria will we make such a decision?

Could this moment be now?

💬 luke-jr commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1802742270)
Even aside from the main issue, it's still ambiguous for the various opcode possibilities... :/

🤔 mzumsande reviewed a pull request: "test: Add missing wait for version to be sent in add_outbound_p2p_connection"
(https://github.com/bitcoin/bitcoin/pull/28822#pullrequestreview-1721365213)
ACK faa2ad88bc01bd434ce19fde19bcc5c78431702f

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1802781216)
Updated 3e7595b11bdad260efb39adc42677ed0beae186d -> 2be5e799ba623f969f5ffc59667a5bc6799df290 ([simplifyMemPoolInteractions_11](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_11) -> [simplifyMemPoolInteractions_12](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_11..simplifyMemPoolInteractions_12))

* Addressed @glozow's [comment](https://github.com/bitc
...

💬 mzumsande commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1802811199)
I changed the first commit to save the default behavior (global `--v2transport`) in the TestNode init and add an entry to `args`, and then deciding in `TestNode::start()` (which is called for both the initial startup and for restarts) based on this and possible `extra_args` whether to actually use v2 or not.

💬 vostrnad commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1802827804)
For anyone intersted in documenting opcodes, there is now https://opcodeexplained.com/ ([GitHub repo here](https://github.com/thunderbiscuit/opcode-explained)).

💬 kevkevinpal commented on pull request "test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)":
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1802851411)
running these two grep commands I got no results back which is good
`grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./test`
`grep -nr -e "Dict\[" -e "List\[" -e "Set\[" -e "Type\[" -e "Tuple\[" -e "FrozenSet\[" ./contrib`

and then running these and checking if there were anything we missed
`grep -nri "from typing import" ./contrib`
`grep -nri "from typing import" ./test`

but only found ones that we couldn't use builtin collection types for

do w
...

💬 ajtowns commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#issuecomment-1802885826)
> Ah, #10730

"When we last discussed making scripts debuggable" -- this and #28802 are prereqs for "bitcoin-util evalscript" which I'm poking at on https://github.com/ajtowns/bitcoin/commits/202309-evalscript . Real debugging seems better done with smarter external tools like https://github.com/dgpv/bsst though.

👍 theStack approved a pull request: "refactor: Miniminer package linearization followups"
(https://github.com/bitcoin/bitcoin/pull/28808#pullrequestreview-1721469632)
LGTM ACK b4b01d3fb42e7b688d97b75f57cfe18cfca6d943

💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387351444)
Interesting that you're getting that error when loading from a backup, but not when loading a regular wallet.

💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-1802999311)
Rebased to see if the asserts added in #28546 catch anything here. (They don't in the test suite and manual testing).

💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387360904)
> I think wallet tests should be separate from consensus tests.

That makes sense.

💬 Sjors commented on pull request "bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC":
(https://github.com/bitcoin/bitcoin/pull/28554#issuecomment-1803011008)
Thanks for rebasing.

re-utACK 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658

💬 Sjors commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1803013981)
I'm fine with adding another contrib script here. But we could also make a new repo under bitcoin-core. Perhaps the choice depends on whether this tool is mainly useful for users, developers or maintainers ([bitcoin-core/bitcoin-maintainer-tools](https://github.com/bitcoin-core/bitcoin-maintainer-tools))?

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1803029813)
re-utACK b31d9bf1ae764421c8937dd126ad9e29845ebc5e