💬 brunoerg commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1570815195)
```suggestion
(void)crypt.Decrypt(cipher_text_ed, plain_text_ed);

```

💬 brunoerg commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1570821642)
Since you're using `ConsumeDeserializable`, I do believe you should adopt the approach introduced in "fuzz: Avoid timeout and bloat in fuzz targets" fabb5046a7365af3079e6e45606d63576bc6ad12

💬 achow101 commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2063938314)
I think the "Update repositories and websites for new version" section could be split into 2 distinct steps of update the website, then update the packaging repos. Apparently flathub polls the website for new releases and their bot will automatically open a pr to the flathub repo with the new version hashes. So that step must be done after the website is updated.

💬 josibake commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063942430)
> Personally, I'd prefer to see files neatly in directories so that folks installing from a tarball directly into /usr/local wouldn't have random odds-and-ends ending up in the root there

We still have the `README.md` in the root, so moving this file doesn't really address that concern. If there is a compelling argument for having _nothing_ in the root, then I'd argue `config/bitcoin.conf.example` would make the most sense (as this is what I've seen for most other tarball distributed software
...

💬 josibake commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063950133)
LGTM, although the commit message and PR title still mention moving to `share/examples`

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1570838311)
This is just being moved but agreed is simple enough to include it here.

Covered in [fd81a37](https://github.com/bitcoin/bitcoin/pull/28834/commits/fd81a37239541d0d508402cd4eeb28f38128c1f2)

📝 BorjaPractica opened a pull request: "modificación"
(https://github.com/bitcoin/bitcoin/pull/29905)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "modificación"
(https://github.com/bitcoin/bitcoin/pull/29905)

📝 fanquake locked a pull request: "modificación"
(https://github.com/bitcoin/bitcoin/pull/29905)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2063985018)
Either you could generate the mainnet index using the code in this pull request, and then compare it. Or I can take your list and compare it to my result. Whoever gets around to it first :-)

👍 guggero approved a pull request: "ZMQ: Support UNIX domain sockets"
(https://github.com/bitcoin/bitcoin/pull/27679#pullrequestreview-2009075318)
Tested and code review ACK 21d0e6c7b7c7

Thanks a lot for the fix!

💬 ryanofsky commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1570911819)
> ok, yeah. Then it is not an issue but, isn't error-prone to call `best_hdr_chain` something that might not be the best chain then? E.g. it seems that if we ever add a check only for the active chain blocks here, the `best_hdr_chain.Contains()` usage temptation will be there.

I don't love the name `best_hdr_chain` but don't think it is too bad. I agree it could potentially be confused with the active chain in new code, and that would be a bug. In my original suggestion https://github.com/bit
...

💬 pablomartin4btc commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#discussion_r1570920312)
Yes, it needs both `activateWindow()` & `raise()`, let me try it on Windows and I'll incorporate your [suggestion](https://github.com/bitcoin-core/gui/pull/817#discussion_r1569483043), thanks!

💬 hebasto commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2064197562)
Concept ACK.

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2064247329)
> > There are no conflicts with #29865.
>
> Looks like there are now?

Well, there is a conflict in the code, which is being removed in #29865. It won't be a problem for either PR if the other one goes first.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571015884)
(deleted `GetCombinedHash`)

🤔 glozow reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2009283703)
Main changes:
- Try same-peer orphanage children (most recent first) before different-peer ones (randomized). Replaced `GetChildren` with `GetChildrenFromSamePeer` and `GetChildrenFromDifferentPeer`.
- Deleted `GetCombinedHash` because it's unused. Also deleted the tests for it
- Changed `GetPackageHash` a bit. Using single-sha256 instead of double-sha256 and using just wtxids instead of serialized vector

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571015190)
deleted

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571014755)
I figure it's easier to read the test this way; you can easily tell that the lexicographical ordering is what I claim it to be

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571015988)
(deleted `GetCombinedHash`)