💬 pinheadmz commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1496566849)
Kinda funny that in/out maps to in/manual ... perhaps `"manual"` should've been the permission flag?

💬 pinheadmz commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1496520271)
nit, I think these should get the `m_` prefix. When I was reviewing a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16 I thought `whitelist_relay` was a local variable and couldn't tell where it was defined. Maybe just me though 🤷

👍 pinheadmz approved a pull request: "p2p: Allow whitelisting manual connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1891576785)
ACK c10f528350152ca9248e8c167b67993fc7321ca3

I left comments on a few nits but none are breaking for me. Happy to re-ACK if you update.

Built and ran all functional and unit tests locally on macOS/arm

Reviewed specific changes since my last ACK:
```
git range-diff abfc8c901d..d69747ab65 5883a8911a..c10f528350
```
- Move relay flags to netpermissions from net
- Only apply new logic to manual connections
- Apply flag to new tests added since last review
```


<details><summary>S
...

💬 pinheadmz commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1496499176)
Now that we're not modifying `nLocalServices` I don't **think** we need the local variable `nodeServices` any more.

💬 luke-jr commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1955210936)
Not sure it makes sense to do this. If you're parsing JSON, surely you can do the trivial things inside rpcauth yourself?

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1891677340)
Updated 3c311734d2fc6a4ca410f254ba3c8e923d58be70 -> e02ab1577f51a6c9af96b49e29ffaa3918ddae6c ([`pr/iparams.4`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.4) -> [`pr/iparams.5`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.4..pr/iparams.5)) using && and std::forward to support rvalue substream arguments and adding tests and documentation

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496589417)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484436443

> Yes, an example would be in net.cpp, but feel free to ignore, if it is too much hassle.
>
> ```c++
> src/net.cpp- DataStream underlying_stream{vSeedsIn};
> src/net.cpp: ParamsStream s{underlying_stream, CAddress::V2_NETWORK};
> ```

Since it didn't complicate implementation of this PR too much, I implemented support for passing any type of substream as an rvalue to ParamsStream, instead of only passing ne
...

🤔 fjahr reviewed a pull request: "p2p: Don't process mutated blocks"
(https://github.com/bitcoin/bitcoin/pull/29412#pullrequestreview-1891184515)
Concept ACK

"`CBlock::GetHash()` is a foot-gun without a prior mutation check", I hadn't felt this strongly about but I get it. I think it makes sense to rename it. I have drafted it here: https://github.com/fjahr/bitcoin/commit/8e11e9c9ec9d52b2c5c04a33f33da0b22ae5e28b If it sounds valuable I will open a PR.

💬 fjahr commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1496312078)
Given the recent changes around the mailing list could also name what's behind this link? So in case it disappears people know what to search for. I know they promised to keep the links but who knows...

💬 fjahr commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1496365733)
It would also be a little bit easier to reason about if when `block.vtx.empty()` is true then we just `return false;` because in that case `std::any_of` will always return false.

💬 luke-jr commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-1955225976)
Probably should be a way to override this, in case someone actually wants a custom-fit manpage?

💬 kristapsk commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1955227816)
> Not sure it makes sense to do this. If you're parsing JSON, surely you can do the trivial things inside rpcauth yourself?

Probably also true, my Bitcoin node setup script currently just does something like:

```bash
python3 "$SCRIPT_DIR/rpcauth.py" \
"$bitcoin_rpcuser" "$bitcoin_rpcpassword" | grep rpcauth >> /etc/bitcoin/bitcoin.conf
```

💬 luke-jr commented on pull request "Correct tooltip wording for watch-only wallets":
(https://github.com/bitcoin-core/gui/pull/792#discussion_r1496623669)
Despite the confusing interfaces, watch-only wallets are not based on addresses (or they would never see spends). Prefer to use less confusing language here.

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496625389)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496589417

> The warning doesn't make sense and I think it is a compiler bug.

Actually it is not a compiler bug. It happens because of the lifetimebound annotation and goes away if I change:

```diff
- ParamsStream(SubStream&& substream LIFETIMEBOUND, const Params& params LIFETIMEBOUND) : m_params{params}, m_substream{std::forward<SubStream>(substream)} {}
+ ParamsStream(SubStream&& substream, const Params& params LIFE
...

💬 luke-jr commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-1955232548)
Wouldn't it be better to hide the covered widgets instead?

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1891730489)
Updated e02ab1577f51a6c9af96b49e29ffaa3918ddae6c -> 33d99bad01637a3454358841b5c1aaf099002afb ([`pr/iparams.5`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.5) -> [`pr/iparams.6`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.5..pr/iparams.6)) adding a new commit simplifying usage of ParamsStream in net.cpp, and fixing inaccurate lifetimebound annotations

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496650793)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496625389

This should be resolved in latest push, which just drops the LIFETIMEBOUND annotation since it isn't always accurate. The only case where specifying the annotation is actually correct is the case where the argument is an lvalue. And in that case, I think the annotation is probably not useful. The language already doesn't allow passing temporaries as rvalues so there is probably not anything the compiler can helpfully war
...

💬 bstin commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1955370336)
The default behavior is the same, so no change unless someone specifically wants json output.

For anyone just outputting "rpcauth=XXXXX" directly into bitcoin.conf, yes the grep solution is fine.

But if, in addition to that, you wanted to extract the *value* of rpcauth, then its more steps. (further, this assumes "=" will always be the delimiter)

By providing a machine-friendly output format that makes such deployments less fragile in the future.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1496674625)
for code readability, such variables could be renamed `inbounds_flooding_tx_relay` / `outbounds_flooding_tx_relay`, which is matching the low-fanout flooding strategy used for them.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1496695880)
I confirm with current implemented bip331 approach there is currently a `MAX_ORPHAN_TOTAL_SIZE` limit.
You can always get a non-standard parent (e.g an under-dust output) and yet the child be policy valid.
There is currently no sanitization of policy equivalence among a set of parent within `ancpkginfo`.
In the future, you could have reconciliation at the `pkgtxns`-level or at package announcement (`ancpkginfo`).
Ideally both, though that something that can be seen once erlay or bip331 are d
...