💬 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).

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1649164186)
It was working but I agree handling the error is safer.

Also I was wrong that it would need to handle the specific "address family for hostname not supported" error.

Implemented your suggestion of just returning from the lambda.

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

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2183041904)
> upgrading the existing RPC method, or adding a new one that's minimal in scope and not SV2 opinionated

I don't think our RPC is suitable for this, because it's inherently poll based. We want to push out new block templates as soon as something better is found.

Another major change compared to the `getblocktemplate` RPC is that the TP hold on to templates for a while (currently until after the next block is found, though could be shorter). It needs to do this because solutions for custom
...

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2183053147)
ASan job has gone from 79 minutes down to 46 minutes with cache.

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1649181039)
Thanks fixed.

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1649181380)
Also added a test for this