💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382610792)
in 03420020b6b4958ee6765be718724b89cd174d5c

`locktime` doesn't belong to `CoinControl` because it has nothing to do with coins being spent. I'd prefer if we keep `CoinControl` scoped down to only containing data necessary to select and spend coins.

Same is true for some other fields in `CoinControl`. Maybe we should extract parameters not related to coins into a separate struct?

💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382610856)
in c8544398fb945b2402274981b22c70fdd8a67b9e

Same comment as for `locktime`

💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382612557)
in 53cb734591cabb37fc6e3d147a4715203cf15ddb

There is redundancy between `m_script_sig ` + `m_script_witness ` and `m_weight`. One can set all of them and reach and inconsistent state, which will lead to error.

Can we make the interface better?

💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382611119)
I'm not sure about 49ce0fe1e2ec87508aea537d17e6bb13791be1aa and probably would prefer to drop it.

It makes interface less clear. It's not obvious that I need to call `SetTxOut` to make input as external.

💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382612097)
in 59a7c7416313693a3c8219363d9ffee1e2b86d4d

Naming is hard. We already have struct `PreSelectedInputs` which is quite confusing as it refers to different data.

`PreSelectedInputs` is basically `set<COutput>`. It seems we should find a better name for `PreSelectedInputs`. Maybe rename it to `PreSelectedCoins`?

It'd be nice to have consistent terms to refer to different data.
How about we use `input` when we talk about tx.in and related info.
And we use `coin` when its enriched data in
...

💬 maflcko commented on pull request "ci: Drop no longer needed "Fix Visual Studio installation" step":
(https://github.com/bitcoin/bitcoin/pull/28796#issuecomment-1793797549)
lgtm ACK 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da

💬 maflcko commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1793798658)
How much data would this add per year to the repo, assuming updates happen?

💬 maflcko commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1793799659)
I don't think the CI win64-cross build has multiprocess enabled. There is only one non-windows one (multiprocess, i686, DEBUG). Also, I don't think guix has multiprocess enabled, and probably won't have it enabled until at least mid-next year. Anyone is welcome to fix the win64-cross multiprocess build, and even enable it in CI or guix, if they want to.

⚠️ TheBlueMatt opened an issue: "Wallet Missing Balances/Unspent"
(https://github.com/bitcoin/bitcoin/issues/28797)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Wallet is missing the balance for txid d49589ccd8488c56fbf12aee2652f760e1e84e6da1edd3dcf98bf6d556151396. The output address (and input address) are mine:

```
getaddressinfo bc1qag6gtevhvuhafph4d73ajlj5vyhh7k4xff0p4f
{
"address": "bc1qag6gtevhvuhafph4d73ajlj5vyhh7k4xff0p4f",
"scriptPubKey": "0014ea3485e597672fd486f56fa3d97e54612f7f5aa6",
"ismine": true,
"solvable": true,

...

📝 hebasto opened a pull request: "build: Drop no longer needed MSVC warning suppressions"
(https://github.com/bitcoin/bitcoin/pull/28798)

💬 sipa commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1793804415)
@maflcko The asmap file added here is around 1.6 MiB, so assuming we update for every 6 months, probably a bit over 3 MiB per year?

🚀 fanquake merged a pull request: "ci: Drop no longer needed "Fix Visual Studio installation" step"
(https://github.com/bitcoin/bitcoin/pull/28796)

🚀 fanquake merged a pull request: "depends: Bump to capnproto-c++-1.0.1"
(https://github.com/bitcoin/bitcoin/pull/28735)

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1793825982)
Rebased on master to fix silent conflict with new I2P tests.

@maflcko thanks. Can you help me understand why this only broke the previous builds task?

👍 pinheadmz approved a pull request: "doc: Add offline signing tutorial"
(https://github.com/bitcoin/bitcoin/pull/28363#pullrequestreview-1714045182)
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33

Confirmed only updates since last review addressed achow101's nits re: `send` and `blank`

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmVH8gIACgkQ5+KYS2KJ
yToV3Q//TyBSa0j3/cGSVnT7dNZREnfh2ecKV5IueWBK1OnS3Bae1eLFS9hikmuG
gjxgRJFcrQYnwrqst72fkXlhXWnldam9NRsMV7LB8e5Ca
...

💬 ryanofsky commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1793830954)
> I don't think the CI win64-cross build has multiprocess enabled

Thanks, I forgot it would only be built if that flag is enabled

📝 theStack opened a pull request: "wallet: cache descriptor ID to avoid repeated descriptor string creation"
(https://github.com/bitcoin/bitcoin/pull/28799)
Right now a wallet descriptor is converted to it's string representation (via `Descriptor::ToString`) repeatedly at different instances:
- on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
- whenever `DescriptorScriptPubKe
...

💬 cmdruid commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1793929086)
Regtest is not an option if you want to host your own test network for other users to test your software, as anyone can grief the network by generating blocks. Default testnet/signet is too slow for actual testing.

I'd like the ability to host a test network that outside users can use, but not grief, while having fast block times (or even no time restriction at all) so that I can incorporate it into CI/CD for different projects.

This seems like a real obvious tool to have for development o
...

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1793951200)
re-utACK b1b12d1b88492fe66b26243760689c7f040c67c2

👍 Sjors approved a pull request: "wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring"
(https://github.com/bitcoin/bitcoin/pull/28546#pullrequestreview-1714193806)
utACK f06016d77d848133fc6568f287bb86b644c9fa69