💬 sipa commented on issue "Update ChaCha20 to RFC 8439?":
(https://github.com/bitcoin/bitcoin/issues/19225#issuecomment-1633082036)
For posterity, this has been fully addressed by #27985.

🤔 furszy reviewed a pull request: "[WIP] descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1527178771)
@sipa, how the allowance of internal `raw()` subscripts would play out with previous releases?.

I'm thinking on the current wallet migration process, which is broken for pubkey unknown `sh(pkh)` scripts (also for `wsh(pkh)`).

This PR could fix the bug and be backported. Then, after having all the pertinent discussions etc. We could implement the new feature (compat breaking change?) for the next release.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1633198398)
@willcl-ark wow thank you so much for catching that. After all the refactoring, we had forgotten to actually *connect to the unix socket!* I added a functional test to ensure that the proxy works now and I have also tested in production with Tor locally.

This required a fairly intense refactor that included inserting 2 new commits and rebasing several others:

`git range-diff 32a5a20225 c3b0f137a9 b1199bce0f`

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1633199594)
@vasild Thanks for your work on this :-) I reorganized the extra commit you sent me. For now, still going without `variant` for `Proxy` just to keep the refator more simple, so leaving the i2p stuff mostly alone as well.

🤔 mzumsande reviewed a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1527052773)
some more comments, not finished yet though.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261645151)
commit 543273d96e896adf5531ed961856aa0eb70cbe57:
nit: `uint256&` like in the line above?

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261605505)
There should be a `setmocktime` call with the initial time somewhere before the first `fastforward`, otherwise this can fail intermittently.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261668254)
4cbb63175398d9b8afe755d2adb24edbd2b4913b:
nit: could use `orphan_wtxid`, here and in various other spots.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261779747)
I don't think there should ever be a `\n` prefix, why would we want to separate the meta information (timestamp, threadname etc.) from the actual log entry?

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261690923)
29d9d326d5193bb9a410a8881eabc93de5dd6266:
`EraseOrphanOfPeer` looks like a rebase error (it's never implemented and removed in d92b017f6818c1de3286e5dbc35af3860fdf7547).

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261786477)
nit: could use `orphan_wtxid` introduced in the previous commit, here and in various other places.

💬 achow101 commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261786736)
In 7601a60cddc4405f58f71879f996322329509dfe "crypto: add Poly1305 class with std::byte Span interface"

Use `UCharCast` cast here (and below)?

💬 theStack commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261813389)
```suggestion
std::vector<std::byte> key(32, std::byte{(unsigned char)i});
std::vector<std::byte> msg(i, std::byte{(unsigned char)i});
```
With the same idea as above. I think all `Make{Writable}ByteSpan` calls can be avoided in this commit, which improves readability IMHO quite a bit.

🤔 theStack reviewed a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1527358127)
LGTM overall. Verified the introduced test vectors (40de8ce39a65cb37a74bebcbf7b34ed8e70b7096) with the [pyca/cryptography](https://cryptography.io/en/latest/) library, which uses OpenSSL in the background (tried with PyCryptodome first, but that didn't allow use of raw Poly1305 without a cipher algorithm). As expected, everything passes with this alternative implementation as well.
<details>
<summary>Python code</summary>

```python
#!/usr/bin/env python3
from cryptography.hazmat.primitive
...

💬 theStack commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261809644)
could let `ParseHex` return a `std::vector<std::byte>` here and below (for the `final_tag` comparison) to avoid `MakeByteSpan` calls later
```suggestion
auto total_key = ParseHex<std::byte>("01020304050607fffefdfcfbfaf9ffffffffffffffffffffffffffff00000000");
```
(maybe at some point in the future we can change `ParseHex` to return a vector of `std::byte` rather than `unsigned char` by default)

💬 megumin9 commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1633404649)
I understand and I'm sorry to bother you.

💬 ajtowns commented on issue "Auto detect IPv6 connectivity":
(https://github.com/bitcoin/bitcoin/issues/28061#issuecomment-1633414939)
> is `EADDRNOTAVAIL` then that means the machine has no IPv6 address configured (but it may be configured later).

If the system can be reconfigured at runtime to make ipv6 available, it seems a bad idea to force the user to also have to restart bitcoind in order for bitcoind to start making/accepting ipv6 connections. (You might also have ipv6 fail due to your router/isp not supporting ipv6, which also may change at runtime)

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1261909269)
Maybe we could recover that as an invariant by having txs added in `MaybeUpdateMempoolForReorg` set the sequence number as 0? The idea being that if they've made it into a block then we've probably announced the headers already so it's no secret that we knew about the tx anyway...

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914712)
Done.

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914858)
Oh nice, I wasn't aware of `ParseHex<std::byte>`. Done.