✅ hebasto closed a pull request: "ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164)

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1649049643)
In any case, I don't see (or forgot) what problem is this trying to solve? It is just a display issue where the task is displayed as "skipped" vs something-else? Not sure if the CI config is the right place to provide fixes/fiddles for cosmetic stuff in forks.

💬 maflcko commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182877327)
> Merging without enabling [won't work](https://github.com/bitcoin/bitcoin/actions/runs/9614060766):

Would have been nice to merge it this way, but unfortunate that it doesn't work.

Good to have the config sitting around in this pull request, anyway. This way it can be picked up easily by anyone in the future.

📝 fanquake opened a pull request: "[26.x] upnp: fix build with miniupnpc 2.2.8 "
(https://github.com/bitcoin/bitcoin/pull/30319)
Backports https://github.com/bitcoin/bitcoin/pull/30283 to the 26.x branch.

💬 fanquake commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2182905009)
Backported to 26.x in #30319.

💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2182927906)
re-ACK 2f9bde6

💬 furszy commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#issuecomment-2182953969)
> Anything that should be added to unit tests?

The behavior change is tested on a functional test.
Functional tests are portable across releases due to our API stability requirement. Unit tests are harder to maintain due to internal code changes.

💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649111896)
From `(273 when high-s)`, I assume this considers a signed input but the docs of `createrawtransaction` and `fundrawtransaction` say that these commands don't generate a signed transaction. So this max weight check on picking 1472 inputs of 273 WU each internally seems to be dependent only on the `weight` parameter passed to `fundrawtransaction` RPC, which is dependent on the user input.
Is there not any other internal worst-case scenario treatment of the inputs weights that might not be depen
...

💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649113587)
I assume the `MAX_STANDARD_TX_WEIGHT` will be replaced by `max_tx_weight` during or after this PR is merged? https://github.com/bitcoin/bitcoin/pull/29523

👍 rkrux approved a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2132779327)
tACK [3f50276](https://github.com/bitcoin/bitcoin/pull/30309/commits/3f5027626593c7af546d3b53fc658c87bb815348)

`make, make check, test/functional` are successful.

Thanks @furszy for this improvement, and keeping the PR short and to the point, easy to review! Asked few questions for my understanding.

💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649036915)
The comment above MAX_STANDARD_WEIGHT says it's the maximum weight we are willing to mine: https://github.com/bitcoin/bitcoin/blob/master/src/policy/policy.h#L27

Should the failure check here not be strictly greater than?

📝 mzumsande opened a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320)
This was suggested by me in the discussion of #30288, which has more context.

If the snapshot is not an ancestor of the most-work header (`m_best_header`), syncing from that alternative chain leading to `m_best_header` should be prioritised. Therefore it's not useful loading the snapshot in this situation.
If the other chain turns out to be invalid or the chain with the snapshot retrieves additional headers so that it's the most-work one again (see functional test), `m_best_header` will cha
...

💬 theStack commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2182987237)
Still trying to fully grasp the `IsMine` inlining commit 89503710386f52d9162f67fcd707eceffa954faa, meanwhile a few comments:
* The PR description is outdated, as it still mentions adding `BerkeleyRODatabase` which was already merged in #26606
* in c653f4fdbfe0607ad9be27e80ad22c9aee314bb7: typo in commit subject (s/MigrateToLegacy/MigrateToDescriptor/)
* To my understanding, the class `LegacyDataSPKM` is supposed to never write to the database (as `BerkeleyRODatabase` obviously doesn't support
...

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2182999892)
@sipa done and managed to clean up `sv2_messages.h` a bit in the process.

💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2183003317)
utACK da205dda14d7e71fe0567944117362c1b820ba8f

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#discussion_r1649141459)
639be6df22800d2e2a170c1af74d51951597834c: this `namespace` is outdated, since this isn't part of the `node` anymore.

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2183005331)
@vasild thank you for your review, all comments addressed.

👍 hebasto approved a pull request: "ci: add option for running tests without volume"
(https://github.com/bitcoin/bitcoin/pull/30310#pullrequestreview-2132950784)
ACK da205dda14d7e71fe0567944117362c1b820ba8f.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1649143746)
Without this change, when there's no ARM machine, it remains pending forever.

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2183009446)
With bigger cache size:

```shell
ccache version 4.9.1
Cacheable calls: 770 / 782 (98.47%)
Hits: 769 / 770 (99.87%)
Direct: 769 / 769 (100.0%)
Preprocessed: 0 / 769 ( 0.00%)
Misses: 1 / 770 ( 0.13%)
Uncacheable calls: 12 / 782 ( 1.53%)
Local storage:
Cache size (GB): 0.2 / 0.2 (96.47%)
Hits: 769 / 770 (99.87%)
Misses: 1 / 770 ( 0.13%)
```

💬 mzumsande commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2183037276)
> I actually thought we did this already some time ago and got confused about that, see [#29996 (comment)](https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1646164004) Is this what you had in mind as well @mzumsande ?

I opened #30320 with my suggestion - I think we have to use `m_best_header` for this and can't use the active chain which only has blocks that were connected (and therefore fully available at that point).