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

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279466130)
It would be useful here - if BDB is compiled - to also open it with BDB, dump and then compare both dumps.

I'm not sure what to do for cases where BDB and RO have slightly different opinions on what they're willing to open at all. It's probably fine to skip comparing files where _either_ refuses to open.

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279468544)
Not in the fuzz docs, but just having it in the test docs is probably fine.

📝 theStack opened a pull request: "test: add script compression coverage for not-on-curve P2PK outputs"
(https://github.com/bitcoin/bitcoin/pull/28193)
This PR adds unit test coverage for the script compression functions `{Compress,Decompress}Script` in the special case of uncompressed P2PK outputs (scriptPubKey: OP_PUSH65 <0x04 ....> OP_CHECKSIG) with [pubkeys that are not fully valid](https://github.com/bitcoin/bitcoin/blob/44b05bf3fef2468783dcebf651654fdd30717e7e/src/pubkey.cpp#L297-L302), i.e. where the encoded point is not on the secp256k1 curve. For those outputs, script compression is not possible, as the y coordinate of the pubkey can't
...

💬 fanquake commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1279500464)
Maybe relevant here, I think it's easier to just avoid (explicitly) copy-pasting ChatGPT or similar output into the project, for now, see #28175.

💬 achow101 commented on pull request "ParseHDKeypath: support h as hardened marker":
(https://github.com/bitcoin/bitcoin/pull/28192#issuecomment-1658730282)
I'm not sure if there's any software that enforces the consistent usage of a hardened marker.

💬 ryanofsky commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1658798341)
NACK from me, because I think legal questions like this are essentially political questions, and you make absurd legal outcomes more likely to happen by expecting them to happen, and by writing official documentation which gives them credence.

If the risk is that openai or microsoft could claim copyright over parts of the bitcoin codebase, that would be absurd because their usage agreements assign away their rights to the output and say it can be used for any purpose.

If the risk is that s
...

💬 kristapsk commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658851397)
> 2\. Users who do care and want full-RBF to be **off** and rely on defaults not changing.

Users should check release notes before upgrading Bitcoin node. Various defaults changes from time to time, this won't be the first time.