💬 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
...
(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
...
(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
(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.
(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.
(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
...
(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.
(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?
(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..
(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
...
(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
(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.
(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.
(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
(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`
(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.
(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
(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);
```
(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
(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.
(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.