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

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279351539)
nit: Can drop the comment? (The others don't have a comment either)

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279321024)
This commit is neither a refactor, nor is it correct. `\n` is already included in `base`.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279330906)
Could add a reason why this should be avoided?

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069)
I think the test isn't run in CI, is it?

👍 MarcoFalke approved a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1554817965)
ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b 🗑

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 3a727cd7ee02c83efcd57d004e
...

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1658468351)
Concept ACK

I still suggest changing fc810c1df899e5b42fe7658ebdddc6b4320240a6 so that `dumpwallet` does not use the read-only format by default, and instead change the tests use it. However with the new fuzz target commit I'm less worried about it.

💬 BitcoinMechanic commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658548723)
> This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
>
> At GAP600 we continue to see strong use of our service for BTC we have seen circa 350k unique trx hash per month (over the last 3 months) requested to our platform. Our clients include - Coinpayment
...

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279447033)
Maybe add a recommendation to the fuzz doc to [mount /tmp to tmpfs](https://ubuntu.com/blog/data-driven-analysis-tmp-on-tmpfs), or ensure we don't use the file system.

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279450655)
This is already in the test docs, no?

```
$ git grep tmpfs test/README.md
test/README.md:sudo mount -t tmpfs -o size=4g tmpfs /mnt/tmp/

💬 jonathanbier commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658569312)
When I first read this, I did not understand what those links to https://mempool.space/ were, as I did not know, but they seem to have a new RBF feature. This RBF information then seems to go after a few days. Therefore I have provided some screenshots below so people can see the feature and it wont disappear. It shows that the transactions were replaced and that there was no RBF flag. I hope this helps.

[Antpool](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122
...