💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1633717315)
In commit "ci: forks can opt-out of CI branch push" (8fbf7449af130126890932986a4d9c0446c2f9c8)

I don't really understand this comment. Maybe it could say why the jobs should not be marked as skipped, or maybe say how they should be shown instead of how they should not be shown.

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1633717668)
In commit "ci: forks can opt-out of CI branch push" (8fbf7449af130126890932986a4d9c0446c2f9c8)

What is $NO_BRANCH? Is it a custom variable like $NO_ARM added later? Could it be documented like NO_ARM is?

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1633752828)
Should remove the `noble` type completely in this file

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2159234188)
I could reproduce once outside the CI env on a 24.04 Ubuntu vm:

```
./configure CC='clang-18 -ftrivial-auto-var-init=pattern' CXX='clang++-18 -ftrivial-auto-var-init=pattern' --with-sanitizers=float-divide-by-zero,integer,undefined --enable-usdt --enable-zmq --with-incompatible-bdb --with-gui=qt5 && make

while ( UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/test_runner.py -j 22 --timeout-fact
...

📝 achow101 opened a pull request: "gui: Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824)
Currently the Migrate Wallet menu item can only be used to migrate the currently loaded wallet. This is not suitable for the future when legacy wallets can no longer be loaded at all, but should still be able to be migrated. This PR changes that menu item into a menu list like Open Wallet and lets users migrate any legacy wallet in their wallet directory regardless of the wallets loaded.

One issue I ran into was dealing with encrypted wallets. Ideally, we would detect whether a wallet is encr
...

📝 achow101 opened a pull request: "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files"
(https://github.com/bitcoin/bitcoin/pull/30265)
When the default wallet is migrated, we do not rename the wallet so we end up having a descriptor wallet with the empty string as its name and the wallet.dat file in the root of the walletdir. This is supposed to be an unsupported configuration and there is no other way to achieve this (other than file copying), but the wallet loading code does not disallow loading such wallets. However `listwalletdir` does not currently list the default wallet if it is sqlite. This is confusing to users, so cha
...

💬 mzumsande commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1633915935)
I can confirm this behavior and also think that this has uncovered a bug in `net_processing`.

I think that the root cause is in [TryDownloadingHistoricalBlocks](https://github.com/bitcoin/bitcoin/blob/b1ba1b178f501daa1afdd91f9efec34e5ec1e294/src/net_processing.cpp#L1511-L1538) (which is responsible for downloading the background chain):
This function calls `FindNextBlocks()` with `pindexWalk` set to the current tip of the background chainstate (`from_tip`), and `target_block` set to the sna
...

💬 mzumsande commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1633934145)
Although I'm not sure if this is an actual problem in practice, because with our DoS protections for low-work blocks and headers we shouldn't really get into this situation with a divergent chain in the first place.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2159465142)
@maflcko Thanks, I can also reproduce that way.

⚠️ edilmedeiros opened an issue: "Won't compile with miniupnpc 2.2.8"
(https://github.com/bitcoin/bitcoin/issues/30266)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Bitcoin-core will not compile if using miniupnpc 2.2.8.

### Expected behaviour

Compile successfully.

### Steps to reproduce

1. Install miniupnpc 2.2.8 from http://miniupnp.free.fr/.
2. `./configure`
3. `make`

### Relevant log output

<details>
<summary>configure script output</summary>

```bash
❯ ./configure --with-boost=/opt/local/libexec/boost/1.78 --with-gui=no
checking for
...

💬 edilmedeiros commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2159613627)
I second this, never understood why not expose all the CLI tools in the binaries distribution.

👋 ryanofsky's pull request is ready for review: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409)

💬 edilmedeiros commented on issue "Won't compile with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/issues/30266#issuecomment-2159644988)
From miniupnpc changelog:

```
VERSION 2.2.8 : released 2024/06/09

2024/05/16:
IPv6: try site-local before link-local

2024/05/08:
upnpc.c: Add -f option to upnpc program (delete multiple port redirections)
UPNP_GetValidIGD(): distinguish between not connected and connected to a
"private" network (with a reserved IP address).
Increments API_VERSION to 18
```

Relevant line in the configure script: https://github.com/bitcoin/bitcoin/blob/b1ba1b178f501daa1afdd91f9efec34
...

💬 edilmedeiros commented on issue "Won't compile with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/issues/30266#issuecomment-2159651901)
Well, this can be fixed:
1. by adjusting the above checks (keep dependency of miniupnpc in version range 2.1 to 2.2.7).
Drawback: miss potential security updates on the upstream package.
2. by diving deeper and using the new API (backward incompatible, possibly increasing friction in a lot of systems until the main package managers maintainers decide to update miniupnpc version).

Note that when someone decide to upgrade the version in `depends`, the code base will have to support the new
...

💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1634195691)
Will do if I retouch.

💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1634196738)
I think that would be too complicated. Feel free to consider a followup, but I would restrain bloating this PR further.

🤔 ajtowns reviewed a pull request: "Tr partial descriptors"
(https://github.com/bitcoin/bitcoin/pull/30243#pullrequestreview-2109323689)
Seems okay to me. Presumably should update doc/descriptors.md though, and might also be nice to have some functional tests demonstrating it works end-to-end? (Add some examples to `DESCS` in wallet_miniscript.py maybe?

💬 ajtowns commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1634192469)
Having an `optional` when in one branch you ignore the value and in the other you assume it always has a value doesn't make a lot of sense to me.

Would it make sense to drop `leaf_version` from `TRNodeInfo` and store the leaf version as the first byte of the script? Alternatively, could just drop the optional and default it to 0.

💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1634204476)
All these

https://github.com/bitcoin/bitcoin/blob/b1ba1b178f501daa1afdd91f9efec34e5ec1e294/test/functional/test_framework/util.py#L393-L423

can be considered as divergence from the default `bitcoind` behavior.

The suggested change adds a new `bitcoind` config option just for the test framework, I do not like that. I guess it may be possible to avoid Tor collisions by spreading the ports without using a new config option, by using `-bind=127.0.0.1:torport=onion`, but then, I do not think
...

💬 emc99 commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2159910695)
Most macs don't come with the storage that bitcoin core requires.

I was down in the store the other day having a look and the storage hdd capacity on macOS machines was limited to 256GiB.

So I suppose the only solution is to purchase extra storage @Sjors