💬 dergoegge commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1658115391)
Concept ACK

> Merging the two means a future V2Transport can handle all this interaction without callers needing to be aware.

🚀

✅ fanquake closed an issue: "fuzz: connman, `m_nodes` is always empty"
(https://github.com/bitcoin/bitcoin/issues/27980)

🚀 fanquake merged a pull request: "fuzz: use `ConnmanTestMsg` in `connman`"
(https://github.com/bitcoin/bitcoin/pull/28091)

💬 Sjors commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658120425)
> by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.

Including the original outputs is not part of BIP 125. It's not realistic to expect an entirely new complex RBF proposal, especially one that's not incentive compatible for miners. It's also something for the mailinglist.

> We have not seen any impact of full RBF on double spend rates for our trxs

This does seem relevant. But it can be interpreted both ways: it could also mean y
...

💬 Sjors commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1658134400)
That test is a good starting point. The current version picks one random number so it's not really testing the limit. It also doesn't test N-of-N (hopefully that's not too slow).

💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658137282)
Unfortunately that build broke again for me for another seemingly unrealted reason. I have the correct version of qemu this time for sure:

```log
will@ubuntu in ~
$ qemu-s390x --version
qemu-s390x version 7.2.0 (Debian 1:7.2+dfsg-5ubuntu2.2)
Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
```

I don't think I can test this on my side, but I am a concept ACK on these changes, and they _look_ correct to me.

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658145208)
Feel free to create an issue with the unrelated stuff you ran into. Happy to take a look. Personally, I spin up a fresh VM (in a cloud or locally) to do testing, so I likely miss most CI UX issues.

💬 vasild commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1658150572)
> this might better wait for authenticated connections

I2P and CJDNS connections are already authenticated. Addresses in those networks represent the node's public key and connections have a source address. Thus to spoof, one would need to steal the private key from its owner.

💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658170208)

> > We have not seen any impact of full RBF on double spend rates for our trxs
>
> This does seem relevant. But it can be interpreted both ways: it could also mean your customers have no intention to double-spend you even though it became technically easier.
>
> Opportunity makes a thief, but this default won't change the opportunity by much. A thief needs software that will perform a double-spend without the RBF flag. Anyone making such software will know they need to either use [prefer
...

💬 Sjors commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658191272)
> with FullRBF significantly adopted by miners

If @petertodd's analysis above is correct, that's already the case and this PR won't change that.

💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1658216350)
Even assuming pinning was an issue, it wouldn't be a blocker. It could simply be released at the same time as some other policy change (package relay, #27677, v3 transactions, the next soft fork, etc). Or even just with sufficient heads-up.

📝 MarcoFalke opened a pull request: "refactor: Remove unused MessageStartChars parameters from BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/28191)
Seems odd to expose these for mocking, when it is not needed.

Fix this by removing the the unused parameters and use the already existing member field instead.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279249579)
_(beware, https://bikeshed.com/)_

I think any constant is ok and `bitcoin-submittx`'s string (`/pynode:0.0.1/`) was already suggested by @sipa here https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1517896418. That suggestion has 6 positive reactions. It is about increasing the anonymity set, in case different versions or different softwares use this technique.

> some kind of standard for "I am not telling you"

I don't think there is, but we can standardize `/pynode:0.0.1/` as
...

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1658332910)
`7a3206dc8e...2541f09439`: add a comment wrt https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276240357

💬 MarcoFalke commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1658337460)
I rebased this on a commit by @fanquake to fix another bug, which I am not sure how it happened.

* Usually the CI system should be cloning iwyu into `/ci_base_install/`. See https://cirrus-ci.com/task/4887069785325568?logs=build#L2243:

```
Cloning into '/ci_base_install/ci/scratch/iwyu/include-what-you-use'...
```

* However, it may or may not (?) copy it for some reason from `/ci_base_install/`. See https://cirrus-ci.com/task/4634154260758528?logs=ci#L117:

```
++ CI_EXEC rsync --a
...

📝 Sjors opened a pull request: "ParseHDKeypath: support h as hardened marker"
(https://github.com/bitcoin/bitcoin/pull/28192)
BIP32 allows both `'` and `h` as hardened derivation marker. Our legacy wallet uses `'`. Since #26076 our descriptor wallets use `h` by default.

`ParseHDKeypath` only supports `'`. It's currently only used in the legacy wallet context, so this doesn't cause any problems. But it will once #22341 uses it (to parse the RPC `path` argument for `getxpub`). Might as well fix it now.

I added a restriction for not combining `h` and `'`. Afaik this currently isn't enforced anywhere else in the code
...

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279277093)
It is good to have all unneeded messages dropped from a central place (where all messages go through) even if it seems that the code will not send such messages now - this way it is proof wrt future changes. The code that decides which message to send is scattered all over the place and is difficult to follow.

I added this comment

```cpp
// Ensure sensitive relay connections only send VERSION, VERACK, TX or PING. Others are not needed and may degrade privacy.
```

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1658351829)
re: https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1553292155

> (The above still doesn't satisfyingly explain _why_ we hold them in `m_blocks_unlinked` rather than treat them as any other block index entry. I assume it's because we use the list to proactively aks for these blocks?)

I also found this pretty confusing when I was reviewing it and found a helpful explanation here: https://en.bitcoin.it/wiki/Bitcoin_Core_0.11_(ch_6):_The_Blockchain. The description of `m_blocks
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279303667)
Thanks, looks like this is even a bugfix, see https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1658337460

Unrelated, for a future idea: Since iwyu is installed (`/usr/local/bin/fix_includes.py`, etc), we could even remove the `/iwyu-build/` and `/include-what-you-use/` folder to save some storage. But yeah, that's for the future.

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1658394919)
re-ACK 3de9672a60a385572c294f796aff60ab38c75861 became more complicated compared to my last review, because of #24914. But it makes sense. Rest of the rebase is straight-forward.