💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1386940797)
Good observation. This is how `ConnectSocketDirectly()` and `ConnectThroughProxy()` are implemented on master currently as well. I think the reason is that we actually send the SOCKS5 proxy the destination host as a string whereas for the direct connection we actually get a socket from the OS. So even if throughProxy accepted a CService, we'd have to split into host string and port right away, anyway. I think I'll leave this alone for now unless other reviewers want to weigh in.

💬 achow101 commented on pull request "ci: remove note re M1 usage":
(https://github.com/bitcoin/bitcoin/pull/28823#issuecomment-1802419915)
ACK 8cbb6196913b22006dac75f719a2834ab0d6c94f

🚀 achow101 merged a pull request: "ci: remove note re M1 usage"
(https://github.com/bitcoin/bitcoin/pull/28823)

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

* Addressed @ismaelsadeeq's [comment](https://github.co
...

🤔 glozow reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1721061557)
ACK 3e7595b11bdad260efb39adc42677ed0beae186d

💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387067811)
nit 65c4b3bac0894dfdf7e1c756697f9dd0da1f97e7: don't need to go to uint256 and back again
```suggestion
const Txid& hash = txinfo.tx->GetHash();
const auto entry{pool.GetEntry(hash)};
```

📝 hebasto opened a pull request: "ci: Switch IWYU to `clang_17` branch"
(https://github.com/bitcoin/bitcoin/pull/28826)
The IWYU version [0.21](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.21) has been tagged, and the `clang_17` branch is available now.

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