💬 achow101 commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1802505911)
ACK fa520848da6d718a07368b42b1a44bd2515e6e5a

💬 brunoerg commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1802536617)
Force-pushed:

- Added a check to control whether we called `Ban` with an invalid subnet/netaddr. If so, we don't compare the banmaps.
- I did some experiments and I decided to keep the lookup functions. Without it, I still get discrepances even avoiding the comparison when we call `Ban` with an invalid subnet/netaddr.

✅ pinheadmz closed an issue: "How to communicate with the web service"
(https://github.com/bitcoin/bitcoin/issues/28596)

💬 pinheadmz commented on issue "How to communicate with the web service":
(https://github.com/bitcoin/bitcoin/issues/28596#issuecomment-1802544156)
Closing for now, @Hossein-Teimouri feel free to add comments if you are still having trouble. Or ask a question on https://bitcoin.stackexchange.com/

💬 mzumsande commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1802573479)
> @mzumsande, wanted to confirm which scenario you're talking about.

I was observing that multiple following tests fail on master with `--v2transport` e.g. `interface_zmq.py`, `p2p_permissions.py`, `feature_assumeutxo.py` and some wallet tests.
These tests have in common that they use `self.restart_node()` with changing some unrelated `extra_args`, which would then cause `--v2transport` to be dropped.

I think the problem is as follows:

- On first startup of a node, `self.extra_args` is
...

🤔 murchandamus reviewed a pull request: "refactor: Miniminer package linearization followups"
(https://github.com/bitcoin/bitcoin/pull/28808#pullrequestreview-1721225781)
ACK 89c146874175f8481c59436813ccf52a177a8664

This does address the comments I left on the prior PR https://github.com/bitcoin/bitcoin/pull/28762, and the conflicting PR appears to still be a draft. Looks cleaner and easier to understand to me, but also not particularly urgent.

💬 murchandamus commented on pull request "refactor: Miniminer package linearization followups":
(https://github.com/bitcoin/bitcoin/pull/28808#discussion_r1387169428)
Nit: First sentence ends in a comma now.

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1387181325)
Huh, I hadn't realised we have different formats for class/struct.

💬 kevkevinpal commented on pull request "refactor: Miniminer package linearization followups":
(https://github.com/bitcoin/bitcoin/pull/28808#discussion_r1387182354)
fixed in [b4b01d3](https://github.com/bitcoin/bitcoin/pull/28808/commits/b4b01d3fb42e7b688d97b75f57cfe18cfca6d943)

🤔 murchandamus reviewed a pull request: "refactor: Miniminer package linearization followups"
(https://github.com/bitcoin/bitcoin/pull/28808#pullrequestreview-1721253191)
Trivial fix of my nit and improved formatting of the corresponding comment.

reACK b4b01d3fb42e7b688d97b75f57cfe18cfca6d943

🤔 stickies-v reviewed a pull request: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802#pullrequestreview-1718257717)
Approach ACK

💬 stickies-v commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1387006587)
I found this quite difficult to read, could be improved with structured bindings, consistent naming, and some other slight improvements, I think. Pushed 3 commits to https://github.com/bitcoin/bitcoin/compare/6be2b82e1ce2207b0b9af01ced67d71361a2047b...stickies-v:bitcoin:pr/28802-suggested-simplifications - what do you think?

Potentially, this could be made more straightforward by having `m_command_args` use iterators into `m_available_args`, but that's probably not worth the iterator manageme
...

💬 stickies-v commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1386743469)
nit: Works fine for now, but perhaps a `uint8_t nesting_level` is a bit more general and not more verbose?

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