💬 hebasto commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182835413)
> Looks good to merge to allow devs to enable this for testing in their fork. However, it should not be enabled in this repo...

Merging without enabling [won't work](https://github.com/bitcoin/bitcoin/actions/runs/9614060766):
> Error
> vmactions/freebsd-vm@v1 is not allowed to be used in bitcoin/bitcoin. Actions in this workflow must be: within a repository owned by bitcoin or matching the following: actions/cache@*, actions/cache/restore@*, actions/cache/save@*, actions/checkout@*, ilamm
...

💬 fanquake commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182839533)
> Merging without enabling won't work:

That seems like a GitHub bug. We shouldn't have to change permissions/configurations in this repository for people to do things in forks.

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1649027490)
hmm good point. I've added `garbage_len` as an optional argument in `generate_keypair_and_garbage()` so that some of the test cases which don't support 0 length garbage can avoid it.

💬 maflcko commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182856049)
> Not encountering an issue in the past does not guarantee the same in the future :)

Correct, but the cost of a trivial post-merge fixup, like adding a missing header or header-guard, in case it happens, should be trivial compared to the overhead to deal with redundant or unrelated intermittent issues, which are ignored by most devs, left to be dealt with by others (see https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue+is%3Aopen+label%3A%22CI+failed%22). If it was free to add more CI tas
...

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