💬 alfonsoromanz commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#discussion_r1570584795)
In Mac, it seems that `activateWindow()` only makes the window active, i.e: the title of the dialog gets highlighted and gets the keyboard input focus. The `raise()` function brings the dialog on top. So, at least in Mac, it makes sense to keep both IMO

💬 mzumsande commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1570597336)
oh - I didn't know about that, interesting!
Another thing we could do is to not only do `peer.wait_for_disconnect()` but also wait until the bitcoind side has disconnected - something like `self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == X)`. Haven't tried that though.

👍 ryanofsky approved a pull request: "index: race fix, lock cs_main while 'm_synced' is subject to change"
(https://github.com/bitcoin/bitcoin/pull/29867#pullrequestreview-2008749979)
Code review ACK 65951e0418c53cbbf30b9ee85e24ccaf729088a1

💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2063768494)
> Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet?

I think we found a real issue with the coin selection target after the rework. But yes, it would be the only one.

> Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding reg
...

💬 josibake commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063776109)
@laanwj @willcl-ark for context, the original goals of having this file were:

1. Make it clear that it was _not_ an example, i.e. nothing in this file is pre-filled for the user, unlike our old bitcoin.conf which did have an example config which was often outdated
2. Make it easy to discover, to deter users from going to some third party for a bitcoin.conf generator (e.g. lopps website for example) or worse, getting a `bitcoin.conf` from "someone on the internet"

For 1, considering that w
...

👍 alfonsoromanz approved a pull request: "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection"
(https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2008812232)
Tested ACK d3da5025f61534d126fef12f198afb65f3a17897

💬 fanquake commented on pull request "test: Add large aligned vmov check for mingw":
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2063806688)
Looks like `python-capstone` at least exists in Guix already: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/engineering.scm#n1820.

💬 laanwj commented on pull request "test: Add large aligned vmov check for mingw":
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2063819614)
> Looks like python-capstone at least exists in Guix already:
> https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/engineering.scm#n1820.

That's good to know! i'm not that surprised, it's a very popular library for doing binary analysis and reverse engineering kind of things.
i'll have a look at adding it.

📝 fjahr opened a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904)
Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.

This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](https://github.com/libevent/libevent/blo
...

💬 laanwj commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2063835209)
Concept ACK
yes please... the hooking we do for `URL_DECODE` and `UrlDecodeFn` made me kind of scared, to be honest (in that context), i think can we get rid of that at the same time?

This will also fix @luke-jr's #29654.

💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1570731552)
In 4b05c654c1:

Using `m_best_header` shouldn't be working (at least not as you expect) for `invalidateblock`.

During `InvalidateBlock()`, we first disconnect all blocks, then call `CheckBlockIndex` without updating `m_best_header` (this is done later on the flow, when we call `InvalidChainFound()`). So, by the time `CheckBlockIndex` is called, `best_hdr_chain` is loaded with a chain that is not the active one.

Any reason not to use `ActiveTip()` instead of `m_best_header` here?

💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-2063852471)
@DrahtBot, I just reviewed it..

💬 willcl-ark commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063858757)
Ok there seems to be some push back here. 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, but happy to conceed on this (perhaps I install binary tarballs [weirdly](https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1895518581)?)

Seems like the path of least resistance is just to append `.example`, so I've done that here now.

> I think all we
...

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

💬 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