💬 ryanofsky commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1570756697)
> Any reason not to use `ActiveTip()` instead of `m_best_header` here?

I think the reason is just that we want `best_hdr_chain` to be the longest chain with the most forks for the optimization to work well. We don't actually care which chain it is because CheckBlockIndex will traverse all the blocks in any case. If a different chain is chosen it just means the blocks will be traversed in a different order, less or more efficiently.

💬 maflcko commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1570777163)
> EDIT: wondering why there are limits

There needs to be a limit, because each test gets allocated the full (maximum) range of ports for the duration of the test_runner (that is for the duration of *all* tests, not the test itself) to avoid port collisions. Thus, the limit needs to exist, because there is a finite number of ports on any system.

👍 vasild approved a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-2008933057)
ACK ae7afa5547889ccf04c1522555e28e025d14f335

💬 vasild 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_r1570773337)
nit: since this line is being changed anyway, consider this:

```suggestion
LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s\n", proxy.ToString(), target_addr.ToStringAddrPort());
```

This would take care to put IPv6 addresses in `[` `]` instead of printing something like `2001:470:88ff:2e::1:8333`

💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1570807877)
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.

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

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